[PATCH 0/2] Submodule harden fetches (WAS: submodule: Fetch the direct sha1 first)

[PATCH 0/2] Submodule harden fetches (WAS: submodule: Fetch the direct sha1 first)

[Stefan Beller]

A simple patch evolves into a series!

First we'll fix a bug by adding the --objects switch to rev-list to
have a stricter check for the desired commit to be there. IIUC this
is not 100% right yet, but it improves the situation.

The second patch introduces an extra fetch in case the first fetch
did not yield the expected commit, using the pattern

    # first 4 lines unchanged:
    if $sha1's history and objects are incomplete:
        fetch ;# normally just like we have done before
    else 
        die ...
    # new code:     
    if $sha1's history and objects are still incomplete:
        fetch $sha1
    else
        die ... 
        
Thanks,
Stefan

Stefan Beller (2):
  submodule: Include check for objects when fetching
  submodule: Try harder to fetch needed sha1 by direct fetching sha1

 git-submodule.sh | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

-- 
2.7.0.rc0.34.ga06e0b3.dirty

[PATCH 1/2] submodule: Include check for objects when fetching

[Stefan Beller]

Junio wrote:
> To be complete, the rev-list command line should also run with
> "--objects"; after all, a commit walker fetch may have downloaded
> commit chain completely but haven't fetched necessary trees and
> blobs when it was killed, and "rev-list $sha1 --not --all" would not
> catch such a breakage without "--objects".

By adding the --objects switch to rev-list we make sure to do a complete
check.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 9bc5c5f..f5d6675 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -746,7 +746,7 @@ Maybe you want to use 'update --init'?")"
 				# 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) &&
+					( (rev=$(git rev-list --objects -n 1 $sha1 --not --all 2>/dev/null) &&
 					 test -z "$rev") || git-fetch)) ||
 				die "$(eval_gettext "Unable to fetch in submodule path '\$displaypath'")"
 			fi
-- 
2.7.0.rc0.34.ga06e0b3.dirty

Re: [PATCH 1/2] submodule: Include check for objects when fetching

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Junio wrote:
>> To be complete, the rev-list command line should also run with
>> "--objects"; after all, a commit walker fetch may have downloaded
>> commit chain completely but haven't fetched necessary trees and
>> blobs when it was killed, and "rev-list $sha1 --not --all" would not
>> catch such a breakage without "--objects".
>
> By adding the --objects switch to rev-list we make sure to do a complete
> check.

You also need to drop "-n1" for the command to be equivalent to
quickfetch, I think.

>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  git-submodule.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 9bc5c5f..f5d6675 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -746,7 +746,7 @@ Maybe you want to use 'update --init'?")"
>  				# 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) &&
> +					( (rev=$(git rev-list --objects -n 1 $sha1 --not --all 2>/dev/null) &&
>  					 test -z "$rev") || git-fetch)) ||
>  				die "$(eval_gettext "Unable to fetch in submodule path '\$displaypath'")"
>  			fi

Re: [PATCH 1/2] submodule: Include check for objects when fetching

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Stefan Beller <sbeller@google.com> writes:
>
>> Junio wrote:
>>> To be complete, the rev-list command line should also run with
>>> "--objects"; after all, a commit walker fetch may have downloaded
>>> commit chain completely but haven't fetched necessary trees and
>>> blobs when it was killed, and "rev-list $sha1 --not --all" would not
>>> catch such a breakage without "--objects".
>>
>> By adding the --objects switch to rev-list we make sure to do a complete
>> check.
>
> You also need to drop "-n1" for the command to be equivalent to
> quickfetch, I think.

Ahh, I think I need to take it back.

As the existing code relies not just on the exit status of rev-list
but also checks if anything comes out of it, having "-n1" would not
hurt.

The case I was worried about was that we have all commits to connect
$sha1 to what are reachable from refs, but some tree/blob objects
referenced by these commits were missing.  By cutting the displaying
with "-n1", rev-list may happily exit successfully after showing $sha1,
without even realizing that some trees/blobs are missing and the history
leading to $sha1 cannot be considered complete.

But because the code checks the empty-ness of $rev, this case is diagnosed
as "$sha1 is not reachable from any of the refs" correctly anyway,
so it should be OK.

>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  git-submodule.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index 9bc5c5f..f5d6675 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -746,7 +746,7 @@ Maybe you want to use 'update --init'?")"
>>  				# 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) &&
>> +					( (rev=$(git rev-list --objects -n 1 $sha1 --not --all 2>/dev/null) &&
>>  					 test -z "$rev") || git-fetch)) ||
>>  				die "$(eval_gettext "Unable to fetch in submodule path '\$displaypath'")"
>>  			fi

