[Buildroot] [PATCH 1/1] support/download: Add git download method for SHAs

[Ricardo Martincoski]

Hello,

On Fri, Oct 14, 2016 at 07:50 PM, Arnout Vandecappelle wrote:

> On 13-10-16 17:50, Bryce Ferguson wrote:
[snip]
>> +++ b/support/download/git
>> @@ -55,6 +55,23 @@ if [ -n "$(_git ls-remote "'${repo}'" "'${cset}'" 2>&1)" ]; then
>>      fi
>>  fi
>>  if [ ${git_done} -eq 0 ]; then
>> +    # See if $cset is a sha that maps to a branch, then shallow clone that branch
>> +    equivalent_ref="$(_git ls-remote --heads --tags "'${repo}'" | awk "/^${cset}/{ print \$2; exit }" | sed 's,refs/\(tags\|heads\)/,,')"
> 
>  There should be a redirect of stderr of ls-remote (we don't want to see any
> errors from it, the relevant error is the one coming from git clone).
> 
>  This line is way too long.
> 
>  Also, you can get rid of the sed by including it in awk:
> 
> 	{ split(\$2, a, \"/\"); print a[3]; exit }

This expression will need be more complex in order to support refs like this:
74c4e04dbb10a5cdeac36e3f1057946e551beb84        refs/heads/feature/vxlan

> 
>  Also, wouldn't it be better to merge this into the first one? ls-remote takes a
> relatively long time, and it seems to be independent of how much refs are
> actually printed, so it seems a waste to do it twice. It does mean that the awk
> program would become a little more complicated because it also has to match the
> cset as a ref.

I agree with merging this 'if' to the previous one.
It also will make the log cleaner, see example messages at the end.

> 
>  Hm, actually, cset may be something like refs/tags/foo so the awk program would
> get quite a bit more complicated... Still, I think it's worth it.

It worth a try.

But maybe it can turn out to be challenging to achieve a single line that
generates the proper reference to be used in the git clone.
In this case we could achieve the same behavior keeping the 2 ifs.
Something like this:

if _git check-ref-format "'${cset}'"; then
# branch or tag, so use the old if (if 'git ls-remote', 'git clone')
else
# sha1, so use the new if (ref='git ls-remote | awk', if ref 'git clone')
fi

Do you think it would be acceptable?

And yet another approach would be to first of all save the output of 'git
ls-remote' with all refs to a temporary file. And then use this file to check
${cset} is a sha1, branch or tag. Later on, in the same script this temp file
could be checked for special refs too (in another patch).
But I don't know if this approach would be acceptable either.

> 
>> +    if [ -n "$equivalent_ref" ]; then
>> +        printf "Doing shallow clone with cset '%s' as '%s'\n" "$cset" "$equivalent_ref"
>> +        if _git clone ${verbose} "${@}" --depth 1 -b "'${equivalent_ref}'" "'${repo}'" "'${basename}'"; then
>> +            if (cd "${basename}" && _git show "'${cset}'" >/dev/null 2>&1); then
> 
>  Can you explain why this bit is still needed? Is there any way that the clone
> could succeed but the cset not be present? It would mean that ls-remote was
> lying to us, no?
> 
>  I can invent one scenario: the symbolic ref got updated between the ls-remote
> and the clone, which means that the wrong commit got shallow-cloned, and the
> actual cset is not reachable. Hm, ok, can you add a comment to explain that?
> 
> 
>  Otherwise looks good :-)
> 
>  Regards,
>  Arnout
[snip]

With this patch applied and using this dirty hack:

TMUX_VERSION = 74c4e04dbb10a5cdeac36e3f1057946e551beb84
TMUX_SITE = https://review.openswitch.net/openswitch/ops
TMUX_SITE_METHOD = git

I get these log messages:

Doing shallow clone
Cloning into 'tmux-74c4e04dbb10a5cdeac36e3f1057946e551beb84'...
warning: Could not find remote branch 74c4e04dbb10a5cdeac36e3f1057946e551beb84 to clone.
fatal: Remote branch 74c4e04dbb10a5cdeac36e3f1057946e551beb84 not found in upstream origin
Shallow clone failed, falling back to doing a full clone
Doing shallow clone with cset '74c4e04dbb10a5cdeac36e3f1057946e551beb84' as 'feature/vxlan'
Cloning into 'tmux-74c4e04dbb10a5cdeac36e3f1057946e551beb84'...
remote: Counting objects: 585, done

Regards,
Ricardo