[bug#68405] [PATCH v2] guix: download: Add support for git repositories.

[Romain Garbage <romain.garbage@inria.fr>]

----- Mail original -----
> De: "Maxim Cournoyer" <maxim.cournoyer@gmail.com>
> À: "Romain Garbage" <romain.garbage@inria.fr>
> Cc: 68405@debbugs.gnu.org, "Ludovic Courtès" <ludo@gnu.org>
> Envoyé: Vendredi 19 Janvier 2024 04:16:42
> Objet: Re: bug#68405: [PATCH v2] guix: download: Add support for git repositories.

> Hello,

Hi Maxim,

Thank you very much for your review. 

I actually pushed a v3 of this patch last Tuesday, somehow the issues have not been merged together.

The new patch is available here: https://issues.guix.gnu.org/68499

I will address your comments below.

> Romain GARBAGE <romain.garbage@inria.fr> writes:
> 
>> Added `--recursive' option.
> 
> I still see a TODO about supporting recursive repos in the code.  Is
> that still the case?

It was removed in v3.
 
>> Removed `pk' call.
>>
>> * guix/scripts/download.scm (git-download-to-store*): Add new variable.
>>   (copy-recursively-without-dot-git): New variable.
>>   (git-download-to-file): Add new variable.
>>   (show-help): Add 'git', 'commit', 'branch' and 'recursive'options
>>   help message.
>>   (%default-options): Add default value for 'git-reference' and
>>   'recursive' options.
>>   (%options): Add 'git', 'commit', 'branch' and 'recursive' command
>>   line options.
>>   (guix-download) [hash]: Compute hash with 'file-hash*' instead of
>>   'port-hash' from (gcrypt hash) module. This allows us to compute
>>   hashes for directories.
>> * doc/guix.texi (Invoking guix-download): Add @item entries for
>>   `git', `commit', `branch' and `recursive' options. Add a paragraph in
>>   the introduction.
>> * tests/guix-download.sh: New tests.
> 
> This sounds good and is something that I'm many many of us have wanted
> for some time.  Thank you for working on it!
> 
> Nitpick about the commit message: the convention seems to be to not use
> a hanging indent when writing GNU ChangeLog messages.

I'll remove it then :)
 
[...]

>> +;; This is a simplified version of 'copy-recursively'.
>> +;; It allows us to filter out the ".git" subfolder.
>> +;; TODO: Remove when 'copy-recursively' supports '#:select?'.
> 
> Is a #:select? planned for copy-recursively?  (in the works?)

For the record, it is the issue #68406 (thanks for reviewing it too!)

[...]
 
> Some lines look like > 80 chars here.  Please break long lines
> accordingly.

Will fix.

[...]

> Here also there are some too long lines in the above hunks; please break
> long lines so they fit within the 80 characters limit.

Ditto.
 
>>             (fmt   (assq-ref opts 'format)))
>>        (format #t "~a~%~a~%" path (fmt hash))
>>        #t)))
>> diff --git a/tests/guix-download.sh b/tests/guix-download.sh
>> index f4cb335eef..3bf63c4b12 100644
>> --- a/tests/guix-download.sh
>> +++ b/tests/guix-download.sh
>> @@ -45,4 +45,46 @@ cmp "$output" "$abs_top_srcdir/README"
>>  # This one should fail.
>>  guix download "file:///does-not-exist" "file://$abs_top_srcdir/README" && false
>>
>> +# Test git support with local repository
> 
> Nitpick: please punctuate standalone comments (here, a missing period).

Will do.

>> +test_directory="$(mktemp -d)"
>> +trap 'chmod -Rf +w "$test_directory"; rm -rf "$test_directory" ; rm -f
>> "$output"' EXIT
> 
> the 'chmod' doesn't seem to be useful; since we force removing with -f ?

I copied it from another test :)
I will remove it.

> And where did the $output variable come from?

It comes from L39 and is used in an already existing test.

[...]

>> +# Same but download to file instead of store
>> +tmpdir="t-archive-dir-$$"
>> +trap 'chmod -Rf +w "$test_directory"; rm -rf "$test_directory" ; rm -f
>> "$output" ; rm -rf "$tmpdir"' EXIT
> 
> It'd look nicer if there was a single global trap call at the top of
> these tests.  Don't forget to  punctuate your comments :-).

Ok, so I'll move all the temporary file/directory creation/definition to the top together with the trap call definition.

I'll submit a v4 with these changes.

Thanks again for reviewing.

-- 
Romain