{"thread":{"id":"50721","subject":"[PATCH] submodule: explain first attempt failure clearly","startedAt":"2019-03-12T17:45:35Z","lastAt":"2019-03-13T23:13:29Z","messageCount":4,"participants":["Jonathan Tan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"371279","messageId":"20190312174522.89306-1-jonathantanmy@google.com","threadId":"50721","inReplyTo":null,"subject":"[PATCH] submodule: explain first attempt failure clearly","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-03-12T17:45:22Z","receivedAt":"2019-03-12T17:45:35Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When cloning with --recurse-submodules a superproject with at least one\nsubmodule with HEAD pointing to an unborn branch, the clone goes\nsomething like this:\n\n\tCloning into 'test'...\n\t<messages about cloning of superproject>\n\tSubmodule '<name>' (<uri>) registered for path '<submodule path>'\n\tCloning into '<submodule path>'...\n\tfatal: Couldn't find remote ref HEAD\n\tUnable to fetch in submodule path '<submodule path>'\n\t<messages about fetching with SHA-1>\n\tFrom <uri>\n\t * branch            <hash> -> FETCH_HEAD\n\tSubmodule path '<submodule path>': checked out '<hash>'\n\nIn other words, first, a fetch is done with no hash arguments (that is,\na fetch of HEAD) resulting in a \"Couldn't find remote ref HEAD\" error;\nthen, a fetch is done given a hash, which succeeds.\n\nThe fetch given a hash was added in fb43e31f2b (\"submodule: try harder\nto fetch needed sha1 by direct fetching sha1\", 2016-02-24), and the\n\"Unable to fetch...\" message was downgraded from a fatal error to a\nnotice in e30d833671 (\"git-submodule.sh: try harder to fetch a\nsubmodule\", 2018-05-16).\n\nThis commit improves the notice to be clearer that we are retrying the\nfetch, and that the previous messages do not necessarily indicate that\nthe whole command fails. In other words:\n\n - If the HEAD-fetch succeeds and we then have the commit we want, no\n   extra messages are printed.\n - If the HEAD-fetch succeeds and we do not have the commit we want, but\n   the hash-fetch succeeds, no additional messages are printed.\n - If the HEAD-fetch succeeds and we do not have the commit we want, but\n   the hash-fetch fails, this is a fatal error.\n - If the HEAD-fetch fails, we print the notice, and if the hash-fetch\n   succeeds, no additional messages are printed.\n - If the HEAD-fetch fails, we print the notice, and if the hash-fetch\n   fails, this is a fatal error.\n\nIt could be said that we should just eliminate the HEAD-fetch\naltogether, but that changes some behavior (in particular, some refs\nthat were opportunistically updated would no longer be), so I have left\nthat alone for now.\n\nThere is an analogous situation with the fetching code in fetch_finish()\nand surrounding functions. For now, I have added a NEEDSWORK.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n git-submodule.sh | 2 +-\n submodule.c      | 7 +++++++\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 514ede2596..2c0fb6d723 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -594,7 +594,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\tsay \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n+\t\t\t\tsay \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'; trying to directly fetch \\$sha1:\")\"\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\ndiff --git a/submodule.c b/submodule.c\nindex 21cf50ca15..b16c0ecc95 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1548,6 +1548,13 @@ static int fetch_finish(int retvalue, struct strbuf *err,\n \tstruct oid_array *commits;\n \n \tif (retvalue)\n+\t\t/*\n+\t\t * NEEDSWORK: This indicates that the overall fetch\n+\t\t * failed, even though there may be a subsequent fetch\n+\t\t * by commit hash that might work. It may be a good\n+\t\t * idea to not indicate failure in this case, and only\n+\t\t * indicate failure if the subsequent fetch fails.\n+\t\t */\n \t\tspf->result = 1;\n \n \tif (!task || !task->sub)\n-- \n2.21.0.155.ge902e9bcae.dirty\n\n"},{"id":"371320","messageId":"xmqqimwnfs25.fsf@gitster-ct.c.googlers.com","threadId":"50721","inReplyTo":"20190312174522.89306-1-jonathantanmy@google.com","subject":"Re: [PATCH] submodule: explain first attempt failure clearly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-13T02:04:34Z","receivedAt":"2019-03-13T02:04:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> When cloning with --recurse-submodules a superproject with at least one\n> submodule with HEAD pointing to an unborn branch, the clone goes\n> something like this:\n>\n> \tCloning into 'test'...\n> \t<messages about cloning of superproject>\n> \tSubmodule '<name>' (<uri>) registered for path '<submodule path>'\n> \tCloning into '<submodule path>'...\n> \tfatal: Couldn't find remote ref HEAD\n> \tUnable to fetch in submodule path '<submodule path>'\n> \t<messages about fetching with SHA-1>\n> \tFrom <uri>\n> \t * branch            <hash> -> FETCH_HEAD\n> \tSubmodule path '<submodule path>': checked out '<hash>'\n>\n> In other words, first, a fetch is done with no hash arguments (that is,\n> a fetch of HEAD) resulting in a \"Couldn't find remote ref HEAD\" error;\n> then, a fetch is done given a hash, which succeeds.\n>\n> The fetch given a hash was added in fb43e31f2b (\"submodule: try harder\n> to fetch needed sha1 by direct fetching sha1\", 2016-02-24), and the\n> \"Unable to fetch...\" message was downgraded from a fatal error to a\n> notice in e30d833671 (\"git-submodule.sh: try harder to fetch a\n> submodule\", 2018-05-16).\n>\n> This commit improves the notice to be clearer that we are retrying the\n> fetch, and that the previous messages do not necessarily indicate that\n> the whole command fails. In other words:\n\n\"additional\" is relative to something.  Is it additional to status-quo?\n>\n>  - If the HEAD-fetch succeeds and we then have the commit we want, no\n>    extra messages are printed.\n>  - If the HEAD-fetch succeeds and we do not have the commit we want, but\n>    the hash-fetch succeeds, no additional messages are printed.\n\nFor example, the user would be helped to be told that we succeeded\ndoing a pointless fetch and we now are going to make a more specific\n\"give me that commit\" fetch to see if it helps.  These may already\nbe given with the current code, or may not be (I don't know\noffhand), but \"no additional messages\" does not help me in knowing\nif the code with this patch would give such a hint to the users.\n\n>  - If the HEAD-fetch succeeds and we do not have the commit we want, but\n>    the hash-fetch fails, this is a fatal error.\n>  - If the HEAD-fetch fails, we print the notice, and if the hash-fetch\n>    succeeds, no additional messages are printed.\n>  - If the HEAD-fetch fails, we print the notice, and if the hash-fetch\n>    fails, this is a fatal error.\n>\n> It could be said that we should just eliminate the HEAD-fetch\n> altogether, but that changes some behavior (in particular, some refs\n> that were opportunistically updated would no longer be), so I have left\n> that alone for now.\n\nOK, that sounds good decision for this patch.\n\nA more fundamental question is why these submodule repositories do\nnot have HEAD to recommend from which branch to fetch by default,\nand if we can do something about it so that HEAD-fetch fails less\noften for the users.\n\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 514ede2596..2c0fb6d723 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -594,7 +594,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\tsay \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n> +\t\t\t\tsay \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'; trying to directly fetch \\$sha1:\")\"\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\nObviously good ;-)\n\nThanks.\n\n> diff --git a/submodule.c b/submodule.c\n> index 21cf50ca15..b16c0ecc95 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1548,6 +1548,13 @@ static int fetch_finish(int retvalue, struct strbuf *err,\n>  \tstruct oid_array *commits;\n>  \n>  \tif (retvalue)\n> +\t\t/*\n> +\t\t * NEEDSWORK: This indicates that the overall fetch\n> +\t\t * failed, even though there may be a subsequent fetch\n> +\t\t * by commit hash that might work. It may be a good\n> +\t\t * idea to not indicate failure in this case, and only\n> +\t\t * indicate failure if the subsequent fetch fails.\n> +\t\t */\n>  \t\tspf->result = 1;\n>  \n>  \tif (!task || !task->sub)\n"},{"id":"371379","messageId":"20190313175738.252961-1-jonathantanmy@google.com","threadId":"50721","inReplyTo":"20190312174522.89306-1-jonathantanmy@google.com","subject":"[PATCH v2] submodule: explain first attempt failure clearly","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-03-13T17:57:38Z","receivedAt":"2019-03-13T17:57:46Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When cloning with --recurse-submodules a superproject with at least one\nsubmodule with HEAD pointing to an unborn branch, the clone goes\nsomething like this:\n\n\tCloning into 'test'...\n\t<messages about cloning of superproject>\n\tSubmodule '<name>' (<uri>) registered for path '<submodule path>'\n\tCloning into '<submodule path>'...\n\tfatal: Couldn't find remote ref HEAD\n\tUnable to fetch in submodule path '<submodule path>'\n\t<messages about fetching with SHA-1>\n\tFrom <uri>\n\t * branch            <hash> -> FETCH_HEAD\n\tSubmodule path '<submodule path>': checked out '<hash>'\n\nIn other words, first, a fetch is done with no hash arguments (that is,\na fetch of HEAD) resulting in a \"Couldn't find remote ref HEAD\" error;\nthen, a fetch is done given a hash, which succeeds.\n\nThe fetch given a hash was added in fb43e31f2b (\"submodule: try harder\nto fetch needed sha1 by direct fetching sha1\", 2016-02-24), and the\n\"Unable to fetch...\" message was downgraded from a fatal error to a\nnotice in e30d833671 (\"git-submodule.sh: try harder to fetch a\nsubmodule\", 2018-05-16).\n\nThis commit improves the notice to be clearer that we are retrying the\nfetch, and that the previous messages (in particular, the fatal errors\nfrom fetch) do not necessarily indicate that the whole command fails. In\nother words:\n\n - If the HEAD-fetch succeeds and we then have the commit we want,\n   git-submodule prints no explanation.\n - If the HEAD-fetch succeeds and we do not have the commit we want, but\n   the hash-fetch succeeds, git-submodule prints no explanation.\n - If the HEAD-fetch succeeds and we do not have the commit we want, but\n   the hash-fetch fails, git-submodule prints a fatal error.\n - If the HEAD-fetch fails, fetch prints a fatal error, and\n   git-submodule informs the user that it will retry by fetching\n   specific commits by hash.\n   - If the hash-fetch then succeeds, git-submodule prints no\n     explanation (besides the ones already printed).\n   - If the HEAD-fetch then fails, git-submodule prints a fatal error.\n\nIt could be said that we should just eliminate the HEAD-fetch\naltogether, but that changes some behavior (in particular, some refs\nthat were opportunistically updated would no longer be), so I have left\nthat alone for now.\n\nThere is an analogous situation with the fetching code in fetch_finish()\nand surrounding functions. For now, I have added a NEEDSWORK.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nChange from v1: No code change; commit message change can be seen from\nthe range-diff below.\n\n> \"additional\" is relative to something.  Is it additional to status-quo?\n> >\n> >  - If the HEAD-fetch succeeds and we then have the commit we want, no\n> >    extra messages are printed.\n> >  - If the HEAD-fetch succeeds and we do not have the commit we want, but\n> >    the hash-fetch succeeds, no additional messages are printed.\n> \n> For example, the user would be helped to be told that we succeeded\n> doing a pointless fetch and we now are going to make a more specific\n> \"give me that commit\" fetch to see if it helps.  These may already\n> be given with the current code, or may not be (I don't know\n> offhand), but \"no additional messages\" does not help me in knowing\n> if the code with this patch would give such a hint to the users.\n\nIt is additional to what fetch prints. To make it clearer, I have\nremoved all mentions of \"additional\" from the commit message. But right\nnow I'm not sure if that whole section is important (since what happens\ncan be deduced quite easily by reading the fewer than 10 lines of code).\n\n(Also, to answer your question, we currently don't tell the user that we\nsucceeded in a pointless fetch and will make another one.)\n\n> A more fundamental question is why these submodule repositories do\n> not have HEAD to recommend from which branch to fetch by default,\n> and if we can do something about it so that HEAD-fetch fails less\n> often for the users.\n\nIf you mean a recommendation to the repository administrator, I guess we\ncan recommend that their HEAD points to something, even if it's just a\nsingle orphan commit with a .txt file explaining what's going on.\n\nRange-diff against v1:\n1:  1e3fd22f33 ! 1:  d7399fc35e submodule: explain first attempt failure clearly\n    @@ -28,19 +28,22 @@\n         submodule\", 2018-05-16).\n     \n         This commit improves the notice to be clearer that we are retrying the\n    -    fetch, and that the previous messages do not necessarily indicate that\n    -    the whole command fails. In other words:\n    +    fetch, and that the previous messages (in particular, the fatal errors\n    +    from fetch) do not necessarily indicate that the whole command fails. In\n    +    other words:\n     \n    -     - If the HEAD-fetch succeeds and we then have the commit we want, no\n    -       extra messages are printed.\n    +     - If the HEAD-fetch succeeds and we then have the commit we want,\n    +       git-submodule prints no explanation.\n          - If the HEAD-fetch succeeds and we do not have the commit we want, but\n    -       the hash-fetch succeeds, no additional messages are printed.\n    +       the hash-fetch succeeds, git-submodule prints no explanation.\n          - If the HEAD-fetch succeeds and we do not have the commit we want, but\n    -       the hash-fetch fails, this is a fatal error.\n    -     - If the HEAD-fetch fails, we print the notice, and if the hash-fetch\n    -       succeeds, no additional messages are printed.\n    -     - If the HEAD-fetch fails, we print the notice, and if the hash-fetch\n    -       fails, this is a fatal error.\n    +       the hash-fetch fails, git-submodule prints a fatal error.\n    +     - If the HEAD-fetch fails, fetch prints a fatal error, and\n    +       git-submodule informs the user that it will retry by fetching\n    +       specific commits by hash.\n    +       - If the hash-fetch then succeeds, git-submodule prints no\n    +         explanation (besides the ones already printed).\n    +       - If the HEAD-fetch then fails, git-submodule prints a fatal error.\n     \n         It could be said that we should just eliminate the HEAD-fetch\n         altogether, but that changes some behavior (in particular, some refs\n\n git-submodule.sh | 2 +-\n submodule.c      | 7 +++++++\n 2 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 514ede2596..2c0fb6d723 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -594,7 +594,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\tsay \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'\")\"\n+\t\t\t\tsay \"$(eval_gettext \"Unable to fetch in submodule path '\\$displaypath'; trying to directly fetch \\$sha1:\")\"\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\ndiff --git a/submodule.c b/submodule.c\nindex 21cf50ca15..b16c0ecc95 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1548,6 +1548,13 @@ static int fetch_finish(int retvalue, struct strbuf *err,\n \tstruct oid_array *commits;\n \n \tif (retvalue)\n+\t\t/*\n+\t\t * NEEDSWORK: This indicates that the overall fetch\n+\t\t * failed, even though there may be a subsequent fetch\n+\t\t * by commit hash that might work. It may be a good\n+\t\t * idea to not indicate failure in this case, and only\n+\t\t * indicate failure if the subsequent fetch fails.\n+\t\t */\n \t\tspf->result = 1;\n \n \tif (!task || !task->sub)\n-- \n2.21.0.155.ge902e9bcae.dirty\n\n"},{"id":"371415","messageId":"xmqqmulycqr0.fsf@gitster-ct.c.googlers.com","threadId":"50721","inReplyTo":"20190313175738.252961-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] submodule: explain first attempt failure clearly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-13T23:13:23Z","receivedAt":"2019-03-13T23:13:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> It is additional to what fetch prints. To make it clearer, I have\n> removed all mentions of \"additional\" from the commit message. But right\n> now I'm not sure if that whole section is important (since what happens\n> can be deduced quite easily by reading the fewer than 10 lines of code).\n\nHmph, perhaps.  It was lot easier to follow, at least to me, though ;-)\n\n> If you mean a recommendation to the repository administrator, I guess we\n> can recommend that their HEAD points to something, even if it's just a\n> single orphan commit with a .txt file explaining what's going on.\n\nYeah, that might make sense, too.\n"}]}