{"thread":{"id":"41476","subject":"[PATCH 2/2] submodule: Try harder to fetch needed sha1 by direct fetching sha1","startedAt":"2016-02-22T22:35:46Z","lastAt":"2016-02-24T03:32:13Z","messageCount":10,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"278918","messageId":"1456180548-20996-1-git-send-email-sbeller@google.com","threadId":"41476","inReplyTo":null,"subject":"[PATCH 0/2] Submodule harden fetches (WAS: submodule: Fetch the direct sha1 first)","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-22T22:35:46Z","receivedAt":"2016-02-22T22:35:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"A simple patch evolves into a series!\n\nFirst we'll fix a bug by adding the --objects switch to rev-list to\nhave a stricter check for the desired commit to be there. IIUC this\nis not 100% right yet, but it improves the situation.\n\nThe second patch introduces an extra fetch in case the first fetch\ndid not yield the expected commit, using the pattern\n\n    # first 4 lines unchanged:\n    if $sha1's history and objects are incomplete:\n        fetch ;# normally just like we have done before\n    else \n        die ...\n    # new code:     \n    if $sha1's history and objects are still incomplete:\n        fetch $sha1\n    else\n        die ... \n        \nThanks,\nStefan\n\nStefan Beller (2):\n  submodule: Include check for objects when fetching\n  submodule: Try harder to fetch needed sha1 by direct fetching sha1\n\n git-submodule.sh | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\n-- \n2.7.0.rc0.34.ga06e0b3.dirty\n"},{"id":"278919","messageId":"1456180548-20996-2-git-send-email-sbeller@google.com","threadId":"41476","inReplyTo":"1456180548-20996-1-git-send-email-sbeller@google.com","subject":"[PATCH 1/2] submodule: Include check for objects when fetching","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-22T22:35:47Z","receivedAt":"2016-02-22T22:35:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Junio wrote:\n> To be complete, the rev-list command line should also run with\n> \"--objects\"; after all, a commit walker fetch may have downloaded\n> commit chain completely but haven't fetched necessary trees and\n> blobs when it was killed, and \"rev-list $sha1 --not --all\" would not\n> catch such a breakage without \"--objects\".\n\nBy adding the --objects switch to rev-list we make sure to do a complete\ncheck.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n git-submodule.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 9bc5c5f..f5d6675 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -746,7 +746,7 @@ Maybe you want to use 'update --init'?\")\"\n \t\t\t\t# Run fetch only if $sha1 isn't present or it\n \t\t\t\t# is not reachable from a ref.\n \t\t\t\t(clear_local_git_env; cd \"$sm_path\" &&\n-\t\t\t\t\t( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&\n+\t\t\t\t\t( (rev=$(git rev-list --objects -n 1 $sha1 --not --all 2>/dev/null) &&\n \t\t\t\t\t test -z \"$rev\") || git-fetch)) ||\n \t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n \t\t\tfi\n-- \n2.7.0.rc0.34.ga06e0b3.dirty\n"},{"id":"278917","messageId":"1456180548-20996-3-git-send-email-sbeller@google.com","threadId":"41476","inReplyTo":"1456180548-20996-1-git-send-email-sbeller@google.com","subject":"[PATCH 2/2] submodule: Try harder to fetch needed sha1 by direct fetching sha1","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-22T22:35:48Z","receivedAt":"2016-02-22T22:35:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When reviewing a change in Gerrit, which also updates a submodule,\na common review practice is to download and cherry-pick the patch locally\nto test it. However when testing it locally, the 'git submodule update'\nmay fail fetching the correct submodule sha1 as the corresponding commit\nin the submodule is not yet part of the project history, but also just a\nproposed change.\n\nIf $sha1 was not part of the default fetch, we try to fetch the $sha1\ndirectly. Some servers however do not support direct fetch by sha1, which\nleads git-fetch to fail quickly. We can fail ourselves here as the still\nmissing sha1 would lead to a failure later in the checkout stage anyway,\nso failing here is as good as we can get.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nWe may want to discuss the error message. It is the same as in the case\nbefore (git-fetch <no args>), this is good for translation, but may be bad\nfor the user as we may want to give extra information.\n(\"We fetched for you and it worked, however the sha1 was not included,\nso we fetched again the server broke it, so we error out.  You used to\nsee error message: ....\")\n\nAlthough this may be too much information?\n\nThanks,\nStefan\n\n git-submodule.sh | 7 +++++++\n 1 file changed, 7 insertions(+)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex f5d6675..f021fe3 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -749,6 +749,13 @@ Maybe you want to use 'update --init'?\")\"\n \t\t\t\t\t( (rev=$(git rev-list --objects -n 1 $sha1 --not --all 2>/dev/null) &&\n \t\t\t\t\t test -z \"$rev\") || git-fetch)) ||\n \t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n+\n+\t\t\t\t# In case the sha1 is still missing, try harder\n+\t\t\t\t# by fetching the sha1 directly.\n+\t\t\t\t(clear_local_git_env; cd \"$sm_path\" &&\n+\t\t\t\t\t( (rev=$(git rev-list --objects -n 1 $sha1 --not --all 2>/dev/null) &&\n+\t\t\t\t\t test -z \"$rev\") || git-fetch $(get_default_remote) $sha1 )) ||\n+\t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n \t\t\tfi\n \n \t\t\t# Is this something we just cloned?\n-- \n2.7.0.rc0.34.ga06e0b3.dirty\n"},{"id":"278945","messageId":"xmqqoab84apu.fsf@gitster.mtv.corp.google.com","threadId":"41476","inReplyTo":"1456180548-20996-2-git-send-email-sbeller@google.com","subject":"Re: [PATCH 1/2] submodule: Include check for objects when fetching","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-22T22:54:53Z","receivedAt":"2016-02-22T22:54:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Junio wrote:\n>> To be complete, the rev-list command line should also run with\n>> \"--objects\"; after all, a commit walker fetch may have downloaded\n>> commit chain completely but haven't fetched necessary trees and\n>> blobs when it was killed, and \"rev-list $sha1 --not --all\" would not\n>> catch such a breakage without \"--objects\".\n>\n> By adding the --objects switch to rev-list we make sure to do a complete\n> check.\n\nYou also need to drop \"-n1\" for the command to be equivalent to\nquickfetch, I think.\n\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  git-submodule.sh | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 9bc5c5f..f5d6675 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -746,7 +746,7 @@ Maybe you want to use 'update --init'?\")\"\n>  \t\t\t\t# Run fetch only if $sha1 isn't present or it\n>  \t\t\t\t# is not reachable from a ref.\n>  \t\t\t\t(clear_local_git_env; cd \"$sm_path\" &&\n> -\t\t\t\t\t( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&\n> +\t\t\t\t\t( (rev=$(git rev-list --objects -n 1 $sha1 --not --all 2>/dev/null) &&\n>  \t\t\t\t\t test -z \"$rev\") || git-fetch)) ||\n>  \t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n>  \t\t\tfi\n"},{"id":"278948","messageId":"xmqqk2lw4a6p.fsf@gitster.mtv.corp.google.com","threadId":"41476","inReplyTo":"xmqqoab84apu.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] submodule: Include check for objects when fetching","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-22T23:06:22Z","receivedAt":"2016-02-22T23:06:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> Junio wrote:\n>>> To be complete, the rev-list command line should also run with\n>>> \"--objects\"; after all, a commit walker fetch may have downloaded\n>>> commit chain completely but haven't fetched necessary trees and\n>>> blobs when it was killed, and \"rev-list $sha1 --not --all\" would not\n>>> catch such a breakage without \"--objects\".\n>>\n>> By adding the --objects switch to rev-list we make sure to do a complete\n>> check.\n>\n> You also need to drop \"-n1\" for the command to be equivalent to\n> quickfetch, I think.\n\nAhh, I think I need to take it back.\n\nAs the existing code relies not just on the exit status of rev-list\nbut also checks if anything comes out of it, having \"-n1\" would not\nhurt.\n\nThe case I was worried about was that we have all commits to connect\n$sha1 to what are reachable from refs, but some tree/blob objects\nreferenced by these commits were missing.  By cutting the displaying\nwith \"-n1\", rev-list may happily exit successfully after showing $sha1,\nwithout even realizing that some trees/blobs are missing and the history\nleading to $sha1 cannot be considered complete.\n\nBut because the code checks the empty-ness of $rev, this case is diagnosed\nas \"$sha1 is not reachable from any of the refs\" correctly anyway,\nso it should be OK.\n\n>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>> ---\n>>  git-submodule.sh | 2 +-\n>>  1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/git-submodule.sh b/git-submodule.sh\n>> index 9bc5c5f..f5d6675 100755\n>> --- a/git-submodule.sh\n>> +++ b/git-submodule.sh\n>> @@ -746,7 +746,7 @@ Maybe you want to use 'update --init'?\")\"\n>>  \t\t\t\t# Run fetch only if $sha1 isn't present or it\n>>  \t\t\t\t# is not reachable from a ref.\n>>  \t\t\t\t(clear_local_git_env; cd \"$sm_path\" &&\n>> -\t\t\t\t\t( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&\n>> +\t\t\t\t\t( (rev=$(git rev-list --objects -n 1 $sha1 --not --all 2>/dev/null) &&\n>>  \t\t\t\t\t test -z \"$rev\") || git-fetch)) ||\n>>  \t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n>>  \t\t\tfi\n"},{"id":"278951","messageId":"xmqq7fhw49ly.fsf@gitster.mtv.corp.google.com","threadId":"41476","inReplyTo":"xmqqk2lw4a6p.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] submodule: Include check for objects when fetching","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-22T23:18:49Z","receivedAt":"2016-02-22T23:18:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>>> Junio wrote:\n>>>> To be complete, the rev-list command line should also run with\n>>>> \"--objects\"; after all, a commit walker fetch may have downloaded\n>>>> commit chain completely but haven't fetched necessary trees and\n>>>> blobs when it was killed, and \"rev-list $sha1 --not --all\" would not\n>>>> catch such a breakage without \"--objects\".\n>>>\n>>> By adding the --objects switch to rev-list we make sure to do a complete\n>>> check.\n>>\n>> You also need to drop \"-n1\" for the command to be equivalent to\n>> quickfetch, I think.\n>\n> Ahh, I think I need to take it back.\n>\n> As the existing code relies not just on the exit status of rev-list\n> but also checks if anything comes out of it, having \"-n1\" would not\n> hurt.\n>\n> The case I was worried about was that we have all commits to connect\n> $sha1 to what are reachable from refs, but some tree/blob objects\n> referenced by these commits were missing.  By cutting the displaying\n> with \"-n1\", rev-list may happily exit successfully after showing $sha1,\n> without even realizing that some trees/blobs are missing and the history\n> leading to $sha1 cannot be considered complete.\n>\n> But because the code checks the empty-ness of $rev, this case is diagnosed\n> as \"$sha1 is not reachable from any of the refs\" correctly anyway,\n> so it should be OK.\n\n... this is embarrassing, but for exactly the same reason, I do not\nthink we need \"--objects\" either.  If $sha1 is not something that is\nalready reachable from any of the refs, \"rev-list -n1\" would show\nsomething, making $rev non-empty, and it would result in a fetch,\nwhether the objects that are reachable from the commits between\nexisting refs and $sha1 are complete or not.  So it is pointless to\nuse \"--objects\" to test their presense.\n\nSorry about the noise ;-)\n\n>>> Signed-off-by: Stefan Beller <sbeller@google.com>\n>>> ---\n>>>  git-submodule.sh | 2 +-\n>>>  1 file changed, 1 insertion(+), 1 deletion(-)\n>>>\n>>> diff --git a/git-submodule.sh b/git-submodule.sh\n>>> index 9bc5c5f..f5d6675 100755\n>>> --- a/git-submodule.sh\n>>> +++ b/git-submodule.sh\n>>> @@ -746,7 +746,7 @@ Maybe you want to use 'update --init'?\")\"\n>>>  \t\t\t\t# Run fetch only if $sha1 isn't present or it\n>>>  \t\t\t\t# is not reachable from a ref.\n>>>  \t\t\t\t(clear_local_git_env; cd \"$sm_path\" &&\n>>> -\t\t\t\t\t( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&\n>>> +\t\t\t\t\t( (rev=$(git rev-list --objects -n 1 $sha1 --not --all 2>/dev/null) &&\n>>>  \t\t\t\t\t test -z \"$rev\") || git-fetch)) ||\n>>>  \t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n>>>  \t\t\tfi\n"},{"id":"278953","messageId":"xmqqzius2t31.fsf@gitster.mtv.corp.google.com","threadId":"41476","inReplyTo":"1456180548-20996-3-git-send-email-sbeller@google.com","subject":"Re: [PATCH 2/2] submodule: Try harder to fetch needed sha1 by direct fetching sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-23T00:01:06Z","receivedAt":"2016-02-23T00:01:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> We may want to discuss the error message. It is the same as in the case\n> before (git-fetch <no args>), this is good for translation, but may be bad\n> for the user as we may want to give extra information.\n> (\"We fetched for you and it worked, however the sha1 was not included,\n> so we fetched again the server broke it, so we error out.  You used to\n> see error message: ....\")\n>\n> Although this may be too much information?\n\nNow the \"go to that submodule directory and see if $sha1 is\nreachable from any of the ref\" check is written twice exactly the\nsame way, it deserves to become a small helper function, I think.\n\nPerhaps something along this line?\n\n git-submodule.sh | 29 ++++++++++++++++++++++++++---\n 1 file changed, 26 insertions(+), 3 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 9bc5c5f..836348f 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -591,6 +591,24 @@ cmd_deinit()\n \tdone\n }\n \n+is_tip_reachable () (\n+\tclear_local_git_env\n+\tcd \"$1\" &&\n+\trev=$(git rev-list -n 1 \"$2\" --not --all 2>/dev/null) &&\n+\ttest -z \"$rev\"\n+)\n+\n+fetch_in_submodule () (\n+\tclear_local_git_env\n+\tcd \"$1\" &&\n+\tcase \"$2\" in\n+\t'')\n+\t\tgit fetch ;;\n+\t*)\n+\t\tgit fetch $(get_default_remote) \"$2\" ;;\n+\tesac\n+)\n+\n #\n # Update each submodule path to correct revision, using clone and checkout as needed\n #\n@@ -745,9 +763,14 @@ Maybe you want to use 'update --init'?\")\"\n \t\t\tthen\n \t\t\t\t# Run fetch only if $sha1 isn't present or it\n \t\t\t\t# is not reachable from a ref.\n-\t\t\t\t(clear_local_git_env; cd \"$sm_path\" &&\n-\t\t\t\t\t( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&\n-\t\t\t\t\t test -z \"$rev\") || git-fetch)) ||\n+\t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n+\t\t\t\tfetch_in_submodule \"$sm_path\" ||\n+\t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n+\n+\t\t\t\t# Now we tried the usual fetch, but $sha1 may\n+\t\t\t\t# not be reachable from any of the refs\n+\t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n+\t\t\t\tfetch_in_submodule \"$sm_path\" \"$sha1\" ||\n \t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n \t\t\tfi\n \n"},{"id":"278954","messageId":"CAGZ79ka-iAHvopwp==fTiuL13sJ_KzqotCNoOgQLJzvpNGH2HA@mail.gmail.com","threadId":"41476","inReplyTo":"xmqqzius2t31.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] submodule: Try harder to fetch needed sha1 by direct fetching sha1","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-23T00:28:03Z","receivedAt":"2016-02-23T00:28:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Feb 22, 2016 at 4:01 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Stefan Beller <sbeller@google.com> writes:\n>\n> > We may want to discuss the error message. It is the same as in the case\n> > before (git-fetch <no args>), this is good for translation, but may be bad\n> > for the user as we may want to give extra information.\n> > (\"We fetched for you and it worked, however the sha1 was not included,\n> > so we fetched again the server broke it, so we error out.  You used to\n> > see error message: ....\")\n> >\n> > Although this may be too much information?\n>\n> Now the \"go to that submodule directory and see if $sha1 is\n> reachable from any of the ref\" check is written twice exactly the\n> same way, it deserves to become a small helper function, I think.\n>\n> Perhaps something along this line?\n\nThis looks very readable.\n\nFeel free to drop both my patches and just introduce this patch as yours!\n\n>\n>  git-submodule.sh | 29 ++++++++++++++++++++++++++---\n>  1 file changed, 26 insertions(+), 3 deletions(-)\n>\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 9bc5c5f..836348f 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -591,6 +591,24 @@ cmd_deinit()\n>         done\n>  }\n>\n> +is_tip_reachable () (\n> +       clear_local_git_env\n> +       cd \"$1\" &&\n> +       rev=$(git rev-list -n 1 \"$2\" --not --all 2>/dev/null) &&\n> +       test -z \"$rev\"\n\nWhile we're talking about this code anyway, I wondered if we can make it\nmore cryptic again (just kidding!) and shorten it a bit by using the\n\n    is_tip_reachable () (\n        clear_local_git_env\n        test -z $(git -C $1 rev-list -n 1 \"$2\" --not --all 2>/dev/null)\n    )\n\n> +)\n> +\n> +fetch_in_submodule () (\n> +       clear_local_git_env\n> +       cd \"$1\" &&\n> +       case \"$2\" in\n> +       '')\n> +               git fetch ;;\n> +       *)\n> +               git fetch $(get_default_remote) \"$2\" ;;\n> +       esac\n> +)\n> +\n>  #\n>  # Update each submodule path to correct revision, using clone and checkout as needed\n>  #\n> @@ -745,9 +763,14 @@ Maybe you want to use 'update --init'?\")\"\n>                         then\n>                                 # Run fetch only if $sha1 isn't present or it\n>                                 # is not reachable from a ref.\n> -                               (clear_local_git_env; cd \"$sm_path\" &&\n> -                                       ( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&\n> -                                        test -z \"$rev\") || git-fetch)) ||\n> +                               is_tip_reachable \"$sm_path\" \"$sha1\" ||\n> +                               fetch_in_submodule \"$sm_path\" ||\n> +                               die \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n> +\n> +                               # Now we tried the usual fetch, but $sha1 may\n> +                               # not be reachable from any of the refs\n> +                               is_tip_reachable \"$sm_path\" \"$sha1\" ||\n> +                               fetch_in_submodule \"$sm_path\" \"$sha1\" ||\n\nFor another split second I wondered about the return code of is_tip_reachable,\nif the result is actually negated, but reading the chaining here, makes sense.\n\nThanks,\nStefan\n\n>                                 die \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n>                         fi\n>\n"},{"id":"278976","messageId":"xmqqmvqs2ao4.fsf@gitster.mtv.corp.google.com","threadId":"41476","inReplyTo":"CAGZ79ka-iAHvopwp==fTiuL13sJ_KzqotCNoOgQLJzvpNGH2HA@mail.gmail.com","subject":"Re: [PATCH 2/2] submodule: Try harder to fetch needed sha1 by direct fetching sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-23T06:38:51Z","receivedAt":"2016-02-23T06:38:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Mon, Feb 22, 2016 at 4:01 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>> > We may want to discuss the error message. It is the same as in the case\n>> > before (git-fetch <no args>), this is good for translation, but may be bad\n>> > for the user as we may want to give extra information.\n>> > (\"We fetched for you and it worked, however the sha1 was not included,\n>> > so we fetched again the server broke it, so we error out.  You used to\n>> > see error message: ....\")\n>> >\n>> > Although this may be too much information?\n>>\n>> Now the \"go to that submodule directory and see if $sha1 is\n>> reachable from any of the ref\" check is written twice exactly the\n>> same way, it deserves to become a small helper function, I think.\n>>\n>> Perhaps something along this line?\n>\n> This looks very readable.\n>\n> Feel free to drop both my patches and just introduce this patch as yours!\n\nNah, this is not my itch but yours ;-)\n\nBesides, have we agreed what error message we want to give to the\nusers yet?\n\n>>\n>>  git-submodule.sh | 29 ++++++++++++++++++++++++++---\n>>  1 file changed, 26 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/git-submodule.sh b/git-submodule.sh\n>> index 9bc5c5f..836348f 100755\n>> --- a/git-submodule.sh\n>> +++ b/git-submodule.sh\n>> @@ -591,6 +591,24 @@ cmd_deinit()\n>>         done\n>>  }\n>>\n>> +is_tip_reachable () (\n>> +       clear_local_git_env\n>> +       cd \"$1\" &&\n>> +       rev=$(git rev-list -n 1 \"$2\" --not --all 2>/dev/null) &&\n>> +       test -z \"$rev\"\n>\n> While we're talking about this code anyway, I wondered if we can make it\n> more cryptic again (just kidding!) and shorten it a bit by using the\n>\n>     is_tip_reachable () (\n>         clear_local_git_env\n>         test -z $(git -C $1 rev-list -n 1 \"$2\" --not --all 2>/dev/null)\n>     )\n>\n>> +)\n>> +\n>> +fetch_in_submodule () (\n>> +       clear_local_git_env\n>> +       cd \"$1\" &&\n>> +       case \"$2\" in\n>> +       '')\n>> +               git fetch ;;\n>> +       *)\n>> +               git fetch $(get_default_remote) \"$2\" ;;\n>> +       esac\n>> +)\n>> +\n>>  #\n>>  # Update each submodule path to correct revision, using clone and checkout as needed\n>>  #\n>> @@ -745,9 +763,14 @@ Maybe you want to use 'update --init'?\")\"\n>>                         then\n>>                                 # Run fetch only if $sha1 isn't present or it\n>>                                 # is not reachable from a ref.\n>> -                               (clear_local_git_env; cd \"$sm_path\" &&\n>> -                                       ( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&\n>> -                                        test -z \"$rev\") || git-fetch)) ||\n>> +                               is_tip_reachable \"$sm_path\" \"$sha1\" ||\n>> +                               fetch_in_submodule \"$sm_path\" ||\n>> +                               die \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n>> +\n>> +                               # Now we tried the usual fetch, but $sha1 may\n>> +                               # not be reachable from any of the refs\n>> +                               is_tip_reachable \"$sm_path\" \"$sha1\" ||\n>> +                               fetch_in_submodule \"$sm_path\" \"$sha1\" ||\n>\n> For another split second I wondered about the return code of is_tip_reachable,\n> if the result is actually negated, but reading the chaining here, makes sense.\n>\n> Thanks,\n> Stefan\n>\n>>                                 die \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n>>                         fi\n>>\n"},{"id":"279119","messageId":"1456284733-28003-1-git-send-email-sbeller@google.com","threadId":"41476","inReplyTo":"xmqqmvqs2ao4.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] submodule: Try harder to fetch needed sha1 by direct fetching sha1","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-24T03:32:13Z","receivedAt":"2016-02-24T03:32:13Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When reviewing a change in Gerrit, which also updates a submodule,\na common review practice is to download and cherry-pick the patch\nlocally to test it. However when testing it locally, the 'git\nsubmodule update' may fail fetching the correct submodule sha1 as\nthe corresponding commit in the submodule is not yet part of the\nproject history, but also just a proposed change.\n\nIf $sha1 was not part of the default fetch, we try to fetch the $sha1\ndirectly. Some servers however do not support direct fetch by sha1,\nwhich leads git-fetch to fail quickly. We can fail ourselves here as\nthe still missing sha1 would lead to a failure later in the checkout\nstage anyway, so failing here is as good as we can get.\n\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n Junio, I took your patch and reworded the error message, so 25 / 26\n lines of the code are still originally authored by you. But as \"it is\n my itch not yours\" I am resending it. If you changed your mind, take\n authorship of this.\n \n Thanks,\n Stefan\n\n git-submodule.sh | 29 ++++++++++++++++++++++++++---\n 1 file changed, 26 insertions(+), 3 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 9bc5c5f..babfc64 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -591,6 +591,24 @@ cmd_deinit()\n \tdone\n }\n \n+is_tip_reachable () (\n+\tclear_local_git_env\n+\tcd \"$1\" &&\n+\trev=$(git rev-list -n 1 \"$2\" --not --all 2>/dev/null) &&\n+\ttest -z \"$rev\"\n+)\n+\n+fetch_in_submodule () (\n+\tclear_local_git_env\n+\tcd \"$1\" &&\n+\tcase \"$2\" in\n+\t'')\n+\t\tgit fetch ;;\n+\t*)\n+\t\tgit fetch $(get_default_remote) \"$2\" ;;\n+\tesac\n+)\n+\n #\n # Update each submodule path to correct revision, using clone and checkout as needed\n #\n@@ -745,10 +763,15 @@ Maybe you want to use 'update --init'?\")\"\n \t\t\tthen\n \t\t\t\t# Run fetch only if $sha1 isn't present or it\n \t\t\t\t# is not reachable from a ref.\n-\t\t\t\t(clear_local_git_env; cd \"$sm_path\" &&\n-\t\t\t\t\t( (rev=$(git rev-list -n 1 $sha1 --not --all 2>/dev/null) &&\n-\t\t\t\t\t test -z \"$rev\") || git-fetch)) ||\n+\t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n+\t\t\t\tfetch_in_submodule \"$sm_path\" ||\n \t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n+\n+\t\t\t\t# Now we tried the usual fetch, but $sha1 may\n+\t\t\t\t# not be reachable from any of the refs\n+\t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n+\t\t\t\tfetch_in_submodule \"$sm_path\" \"$sha1\" ||\n+\t\t\t\tdie \"$(eval_gettext \"Fetched in submodule path '\\$displaypath', but it did not contain $sha1. Direct fetching of that commit failed.\")\"\n \t\t\tfi\n \n \t\t\t# Is this something we just cloned?\n-- \n2.7.0.rc0.34.ga06e0b3.dirty\n"}]}