Re: [PATCH 1/2] submodule: Include check for objects when fetching

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> Junio wrote:
>>>> To be complete, the rev-list command line should also run with
>>>> "--objects"; after all, a commit walker fetch may have downloaded
>>>> commit chain completely but haven't fetched necessary trees and
>>>> blobs when it was killed, and "rev-list $sha1 --not --all" would not
>>>> catch such a breakage without "--objects".
>>>
>>> By adding the --objects switch to rev-list we make sure to do a complete
>>> check.
>>
>> You also need to drop "-n1" for the command to be equivalent to
>> quickfetch, I think.
>
> Ahh, I think I need to take it back.
>
> As the existing code relies not just on the exit status of rev-list
> but also checks if anything comes out of it, having "-n1" would not
> hurt.
>
> The case I was worried about was that we have all commits to connect
> $sha1 to what are reachable from refs, but some tree/blob objects
> referenced by these commits were missing.  By cutting the displaying
> with "-n1", rev-list may happily exit successfully after showing $sha1,
> without even realizing that some trees/blobs are missing and the history
> leading to $sha1 cannot be considered complete.
>
> But because the code checks the empty-ness of $rev, this case is diagnosed
> as "$sha1 is not reachable from any of the refs" correctly anyway,
> so it should be OK.

... this is embarrassing, but for exactly the same reason, I do not
think we need "--objects" either.  If $sha1 is not something that is
already reachable from any of the refs, "rev-list -n1" would show
something, making $rev non-empty, and it would result in a fetch,
whether the objects that are reachable from the commits between
existing refs and $sha1 are complete or not.  So it is pointless to
use "--objects" to test their presense.

Sorry about the noise ;-)

>>> Signed-off-by: Stefan Beller <sbeller@google.com>
>>> ---
>>>  git-submodule.sh | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/git-submodule.sh b/git-submodule.sh
>>> index 9bc5c5f..f5d6675 100755
>>> --- a/git-submodule.sh
>>> +++ b/git-submodule.sh
>>> @@ -746,7 +746,7 @@ Maybe you want to use 'update --init'?")"
>>>  				# 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) &&
>>> +					( (rev=$(git rev-list --objects -n 1 $sha1 --not --all 2>/dev/null) &&
>>>  					 test -z "$rev") || git-fetch)) ||
>>>  				die "$(eval_gettext "Unable to fetch in submodule path '\$displaypath'")"
>>>  			fi

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

[Stefan Beller]

When reviewing a change in Gerrit, which also updates a submodule,
a common review practice is to download and cherry-pick the patch locally
to test it. However when testing it locally, the 'git submodule update'
may fail fetching the correct submodule sha1 as the corresponding commit
in the submodule is not yet part of the project history, but also just a
proposed change.

If $sha1 was not part of the default fetch, we try to fetch the $sha1
directly. Some servers however do not support direct fetch by sha1, which
leads git-fetch to fail quickly. We can fail ourselves here as the still
missing sha1 would lead to a failure later in the checkout stage anyway,
so failing here is as good as we can get.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

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?

Thanks,
Stefan

 git-submodule.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/git-submodule.sh b/git-submodule.sh
index f5d6675..f021fe3 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -749,6 +749,13 @@ Maybe you want to use 'update --init'?")"
 					( (rev=$(git rev-list --objects -n 1 $sha1 --not --all 2>/dev/null) &&
 					 test -z "$rev") || git-fetch)) ||
 				die "$(eval_gettext "Unable to fetch in submodule path '\$displaypath'")"
+
+				# In case the sha1 is still missing, try harder
+				# by fetching the sha1 directly.
+				(clear_local_git_env; cd "$sm_path" &&
+					( (rev=$(git rev-list --objects -n 1 $sha1 --not --all 2>/dev/null) &&
+					 test -z "$rev") || git-fetch $(get_default_remote) $sha1 )) ||
+				die "$(eval_gettext "Unable to fetch in submodule path '\$displaypath'")"
 			fi
 
 			# Is this something we just cloned?
-- 
2.7.0.rc0.34.ga06e0b3.dirty

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

[Junio C Hamano]

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?

 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"
+)
+
+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" ||
 				die "$(eval_gettext "Unable to fetch in submodule path '\$displaypath'")"
 			fi
 

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

[Stefan Beller]

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!

>
>  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
>

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

[Junio C Hamano]

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
>>

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

[Stefan Beller]

When reviewing a change in Gerrit, which also updates a submodule,
a common review practice is to download and cherry-pick the patch
locally to test it. However when testing it locally, the 'git
submodule update' may fail fetching the correct submodule sha1 as
the corresponding commit in the submodule is not yet part of the
project history, but also just a proposed change.

If $sha1 was not part of the default fetch, we try to fetch the $sha1
directly. Some servers however do not support direct fetch by sha1,
which leads git-fetch to fail quickly. We can fail ourselves here as
the still missing sha1 would lead to a failure later in the checkout
stage anyway, so failing here is as good as we can get.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---

 Junio, I took your patch and reworded the error message, so 25 / 26
 lines of the code are still originally authored by you. But as "it is
 my itch not yours" I am resending it. If you changed your mind, take
 authorship of this.
 
 Thanks,
 Stefan

 git-submodule.sh | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 9bc5c5f..babfc64 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"
+)
+
+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,10 +763,15 @@ 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" ||
+				die "$(eval_gettext "Fetched in submodule path '\$displaypath', but it did not contain $sha1. Direct fetching of that commit failed.")"
 			fi
 
 			# Is this something we just cloned?
-- 
2.7.0.rc0.34.ga06e0b3.dirty