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

  • Done
  • quality assurance status badge
Details
3 participants
  • Leo Famulari
  • Ludovic Courtès
  • Marius Bakke
Owner
unassigned
Submitted by
Marius Bakke
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 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(-)

Toggle diff (32 lines)
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)))
(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 from
scratch without your patch and it crashed:

------
LOAD gnu/tests/install.scm
Backtrace:
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/kgACgkQJkb6MLrK
fwiRkRAAkygHaxwgiUyYKDRaO+Ip7KzFpR80lDagpzDFd9l2mnFYV8krpOBPqcdI
886F9L/DJPjPADYtmX7s/6b4nOhSrrnUiL8dhvD7PEv3W0S6lhZH1rKQhRbZrnj1
/MXxPEuHyT3Uv8aiIgxmA6jb+pRMXGGBZzHZ6VVs1DFyRr1JN9PpWkVXceq/3gD0
lF6O6X5vOR4vc41rbGBSEhdTzCAvtl290nKCMm9f5Wpr3cgPqwJS1JrMOzBJcNry
ld1ebsViD/Qzbxs8NjAUTYHhXpTteDqfDk7oDkolVTLwrzrjFI4g4JgKasf4tb6f
+i3+Zy6a7PGeoRfk0dykm3TfHmUnQrnuIsh2n7T6X6frc09hwfGVc9TEofybu39x
pNmXftaXmDqMIXuw5vDRD80pF66lpucHmSjfsumSmScRRQXPYwYzhWqSNH70mnlm
+fEXZxcCkSXt76Rln04vQShiVSa/voJthx5WPcwCuCj6SuW5LWIWXLOAUIZelPR6
yvTMqlMwgWE6J3CXFF7Y9QCWgrloAuQCBjpGhKPqlT4oVYNlr1dfXHDzIM4K8IIh
gngK6m0hUckhxDBZJOnlN/ss79a7MRvZRiPe7doqsxppa82n84N9mOHkAWW4tpX7
Bd52s8MqrePIpFNYsX5OT4wr3RK8WF+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 needed
here.

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 in
aed0a594058a59bc3bb1d2686391dc0e8a181b1f. :-)

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 I
managed to simplify the code further by assuming that is the case:
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(-)

Toggle diff (35 lines)
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) "/"))
(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
WDYT?
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCgAdFiEEu7At3yzq9qgNHeZDoqBt8qM6VPoFAlunkWUACgkQoqBt8qM6
VPo9lAf/TPJsRa7pJU5sQZ80ls2SGzxjuXtMNPeuiDKEwEG3pI86U1/wX+xnFyvD
Vndt0rNUhmo0LPya1MyIoYFQy5pBj8R1RyhZNldthCn4maMx6/WVhITEKpnU/NJl
k5y9P52j7LrZdoMq4p55UpL9QSJee3rJ0GA2sK23W2V2G2Hog1hMIwxH9WGAy6UX
qCL3Lp8a3Id2d7uRy/wq9cztMTjlJ+LYahAaQggE4pSEJk2dILlgc9yHam/HfHvV
jkW9BNNshF68D+aOPNn/wfBtELw5Qa/6BeVsrPoS/GACMYJAMS9ZM++kXNb7aA5o
ef1Bjp0bEyZnbUqg9/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-----

iQEzBAEBCgAdFiEEu7At3yzq9qgNHeZDoqBt8qM6VPoFAluqubAACgkQoqBt8qM6
VPrqUQf+INwcG095FB4qf3gOCStrlA4/104C86u89OzRvjJ/yAu1Pp1q0smIgTlT
sWKOw4JbBypMeAJW1t1OXpOMA/igFEoWtg4v8IyB2A6bIS57+m8bA3f190f5Y5oz
+hk4Irw5+3tAD1uv0eZiPhyJZAr4y/+8hMI6Gi65f0VuDS/IulrYxyylM+tGcFMh
bzbPL7fJzeXsE8gZ63R3g59h+FfBI3PQkYpr7Ns/XInMo7zkdtizqbaOWuZArehb
rMd8nxMeoIyY1dxg82i5uNKX10DAzGuvVTRcNksdXwgZokeoD178Nb+wI8rutPyn
wYyVeVFcfAg+5fhq+NHJbORiZpYckw==
=0BV1
-----END PGP SIGNATURE-----

Closed
?