{"thread":{"id":"48468","subject":"[PATCH] git-submodule.sh: try harder to fetch a submodule","startedAt":"2018-05-11T23:18:00Z","lastAt":"2018-05-15T20:05:04Z","messageCount":7,"participants":["Stefan Beller","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"347400","messageId":"20180511231751.162855-1-sbeller@google.com","threadId":"48468","inReplyTo":null,"subject":"[PATCH] git-submodule.sh: try harder to fetch a submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-11T23:17:51Z","receivedAt":"2018-05-11T23:18:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This is the logical continuum of fb43e31f2b4 (submodule: try harder to\nfetch needed sha1 by direct fetching sha1, 2016-02-23) and fixes it as\nsome assumptions were not correct.\n\n> If $sha1 was not part of the default fetch ... fail ourselves here\nassumes that the fetch_in_submodule only fails when the serverside does\nnot support fetching by sha1.\n\nThere are other failures, why such a fetch may fail, such as\n    fatal: Couldn't find remote ref HEAD\nwhich can happen if the remote side doesn't advertise HEAD. Not advertising\nHEAD is allowed by the protocol spec and would happen, if HEAD points at a\nref, that this user cannot see (due to ACLs for example).\n\nSo do try even harder for a submodule by ignoring the exit code of the\nfirst fetch and rather relying on the following is_tip_reachable to\nsee if we try fetching again.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n git-submodule.sh | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 24914963ca2..13b378a6c8f 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -614,7 +614,6 @@ cmd_update()\n \t\t\t\t# is not reachable from a ref.\n \t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n \t\t\t\tfetch_in_submodule \"$sm_path\" $depth ||\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-- \n2.17.0.582.gccdcbd54c44.dirty\n\n"},{"id":"347401","messageId":"20180511232840.GB41222@aiede.svl.corp.google.com","threadId":"48468","inReplyTo":"20180511231751.162855-1-sbeller@google.com","subject":"Re: [PATCH] git-submodule.sh: try harder to fetch a submodule","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-05-11T23:28:40Z","receivedAt":"2018-05-11T23:28:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> This is the logical continuum of fb43e31f2b4 (submodule: try harder to\n> fetch needed sha1 by direct fetching sha1, 2016-02-23) and fixes it as\n> some assumptions were not correct.\n\nInteresting.\n\nI think what would help most is an example set of commands I can use\nto reproduce this (bonus points if in the form of a test).\n\n> > If $sha1 was not part of the default fetch ... fail ourselves here\n> assumes that the fetch_in_submodule only fails when the serverside does\n\nI'm having some trouble with the formatting here.  Is the part\npreceded by '>' a quote, and if so a quote from what?\n\n> not support fetching by sha1.\n>\n> There are other failures, why such a fetch may fail, such as\n>     fatal: Couldn't find remote ref HEAD\n> which can happen if the remote side doesn't advertise HEAD. Not advertising\n\nnit: it can be useful to have a blank line before and after such\nexample output to help both my eyes and tools like \"git log\n--format='%w(100)%b'\" to understand the formatting.\n\n> HEAD is allowed by the protocol spec and would happen, if HEAD points at a\n> ref, that this user cannot see (due to ACLs for example).\n\nA more typical example would be if the ref simply doesn't exist (i.e.,\nis a branch yet to be born).\n\n> So do try even harder for a submodule by ignoring the exit code of the\n> first fetch and rather relying on the following is_tip_reachable to\n> see if we try fetching again.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  git-submodule.sh | 1 -\n>  1 file changed, 1 deletion(-)\n> \n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 24914963ca2..13b378a6c8f 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -614,7 +614,6 @@ cmd_update()\n>  \t\t\t\t# is not reachable from a ref.\n>  \t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n>  \t\t\t\tfetch_in_submodule \"$sm_path\" $depth ||\n\nIs keeping the '||' at the end of this line intended?\n\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\" $depth \"$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\nShould this error message be changed?\n\nThanks,\nJonathan\n"},{"id":"347402","messageId":"CAGZ79kYcOgBM_XQRidsB-T2V+vfuiG5rrNKTvhHYwzPs3mF4qA@mail.gmail.com","threadId":"48468","inReplyTo":"20180511232840.GB41222@aiede.svl.corp.google.com","subject":"Re: [PATCH] git-submodule.sh: try harder to fetch a submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-11T23:42:29Z","receivedAt":"2018-05-11T23:42:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, May 11, 2018 at 4:28 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Hi,\n>\n> Stefan Beller wrote:\n>\n>> This is the logical continuum of fb43e31f2b4 (submodule: try harder to\n>> fetch needed sha1 by direct fetching sha1, 2016-02-23) and fixes it as\n>> some assumptions were not correct.\n>\n> Interesting.\n>\n> I think what would help most is an example set of commands I can use\n> to reproduce this (bonus points if in the form of a test).\n\nI tried coming up with a test in git-core that produces a remote similar to\nGerrit, and let me tell you, it's not Git that is weird here. ;)\n\n>> > If $sha1 was not part of the default fetch ... fail ourselves here\n>> assumes that the fetch_in_submodule only fails when the serverside does\n>\n> I'm having some trouble with the formatting here.  Is the part\n> preceded by '>' a quote, and if so a quote from what?\n\nThe quote is from fb43e31f2b4.\n\n>> There are other failures, why such a fetch may fail, such as\n>>     fatal: Couldn't find remote ref HEAD\n>> which can happen if the remote side doesn't advertise HEAD. Not advertising\n>\n> nit: it can be useful to have a blank line before and after such\n> example output to help both my eyes and tools like \"git log\n> --format='%w(100)%b'\" to understand the formatting.\n\nWhy would you use this formatting?\n\n>\n>> HEAD is allowed by the protocol spec and would happen, if HEAD points at a\n>> ref, that this user cannot see (due to ACLs for example).\n>\n> A more typical example would be if the ref simply doesn't exist (i.e.,\n> is a branch yet to be born).\n\nOh, I checked that but not for the submodule case, let me check that.\n\n>> diff --git a/git-submodule.sh b/git-submodule.sh\n>> index 24914963ca2..13b378a6c8f 100755\n>> --- a/git-submodule.sh\n>> +++ b/git-submodule.sh\n>> @@ -614,7 +614,6 @@ cmd_update()\n>>                               # is not reachable from a ref.\n>>                               is_tip_reachable \"$sm_path\" \"$sha1\" ||\n>>                               fetch_in_submodule \"$sm_path\" $depth ||\n>\n> Is keeping the '||' at the end of this line intended?\n\nyes, as we only want to run the code below if there was some error.\n\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\" $depth \"$sha1\" ||\n>>                               die \"$(eval_gettext \"Fetched in submodule path '\\$displaypath', but it did not contain \\$sha1. Direct fetching of that commit failed.\")\"\n>\n> Should this error message be changed?\n\nI don't think so?\n"},{"id":"347403","messageId":"xmqqmux5g2pa.fsf@gitster-ct.c.googlers.com","threadId":"48468","inReplyTo":"20180511232840.GB41222@aiede.svl.corp.google.com","subject":"Re: [PATCH] git-submodule.sh: try harder to fetch a submodule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-12T00:03:29Z","receivedAt":"2018-05-12T00:03:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>> HEAD is allowed by the protocol spec and would happen, if HEAD points at a\n>> ref, that this user cannot see (due to ACLs for example).\n>\n> A more typical example would be if the ref simply doesn't exist (i.e.,\n> is a branch yet to be born).\n\nIndeed this is interesting.  At first glance I thought this was\nabout underlying \"git clone\" failing to grab things from a\nrepository with unborn HEAD, but that part works perfectly OK.  And\nif this failed clone were a full-repository clone that tried to grab\neven HEAD, then it is likely that we got the tip we need to populate\nthe submodule's working tree (or the remote is useless for that in\nthe first place).  So the \"allow to try even harder\" is probably a\ngood direction to go in.\n\n>>  \t\t\t\t# is not reachable from a ref.\n>>  \t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n>>  \t\t\t\tfetch_in_submodule \"$sm_path\" $depth ||\n>\n> Is keeping the '||' at the end of this line intended?\n\nGood question.  It used to be\n\n\tguard1 || action1 || die\n\tguard2 || action2 || die\n\nEven after a successful exit from \"action1\", the code used to try\nthe second attempt, and the patch is removing the first die, making\nthe whole thing into\n\n\tguard1 || action1 ||\n\tguard2 || action2 || die\n\nwhich suggests a grave regression, doesn't it?  \"action1\" (a whole\nrepository fetch) may not pull down the needed commit even the fetch\noperation itself may succeed, in which case \"guard2\" notices that\nthe tip is still not here and \"action2\" (an exact SHA-1 fetch) tries\nto pull down the exact thing as the last resort.\n\nSo it probably should be more like\n\n\tguard1 || action1 || warn\n\tguard2 || action2 || die\n\nso that no matter what the outcome of the action1 is, the second set\ngets executed.\n\n\n"},{"id":"347707","messageId":"CAGZ79ka_8GmL9j9mTNLkbqN3xNkfCfedzs2st-tH8jMjQ2A4DQ@mail.gmail.com","threadId":"48468","inReplyTo":"xmqqmux5g2pa.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] git-submodule.sh: try harder to fetch a submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-15T19:07:02Z","receivedAt":"2018-05-15T19:07:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, May 11, 2018 at 5:03 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> A more typical example would be if the ref simply doesn't exist (i.e.,\n>> is a branch yet to be born).\n>\n> Indeed this is interesting.  At first glance I thought this was\n> about underlying \"git clone\" failing to grab things from a\n> repository with unborn HEAD, but that part works perfectly OK.\n\nok.\n\n> So it probably should be more like\n>\n>         guard1 || action1 || warn\n>         guard2 || action2 || die\n>\n> so that no matter what the outcome of the action1 is, the second set\n> gets executed.\n>\n\nI'll resend it with a warning (using say()).\n\nI think we have 2 bugs and this is merely fixing the second bug.\n\nThe first bug:\nWe had a call chain as\n  git clone --recurse-submodules=<path spec>\n    -> git submodule update --init --recursive $(pathspec)\n      -> git submodule--helper update-clone # will clone\n        -> git submodule helper clone\n          -> git clone --no-checkout --separate-git-dir ...\n\nThe call to the \"git clone\" produces an interesting\nsubmodule state:\n\n  $ git init confused-head\n  $ (cd confused-head && git branch test \\\n        $(git commit-tree $(git write-tree) -m test))\n  $ git clone --no-checkout  --depth=1 \\\n        --separate-git-dir=test.git confused-head/.git test\nCloning into 'test'...\nwarning: --depth is ignored in local clones; use file:// instead.\ndone.\n\n  $ git -C test.git config remote.origin.fetch\n  $ echo $?\n1\n\n(A) Despite the warning of --depth having no impact, the\n  omission thereof changes the repository state.\n(B) There is no remote.origin.fetch configuration, which\n  is weird. See builtin/clone.c:830, that states for this case:\n\n    /*\n     * otherwise, the next \"git fetch\" will\n     * simply fetch from HEAD without updating\n     * any remote-tracking branch, which is what\n     * we want.\n     */\n\nI disagree as the next fetch will be confused\n(HEAD is not advertised on the next ls-remote)\n\nThe patch that is under discussion here is merely\npapering over the effect of having no fetch spec,\nby allowing the second fetch (fetching the sha1 directly)\nto run, which ignores the configuration as a refspec is\ngiven.\n\nHowever it is still a bug, as such repositories are out there,\nwhich is why I said there are 2 bugs initially. It's just that\nthe first bug enables the second bug.\n\nThanks,\nStefan\n"},{"id":"347709","messageId":"20180515194054.84388-1-sbeller@google.com","threadId":"48468","inReplyTo":"CAGZ79ka_8GmL9j9mTNLkbqN3xNkfCfedzs2st-tH8jMjQ2A4DQ@mail.gmail.com","subject":"[PATCH] git-submodule.sh: try harder to fetch a submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-15T19:40:54Z","receivedAt":"2018-05-15T19:41:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This is the logical continuum of fb43e31f2b4 (submodule: try harder to\nfetch needed sha1 by direct fetching sha1, 2016-02-23) and fixes it as\nsome assumptions were not correct.\n\nThe commit states:\n> If $sha1 was not part of the default fetch ... fail ourselves here\n> assumes that the fetch_in_submodule only fails when the serverside does\n> not support fetching by sha1.\n\nThere are other failures, why such a fetch may fail, such as\n    fatal: Couldn't find remote ref HEAD\nwhich can happen if the remote side doesn't advertise HEAD and we do not\nhave a local fetch refspec.\n\nNot advertising HEAD is allowed by the protocol spec and would happen,\nif HEAD points at an unborn branch for example.\n\nNot having a local fetch refspec can happen when submodules are fetched\nshallowly, as then git-clone doesn't setup a fetch refspec.\n\nSo do try even harder for a submodule by ignoring the exit code of the\nfirst fetch and rather relying on the following is_tip_reachable to\nsee if we try fetching again.\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 24914963ca2..00fcd69138f 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -614,7 +614,7 @@ cmd_update()\n \t\t\t\t# is not reachable from a ref.\n \t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n \t\t\t\tfetch_in_submodule \"$sm_path\" $depth ||\n-\t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n+\t\t\t\tsay \"$(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-- \n2.17.0.582.gccdcbd54c44.dirty\n\n"},{"id":"347717","messageId":"20180515200432.GA40044@aiede.svl.corp.google.com","threadId":"48468","inReplyTo":"CAGZ79ka_8GmL9j9mTNLkbqN3xNkfCfedzs2st-tH8jMjQ2A4DQ@mail.gmail.com","subject":"Re: [PATCH] git-submodule.sh: try harder to fetch a submodule","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-05-15T20:04:32Z","receivedAt":"2018-05-15T20:05:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> I'll resend it with a warning (using say()).\n\nThanks, makes sense.\n\n> I think we have 2 bugs and this is merely fixing the second bug.\n\nI'm fearing that there are more than two.\n\n[...]\n>   $ git init confused-head\n>   $ (cd confused-head && git branch test \\\n>         $(git commit-tree $(git write-tree) -m test))\n>   $ git clone --no-checkout  --depth=1 \\\n>         --separate-git-dir=test.git confused-head/.git test\n> Cloning into 'test'...\n> warning: --depth is ignored in local clones; use file:// instead.\n> done.\n>\n>   $ git -C test.git config remote.origin.fetch\n>   $ echo $?\n> 1\n>\n> (A) Despite the warning of --depth having no impact, the\n>   omission thereof changes the repository state.\n> (B) There is no remote.origin.fetch configuration, which\n>   is weird. See builtin/clone.c:830, that states for this case:\n\nI can reproduce the issue without submodules and without --local,\nas follows:\n\n\tgit init --bare empty.git\n\tgit init --bare almost-empty.git\n\tgit -C ~/src/git push $(pwd)/almost-empty HEAD:refs/heads/upstream\n\n\tgit clone --single-branch file://$(pwd)/empty.git\n\tgit clone --single-branch file://$(pwd)/almost-empty.git\n\n\tgit -C almost-empty.git branch -D upstream\n\n\tgit -C empty fetch\n\tgit -C almost-empty fetch\n\nExpected result:\nBoth fetches succeed.\n\nActual result:\nFirst fetch succeeds, second produces\n\"fatal: Couldn't find remote ref HEAD\".\n\nNote that empty.git and almost-empty.git are basically identical.\nThe difference instead lies in the clones' .git/config files:\n\ndiff --git 1/empty/.git/config 2/almost-empty/.git/config\nindex b51bb0d..ee21198 100644\n--- 1/empty/.git/config\n+++ 2/almost-empty/.git/config\n@@ -4,7 +4,4 @@\n        bare = false\n        logallrefupdates = true\n [remote \"origin\"]\n-       url = file:///tmp/t/empty.git\n-[branch \"master\"]\n-       remote = origin\n-       merge = refs/heads/master\n+       url = file:///tmp/t/almost-empty.git\n\nThanks,\nJonathan\n"}]}