Re: [PATCH 1/7] guix: git: Support shallow git clones if a tag is available

[ng0]

Ludovic Courtès writes:

> Andy Wingo <address@hidden> skribis:
>
>> * guix/build/git.scm (git-fetch): Instead of cloning the remote repo, use the
>>   lower-level "init" / "fetch" / "checkout" operations.  This lets us make a
>>   shallow checkout if we are checking out a tag.
>>
>> * guix/git-download.scm (<git-reference>): Add tag field.
>>   (git-fetch): Support git references with tags but no commits.
>
> This sounds like a nice improvement.  Please move the “This lets us”
> sentence to a comment, and drop the newline between entries.
>
>> --- a/guix/build/git.scm
>> +++ b/guix/build/git.scm
>
> Make sure to add a copyright line in there.
>
> [...]
>
>>    git-reference make-git-reference
>>    git-reference?
>>    (url        git-reference-url)
>> -  (commit     git-reference-commit)
>> +  (commit     git-reference-commit (default #f))
>> +  (tag        git-reference-tag (default #f))
>
> So far we’ve been using ‘commit’ to denote commits or tags.  I guess a
> bunch of packages will need to be updated.
>
> There’s the issue that one could erroneously specify both ‘commit’ and
> ‘tag’, and then one of them would be ignored.
>
> What about doing something like:
>
>   1. renaming ‘commit’ to ‘spec’;
>   2. adding a ‘type’ field which would default to 'commit and could be
>      changed to 'tag
>
> ?

Was the idea here to change the code to make use of spec inside
the package definitions and other modules which use git-fetch, so
that all instances of "commit" have to be replaced?

I have Andy's patch applied now, but to debug the outcome of this
(commit is no longer a bound variable), I need to know what the
intention was.

>> +                   ;; FIXME: Pass #:tag when fixed daemons are widely
>> +                   ;; deployed.
>> +                   ;; #:tag '#$(git-reference-tag ref)
>
> The daemon is not involved here, AFAICS, so you can uncomment the line.
>
> Thanks,
> Ludo’.


-- 
ng0 . https://www.inventati.org/patternsinthechaos/