[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[bug#68405] [PATCH v2] guix: download: Add support for git repositories.
From: |
Romain Garbage |
Subject: |
[bug#68405] [PATCH v2] guix: download: Add support for git repositories. |
Date: |
Fri, 19 Jan 2024 09:53:23 +0100 (CET) |
----- 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