[PATCH] git-download: Don't assume the git checkout is the parent of ".git".

DoneSubmitted by Marius Bakke.
Details
3 participants
  • Leo Famulari
  • Ludovic Courtès
  • Marius Bakke
Owner
unassigned
Severity
normal
M
M
Marius Bakke wrote on 15 Sep 2018 12:10
(address . guix-patches@gnu.org)
20180915101034.10102-1-mbakke@fastmail.com
This makes it play nicely with worktrees.
* guix/git-download.scm (git-file-list): Use REPOSITORY-WORKING-DIRECTORY tolocate checkout. Rename from "top" to "workdir".---
Notes: Guix, This fixes (current-guix) for me in a worktree. Testing needed on other git setups!
guix/git-download.scm | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
Toggle diff (32 lines)diff --git a/guix/git-download.scm b/guix/git-download.scmindex 24cf11be5..a7c8173f4 100644--- a/guix/git-download.scm+++ b/guix/git-download.scm@@ -158,19 +158,22 @@ also includes directories, not just regular files. The returned file names are relative to DIRECTORY, which is not necessarily the root of the checkout." (let* ((directory (canonicalize-path directory)) (dot-git (repository-discover directory))- (top (dirname dot-git)) (repository (repository-open dot-git))+ (workdir (canonicalize-path+ ;; XXX: This variable is mistakenly private in Guile-Git 0.1.0.+ ((@@ (git repository) repository-working-directory)+ repository))) (head (repository-head repository)) (oid (reference-target head)) (commit (commit-lookup repository oid)) (tree (commit-tree commit)) (files (tree-list tree))) (repository-close! repository)- (if (string=? top directory)+ (if (string=? workdir directory) files (let ((relative (string-append (string-drop directory- (+ 1 (string-length top)))+ (+ 1 (string-length workdir))) "/"))) (filter-map (lambda (file) (and (string-prefix? relative file)-- 2.19.0
L
L
Leo Famulari wrote on 17 Sep 2018 21:19
(name . Marius Bakke)(address . mbakke@fastmail.com)(address . 32740@debbugs.gnu.org)
20180917191939.GA7741@jasmine.lan
On Sat, Sep 15, 2018 at 12:10:34PM +0200, Marius Bakke wrote:
Toggle quote (5 lines)> This makes it play nicely with worktrees.> > * guix/git-download.scm (git-file-list): Use REPOSITORY-WORKING-DIRECTORY to> locate checkout. Rename from "top" to "workdir".
Thanks!
Toggle quote (8 lines)> ---> > Notes:> Guix,> > This fixes (current-guix) for me in a worktree. Testing needed on other git> setups!
To clarify what this fixes, I tried building a fresh worktree fromscratch without your patch and it crashed:
------ LOAD gnu/tests/install.scmBacktrace:In srfi/srfi-1.scm: 592:29 19 (map1 (#<<service> type: #<service-type mingetty cf7?> ?)) 592:29 18 (map1 (#<<service> type: #<service-type mingetty cf7?> ?)) 592:29 17 (map1 (#<<service> type: #<service-type mingetty cf7?> ?)) 592:29 16 (map1 (#<<service> type: #<service-type syslog c7537?> ?)) 592:17 15 (map1 (#<<service> type: #<service-type guix 43f8be0?> ?))In ice-9/eval.scm: 196:43 14 (_ #(#(#(#<directory (gnu tests install) 110a20a0>) ?) ?)) 293:34 13 (_ #(#(#(#<directory (gnu tests install) 110a20a0>) ?) ?)) 293:34 12 (_ #(#(#(#<directory (gnu packages package-manag?> ?)) ?)) 174:20 11 (_ #(#(#(#<directory (gnu packages package-manag?> ?)) ?)) 177:49 10 (lp (#<procedure ed4d8c0 at ice-9/eval.scm:282:4 (en?> ?)) 177:49 9 (lp (#<procedure ed4d8a0 at ice-9/eval.scm:282:4 (en?> ?)) 177:49 8 (lp (#<procedure ed4d880 at ice-9/eval.scm:282:4 (en?> ?)) 177:32 7 (lp (#<procedure ed4d860 at ice-9/eval.scm:182:7 (env)>))In unknown file: 6 (force #<promise #<procedure 8aa6580 at ice-9/eval.scm:?>)In ice-9/eval.scm: 293:34 5 (_ #(#(#<directory (gnu packages package-management?> ?)))In ice-9/boot-9.scm: 829:9 4 (catch git-error #<procedure ed4d460 at ice-9/eval.scm?> ?)In ice-9/eval.scm: 293:34 3 (_ #(#(#<directory (guix git-download) 3ec5c80> "/ho?"))) 293:34 2 (_ #(#(#(#(#(#(#(#(#(#(#(#) ?) ?) ?) ?) ?) ?) ?) ?) ?) ?)) 159:9 1 (_ #(#(#(#(#(#(#(#(#(#(#(#) ?) ?) ?) ?) ?) ?) ?) ?) ?) ?))In unknown file: 0 (string-drop "/home/leo/tmp/mytest" 35)
ERROR: In procedure string-drop:Value out of range 0 to 20: 35------
Your patch avoids this crash, which I was stymied by a couple days ago!Has it always been there? I've been using Guix worktrees for a while.
-----BEGIN PGP SIGNATURE-----
iQIzBAABCAAdFiEEsFFZSPHn08G5gDigJkb6MLrKfwgFAluf/kgACgkQJkb6MLrKfwiRkRAAkygHaxwgiUyYKDRaO+Ip7KzFpR80lDagpzDFd9l2mnFYV8krpOBPqcdI886F9L/DJPjPADYtmX7s/6b4nOhSrrnUiL8dhvD7PEv3W0S6lhZH1rKQhRbZrnj1/MXxPEuHyT3Uv8aiIgxmA6jb+pRMXGGBZzHZ6VVs1DFyRr1JN9PpWkVXceq/3gD0lF6O6X5vOR4vc41rbGBSEhdTzCAvtl290nKCMm9f5Wpr3cgPqwJS1JrMOzBJcNryld1ebsViD/Qzbxs8NjAUTYHhXpTteDqfDk7oDkolVTLwrzrjFI4g4JgKasf4tb6f+i3+Zy6a7PGeoRfk0dykm3TfHmUnQrnuIsh2n7T6X6frc09hwfGVc9TEofybu39xpNmXftaXmDqMIXuw5vDRD80pF66lpucHmSjfsumSmScRRQXPYwYzhWqSNH70mnlm+fEXZxcCkSXt76Rln04vQShiVSa/voJthx5WPcwCuCj6SuW5LWIWXLOAUIZelPR6yvTMqlMwgWE6J3CXFF7Y9QCWgrloAuQCBjpGhKPqlT4oVYNlr1dfXHDzIM4K8IIhgngK6m0hUckhxDBZJOnlN/ss79a7MRvZRiPe7doqsxppa82n84N9mOHkAWW4tpX7Bd52s8MqrePIpFNYsX5OT4wr3RK8WF+3LhNK9hA9vW/zp2kijLg==1JAQ-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 21 Sep 2018 11:48
(name . Marius Bakke)(address . mbakke@fastmail.com)(address . 32740@debbugs.gnu.org)
87sh235ggi.fsf@gnu.org
Hello Marius!
Marius Bakke <mbakke@fastmail.com> skribis:
Toggle quote (30 lines)> This makes it play nicely with worktrees.>> * guix/git-download.scm (git-file-list): Use REPOSITORY-WORKING-DIRECTORY to> locate checkout. Rename from "top" to "workdir".> --->> Notes:> Guix,> > This fixes (current-guix) for me in a worktree. Testing needed on other git> setups!>> guix/git-download.scm | 9 ++++++---> 1 file changed, 6 insertions(+), 3 deletions(-)>> diff --git a/guix/git-download.scm b/guix/git-download.scm> index 24cf11be5..a7c8173f4 100644> --- a/guix/git-download.scm> +++ b/guix/git-download.scm> @@ -158,19 +158,22 @@ also includes directories, not just regular files. The returned file names> are relative to DIRECTORY, which is not necessarily the root of the checkout."> (let* ((directory (canonicalize-path directory))> (dot-git (repository-discover directory))> - (top (dirname dot-git))> (repository (repository-open dot-git))> + (workdir (canonicalize-path> + ;; XXX: This variable is mistakenly private in Guile-Git 0.1.0.> + ((@@ (git repository) repository-working-directory)> + repository)))
I think we can avoid the call to ‘canonicalize-path’ here, can’t we?It’s costly, and since we did it just above, it shouldn’t be neededhere.
Otherwise LGTM, thank you!
Ludo’.
L
L
Ludovic Courtès wrote on 21 Sep 2018 11:49
(name . Leo Famulari)(address . leo@famulari.name)
87o9cr5gfb.fsf@gnu.org
Hi Leo,
Leo Famulari <leo@famulari.name> skribis:
Toggle quote (3 lines)> Your patch avoids this crash, which I was stymied by a couple days ago!> Has it always been there? I've been using Guix worktrees for a while.
I kindly introduced the bug inaed0a594058a59bc3bb1d2686391dc0e8a181b1f. :-)
Ludo’.
M
M
Marius Bakke wrote on 23 Sep 2018 15:13
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 32740@debbugs.gnu.org)
87y3bsl5lm.fsf@fastmail.com
ludo@gnu.org (Ludovic Courtès) writes:
Toggle quote (38 lines)> Hello Marius!>> Marius Bakke <mbakke@fastmail.com> skribis:>>> This makes it play nicely with worktrees.>>>> * guix/git-download.scm (git-file-list): Use REPOSITORY-WORKING-DIRECTORY to>> locate checkout. Rename from "top" to "workdir".>> --->>>> Notes:>> Guix,>> >> This fixes (current-guix) for me in a worktree. Testing needed on other git>> setups!>>>> guix/git-download.scm | 9 ++++++--->> 1 file changed, 6 insertions(+), 3 deletions(-)>>>> diff --git a/guix/git-download.scm b/guix/git-download.scm>> index 24cf11be5..a7c8173f4 100644>> --- a/guix/git-download.scm>> +++ b/guix/git-download.scm>> @@ -158,19 +158,22 @@ also includes directories, not just regular files. The returned file names>> are relative to DIRECTORY, which is not necessarily the root of the checkout.">> (let* ((directory (canonicalize-path directory))>> (dot-git (repository-discover directory))>> - (top (dirname dot-git))>> (repository (repository-open dot-git))>> + (workdir (canonicalize-path>> + ;; XXX: This variable is mistakenly private in Guile-Git 0.1.0.>> + ((@@ (git repository) repository-working-directory)>> + repository)))>> I think we can avoid the call to ‘canonicalize-path’ here, can’t we?> It’s costly, and since we did it just above, it shouldn’t be needed> here.
You're right. I mostly needed it because (repository-working-directory)includes a trailing slash. This behaviour seems to be consistent, so Imanaged to simplify the code further by assuming that is the case:
From e8b443e1de0a5b1e3dfeee024cd0625790f4f834 Mon Sep 17 00:00:00 2001From: Marius Bakke <mbakke@fastmail.com>Date: Sat, 15 Sep 2018 11:53:40 +0200Subject: [PATCH] git-download: Don't assume the working directory is the parent of ".git".
* guix/git-download.scm (git-file-list): Use REPOSITORY-WORKING-DIRECTORY tolocate checkout. Rename from "top" to "workdir".--- guix/git-download.scm | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
Toggle diff (35 lines)diff --git a/guix/git-download.scm b/guix/git-download.scmindex 24cf11be5..eb20927c7 100644--- a/guix/git-download.scm+++ b/guix/git-download.scm@@ -156,22 +156,21 @@ HASH-ALGO (a symbol). Use NAME as the file name, or a generic name if #f." The result is similar to that of the 'git ls-files' command, except that it also includes directories, not just regular files. The returned file names are relative to DIRECTORY, which is not necessarily the root of the checkout."- (let* ((directory (canonicalize-path directory))+ (let* ((directory (string-append (canonicalize-path directory) "/")) (dot-git (repository-discover directory))- (top (dirname dot-git)) (repository (repository-open dot-git))+ ;; XXX: This procedure is mistakenly private in Guile-Git 0.1.0.+ (workdir ((@@ (git repository) repository-working-directory)+ repository)) (head (repository-head repository)) (oid (reference-target head)) (commit (commit-lookup repository oid)) (tree (commit-tree commit)) (files (tree-list tree))) (repository-close! repository)- (if (string=? top directory)+ (if (string=? workdir directory) files- (let ((relative (string-append- (string-drop directory- (+ 1 (string-length top)))- "/")))+ (let ((relative (string-drop directory (string-length workdir)))) (filter-map (lambda (file) (and (string-prefix? relative file) (string-drop file (string-length relative))))-- 2.19.0
-----BEGIN PGP SIGNATURE-----
iQEzBAEBCgAdFiEEu7At3yzq9qgNHeZDoqBt8qM6VPoFAlunkWUACgkQoqBt8qM6VPo9lAf/TPJsRa7pJU5sQZ80ls2SGzxjuXtMNPeuiDKEwEG3pI86U1/wX+xnFyvDVndt0rNUhmo0LPya1MyIoYFQy5pBj8R1RyhZNldthCn4maMx6/WVhITEKpnU/NJlk5y9P52j7LrZdoMq4p55UpL9QSJee3rJ0GA2sK23W2V2G2Hog1hMIwxH9WGAy6UXqCL3Lp8a3Id2d7uRy/wq9cztMTjlJ+LYahAaQggE4pSEJk2dILlgc9yHam/HfHvVjkW9BNNshF68D+aOPNn/wfBtELw5Qa/6BeVsrPoS/GACMYJAMS9ZM++kXNb7aA5oef1Bjp0bEyZnbUqg9/V9itdbq/Rr1Q===F3Sd-----END PGP SIGNATURE-----
L
L
Ludovic Courtès wrote on 23 Sep 2018 21:55
(name . Marius Bakke)(address . mbakke@fastmail.com)(address . 32740@debbugs.gnu.org)
87d0t4rnss.fsf@gnu.org
Hello!
Marius Bakke <mbakke@fastmail.com> skribis:
Toggle quote (23 lines)> From e8b443e1de0a5b1e3dfeee024cd0625790f4f834 Mon Sep 17 00:00:00 2001> From: Marius Bakke <mbakke@fastmail.com>> Date: Sat, 15 Sep 2018 11:53:40 +0200> Subject: [PATCH] git-download: Don't assume the working directory is the> parent of ".git".>> * guix/git-download.scm (git-file-list): Use REPOSITORY-WORKING-DIRECTORY to> locate checkout. Rename from "top" to "workdir".> ---> guix/git-download.scm | 13 ++++++-------> 1 file changed, 6 insertions(+), 7 deletions(-)>> diff --git a/guix/git-download.scm b/guix/git-download.scm> index 24cf11be5..eb20927c7 100644> --- a/guix/git-download.scm> +++ b/guix/git-download.scm> @@ -156,22 +156,21 @@ HASH-ALGO (a symbol). Use NAME as the file name, or a generic name if #f."> The result is similar to that of the 'git ls-files' command, except that it> also includes directories, not just regular files. The returned file names> are relative to DIRECTORY, which is not necessarily the root of the checkout."> - (let* ((directory (canonicalize-path directory))> + (let* ((directory (string-append (canonicalize-path directory) "/"))
Could you just add a comment here explaining that‘repository-working-directory’ always appends a trailing slash?
Otherwise LGTM, thank you!
Ludo’.
M
M
Marius Bakke wrote on 26 Sep 2018 00:41
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 32740-done@debbugs.gnu.org)
87lg7pkxn3.fsf@fastmail.com
ludo@gnu.org (Ludovic Courtès) writes:
Toggle quote (30 lines)> Hello!>> Marius Bakke <mbakke@fastmail.com> skribis:>>> From e8b443e1de0a5b1e3dfeee024cd0625790f4f834 Mon Sep 17 00:00:00 2001>> From: Marius Bakke <mbakke@fastmail.com>>> Date: Sat, 15 Sep 2018 11:53:40 +0200>> Subject: [PATCH] git-download: Don't assume the working directory is the>> parent of ".git".>>>> * guix/git-download.scm (git-file-list): Use REPOSITORY-WORKING-DIRECTORY to>> locate checkout. Rename from "top" to "workdir".>> --->> guix/git-download.scm | 13 ++++++------->> 1 file changed, 6 insertions(+), 7 deletions(-)>>>> diff --git a/guix/git-download.scm b/guix/git-download.scm>> index 24cf11be5..eb20927c7 100644>> --- a/guix/git-download.scm>> +++ b/guix/git-download.scm>> @@ -156,22 +156,21 @@ HASH-ALGO (a symbol). Use NAME as the file name, or a generic name if #f.">> The result is similar to that of the 'git ls-files' command, except that it>> also includes directories, not just regular files. The returned file names>> are relative to DIRECTORY, which is not necessarily the root of the checkout.">> - (let* ((directory (canonicalize-path directory))>> + (let* ((directory (string-append (canonicalize-path directory) "/"))>> Could you just add a comment here explaining that> ‘repository-working-directory’ always appends a trailing slash?
Good idea.
Toggle quote (2 lines)> Otherwise LGTM, thank you!
Pushed as 280fc8351230a8fea086d9bbce919ba8395f312c, thanks!
-----BEGIN PGP SIGNATURE-----
iQEzBAEBCgAdFiEEu7At3yzq9qgNHeZDoqBt8qM6VPoFAluqubAACgkQoqBt8qM6VPrqUQf+INwcG095FB4qf3gOCStrlA4/104C86u89OzRvjJ/yAu1Pp1q0smIgTlTsWKOw4JbBypMeAJW1t1OXpOMA/igFEoWtg4v8IyB2A6bIS57+m8bA3f190f5Y5oz+hk4Irw5+3tAD1uv0eZiPhyJZAr4y/+8hMI6Gi65f0VuDS/IulrYxyylM+tGcFMhbzbPL7fJzeXsE8gZ63R3g59h+FfBI3PQkYpr7Ns/XInMo7zkdtizqbaOWuZArehbrMd8nxMeoIyY1dxg82i5uNKX10DAzGuvVTRcNksdXwgZokeoD178Nb+wI8rutPynwYyVeVFcfAg+5fhq+NHJbORiZpYckw===0BV1-----END PGP SIGNATURE-----
Closed
?
Your comment

This issue is archived.

To comment on this conversation send email to 32740@debbugs.gnu.org