Stefan Beller <sbeller@xxxxxxxxxx> writes:
> On Mon, Feb 22, 2016 at 4:01 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>>
>> Stefan Beller <sbeller@xxxxxxxxxx> 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
>>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html