Re: [PATCH 2/2] submodule: Try harder to fetch needed sha1 by direct fetching sha1

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> On Mon, Feb 22, 2016 at 4:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Stefan Beller <sbeller@google.com> writes:
>>
>> > We may want to discuss the error message. It is the same as in the case
>> > before (git-fetch <no args>), this is good for translation, but may be bad
>> > for the user as we may want to give extra information.
>> > ("We fetched for you and it worked, however the sha1 was not included,
>> > so we fetched again the server broke it, so we error out.  You used to
>> > see error message: ....")
>> >
>> > Although this may be too much information?
>>
>> Now the "go to that submodule directory and see if $sha1 is
>> reachable from any of the ref" check is written twice exactly the
>> same way, it deserves to become a small helper function, I think.
>>
>> Perhaps something along this line?
>
> This looks very readable.
>
> Feel free to drop both my patches and just introduce this patch as yours!

Nah, this is not my itch but yours ;-)

Besides, have we agreed what error message we want to give to the
users yet?

>>
>>  git-submodule.sh | 29 ++++++++++++++++++++++++++---
>>  1 file changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index 9bc5c5f..836348f 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -591,6 +591,24 @@ cmd_deinit()
>>         done
>>  }
>>
>> +is_tip_reachable () (
>> +       clear_local_git_env
>> +       cd "$1" &&
>> +       rev=$(git rev-list -n 1 "$2" --not --all 2>/dev/null) &&
>> +       test -z "$rev"
>
> While we're talking about this code anyway, I wondered if we can make it
> more cryptic again (just kidding!) and shorten it a bit by using the
>
>     is_tip_reachable () (
>         clear_local_git_env
>         test -z $(git -C $1 rev-list -n 1 "$2" --not --all 2>/dev/null)
>     )
>
>> +)
>> +
>> +fetch_in_submodule () (
>> +       clear_local_git_env
>> +       cd "$1" &&
>> +       case "$2" in
>> +       '')
>> +               git fetch ;;
>> +       *)
>> +               git fetch $(get_default_remote) "$2" ;;
>> +       esac
>> +)
>> +
>>  #
>>  # Update each submodule path to correct revision, using clone and checkout as needed
>>  #
>> @@ -745,9 +763,14 @@ Maybe you want to use 'update --init'?")"
>>                         then
>>                                 # Run fetch only if $sha1 isn't present or it
>>                                 # is not reachable from a ref.
>> -                               (clear_local_git_env; cd "$sm_path" &&
>> -                                       ( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&
>> -                                        test -z "$rev") || git-fetch)) ||
>> +                               is_tip_reachable "$sm_path" "$sha1" ||
>> +                               fetch_in_submodule "$sm_path" ||
>> +                               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" "$sha1" ||
>
> For another split second I wondered about the return code of is_tip_reachable,
> if the result is actually negated, but reading the chaining here, makes sense.
>
> Thanks,
> Stefan
>
>>                                 die "$(eval_gettext "Unable to fetch in submodule path '\$displaypath'")"
>>                         fi
>>