Re: [PATCH] git-submodule.sh: try harder to fetch a submodule

[Stefan Beller <sbeller@google.com>]

On Fri, May 11, 2018 at 4:28 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi,
>
> Stefan Beller wrote:
>
>> This is the logical continuum of fb43e31f2b4 (submodule: try harder to
>> fetch needed sha1 by direct fetching sha1, 2016-02-23) and fixes it as
>> some assumptions were not correct.
>
> Interesting.
>
> I think what would help most is an example set of commands I can use
> to reproduce this (bonus points if in the form of a test).

I tried coming up with a test in git-core that produces a remote similar to
Gerrit, and let me tell you, it's not Git that is weird here. ;)

>> > If $sha1 was not part of the default fetch ... fail ourselves here
>> assumes that the fetch_in_submodule only fails when the serverside does
>
> I'm having some trouble with the formatting here.  Is the part
> preceded by '>' a quote, and if so a quote from what?

The quote is from fb43e31f2b4.

>> There are other failures, why such a fetch may fail, such as
>>     fatal: Couldn't find remote ref HEAD
>> which can happen if the remote side doesn't advertise HEAD. Not advertising
>
> nit: it can be useful to have a blank line before and after such
> example output to help both my eyes and tools like "git log
> --format='%w(100)%b'" to understand the formatting.

Why would you use this formatting?

>
>> HEAD is allowed by the protocol spec and would happen, if HEAD points at a
>> ref, that this user cannot see (due to ACLs for example).
>
> A more typical example would be if the ref simply doesn't exist (i.e.,
> is a branch yet to be born).

Oh, I checked that but not for the submodule case, let me check that.

>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index 24914963ca2..13b378a6c8f 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -614,7 +614,6 @@ cmd_update()
>>                               # is not reachable from a ref.
>>                               is_tip_reachable "$sm_path" "$sha1" ||
>>                               fetch_in_submodule "$sm_path" $depth ||
>
> Is keeping the '||' at the end of this line intended?

yes, as we only want to run the code below if there was some error.

>> -                             die "$(eval_gettext "Unable to fetch in submodule path '\$displaypath'")"
>>
>>                               # Now we tried the usual fetch, but $sha1 may
>>                               # not be reachable from any of the refs
>>                               is_tip_reachable "$sm_path" "$sha1" ||
>>                               fetch_in_submodule "$sm_path" $depth "$sha1" ||
>>                               die "$(eval_gettext "Fetched in submodule path '\$displaypath', but it did not contain \$sha1. Direct fetching of that commit failed.")"
>
> Should this error message be changed?

I don't think so?