{"thread":{"id":"44880","subject":"[PATCH] transport submodules: correct error message","startedAt":"2017-01-13T23:54:47Z","lastAt":"2017-01-18T00:22:01Z","messageCount":5,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"309405","messageId":"20170113235427.5768-1-sbeller@google.com","threadId":"44880","inReplyTo":null,"subject":"[PATCH] transport submodules: correct error message","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-13T23:54:27Z","receivedAt":"2017-01-13T23:54:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When push.recurseSubmodules is set to \"check\" or \"on-demand\", the transport\nlayer tries to determine if a submodule needs pushing. This check is done\nby walking all remote refs that are known.\n\nFor remotes we only store the refs/heads/* (and tags), which doesn't\ninclude all commits. In e.g. Gerrit commits often end up at refs/changes/*\n(that we do not store) when pushing to refs/for/master (which we also do\nnot store). So a workflow such as the following still fails:\n\n $ git -C push origin HEAD:refs/for/master\n $ git push origin HEAD:refs/for/master\n The following submodule paths contain changes that can\n not be found on any remote:\n submodule\n\n Please try\n\n git push --recurse-submodules=on-demand\n\n or cd to the path and use\n\n git push\n\n to push them to a remote.\n\nTrying to push with --recurse-submodules=on-demand would run into\nthe same problem. To fix this issue\n 1) specifically mention that we looked for branches on the remote.\n 2) advertise pushing without recursing into submodules. (\"Use this\n command to make the error message go away\")\n\nWhile at it, remove some empty lines, as they blow up the error message.\n\nReported-by: Dave Borowitz \nSigned-off-by: Stefan Beller \n---\n transport.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/transport.c b/transport.c\nindex 3e8799a611..2445bf0dca 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -883,14 +883,14 @@ static void die_with_unpushed_submodules(struct string_list *needs_pushing)\n \tint i;\n \n \tfprintf(stderr, _(\"The following submodule paths contain changes that can\\n\"\n-\t\t\t\"not be found on any remote:\\n\"));\n+\t\t\t\"not be found on any remote branch:\\n\"));\n \tfor (i = 0; i < needs_pushing->nr; i++)\n \t\tfprintf(stderr, \" %s\\n\", needs_pushing->items[i].string);\n-\tfprintf(stderr, _(\"\\nPlease try\\n\\n\"\n-\t\t\t \"\tgit push --recurse-submodules=on-demand\\n\\n\"\n-\t\t\t \"or cd to the path and use\\n\\n\"\n-\t\t\t \"\tgit push\\n\\n\"\n-\t\t\t \"to push them to a remote.\\n\\n\"));\n+\tfprintf(stderr, _(\"\\nSuppress submodule checks via\\n\"\n+\t\t\t \"\tgit push --no-recurse-submodules\\n\"\n+\t\t\t \"or cd to the path and use\\n\"\n+\t\t\t \"\tgit push\\n\"\n+\t\t\t \"to push them to a remote.\\n\"));\n \n \tstring_list_clear(needs_pushing, 0);\n \n-- \n2.11.0.297.g298debce27\n\n"},{"id":"309587","messageId":"xmqq8tq9ckxo.fsf@gitster.mtv.corp.google.com","threadId":"44880","inReplyTo":"20170113235427.5768-1-sbeller@google.com","subject":"Re: [PATCH] transport submodules: correct error message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-17T23:42:59Z","receivedAt":"2017-01-17T23:57:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Trying to push with --recurse-submodules=on-demand would run into\n> the same problem. To fix this issue\n> 1) specifically mention that we looked for branches on the remote.\n\nThat makes an incorrect statement (\"not found on any remote\"---we\ndid not inspect all of the said remote, only heads and tags) into an\nirrelevant statement (\"not found on any remote branch\"---the end\nuser would say \"so what? I know it exists there, it's just that not\nall remote refs have corresponding tracking ref locally on our side\").\n\nPerhaps it may be an improvement.\n\n> 2) advertise pushing without recursing into submodules. (\"Use this\n> command to make the error message go away\")\n\nNot mentioning \"on-demand\" may be an improvement for those who do\nuse set-up like Dave has, where remote tracking information is\nincomplete if you only look at heads and refs, in the sense that we\nno longer suggest ineffective workaround. \n\nBut would it be an improvement to suggest --no-recurse-submodules?\n\nThis issue seems like a property of the particular set-up, as\nopposed to being a one-off issue. The next, subsequent and probably\nall future pushes from that repository will have the same issue\nbecause the root cause is not due to the relative position of\ncommits we have locally vs the tips of remote, but due to the way\nremote tracking is set-up, no?\n\nIf that is the case, perhaps configuring push.recurseSubmodules to\nturn this off (especially because you plan to turn the defaul to\n\"check\") and not giving the command line option would give a more\npleasant end-user experience, I suspect.\n\n\n>\n> While at it, remove some empty lines, as they blow up the error message.\n>\n> Reported-by: Dave Borowitz \n> Signed-off-by: Stefan Beller \n> ---\n> transport.c | 12 ++++++------\n> 1 file changed, 6 insertions(+), 6 deletions(-)\n>\n> diff --git a/transport.c b/transport.c\n> index 3e8799a611..2445bf0dca 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -883,14 +883,14 @@ static void die_with_unpushed_submodules(struct string_list *needs_pushing)\n> \tint i;\n> \n> \tfprintf(stderr, _(\"The following submodule paths contain changes that can\\n\"\n> -\t\t\t\"not be found on any remote:\\n\"));\n> +\t\t\t\"not be found on any remote branch:\\n\"));\n> \tfor (i = 0; i < needs_pushing->nr; i++)\n> \t\tfprintf(stderr, \" %s\\n\", needs_pushing->items[i].string);\n> -\tfprintf(stderr, _(\"\\nPlease try\\n\\n\"\n> -\t\t\t \"\tgit push --recurse-submodules=on-demand\\n\\n\"\n> -\t\t\t \"or cd to the path and use\\n\\n\"\n> -\t\t\t \"\tgit push\\n\\n\"\n> -\t\t\t \"to push them to a remote.\\n\\n\"));\n> +\tfprintf(stderr, _(\"\\nSuppress submodule checks via\\n\"\n> +\t\t\t \"\tgit push --no-recurse-submodules\\n\"\n> +\t\t\t \"or cd to the path and use\\n\"\n> +\t\t\t \"\tgit push\\n\"\n> +\t\t\t \"to push them to a remote.\\n\"));\n> \n> \tstring_list_clear(needs_pushing, 0);\n"},{"id":"309596","messageId":"CAGZ79kYu1Y2pwk9+kbSrMxwP3S0n8FMW6f4wdEE0mrACqrOPNA@mail.gmail.com","threadId":"44880","inReplyTo":"xmqq8tq9ckxo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] transport submodules: correct error message","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-18T00:08:50Z","receivedAt":"2017-01-18T00:09:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jan 17, 2017 at 3:42 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> Trying to push with --recurse-submodules=on-demand would run into\n>> the same problem. To fix this issue\n>> 1) specifically mention that we looked for branches on the remote.\n>\n> That makes an incorrect statement (\"not found on any remote\"---we\n> did not inspect all of the said remote, only heads and tags) into an\n> irrelevant statement (\"not found on any remote branch\"---the end\n> user would say \"so what? I know it exists there, it's just that not\n> all remote refs have corresponding tracking ref locally on our side\").\n\neh. So to be correct we need to tell the user we did not find any match on\na \"remote-tracking branch\" as the gitglossary puts it.\n\n>\n> Perhaps it may be an improvement.\n>\n>> 2) advertise pushing without recursing into submodules. (\"Use this\n>> command to make the error message go away\")\n>\n> Not mentioning \"on-demand\" may be an improvement for those who do\n> use set-up like Dave has,\n\nDaves setup is not a special setup; it is just git@next (later than\n1863e05af5) in a repo that happens to have submodules; using\nGerrit as the code review system.\n\n> where remote tracking information is\n> incomplete if you only look at heads and refs, in the sense that we\n> no longer suggest ineffective workaround.\n\ns/ineffective/an effective/ ?\n\nYes. I think this is a problem in general with Git and Gerrit, as they have\ndifferent assumptions of the refs/ namespace.\n(You can obtain all Gerrit changes in flight via fetching the refs/changes/*\nsimilar to you pushing all git.git working branches not to all repositories,\nbut only https://github.com/gitster/git/)\n\n> But would it be an improvement to suggest --no-recurse-submodules?\n\nWell it is certainly better than the current situation, the user DID\nuse --recurse-submodules but still git refuses and advises to use\n--recurse-submodules, which is understandable with background\nknowledge, but very confusing at least.\n\nThe hint for --no-recurse-submodules can be understood as:\n \"I told you there is something fishy with submodules, now you\n can just ignore this warning and murk up your history as you like.\"\nwhich let's me question the correctness of assumptions in\n1863e05af5^2.\n\n> This issue seems like a property of the particular set-up, as\n> opposed to being a one-off issue. The next, subsequent\n\nuntil Dave got the changes in the submodules reviewed and they\nappear on a target branch, then fetching them, then the warning will\nbe gone.\n\n> and probably\n> all future pushes from that repository will have the same issue\n> because the root cause is not due to the relative position of\n> commits we have locally vs the tips of remote, but due to the way\n> remote tracking is set-up, no?\n\nYes; ideally we'd have a remote-tracking ref for each change\npushed to Gerrit.\n\n>\n> If that is the case, perhaps configuring push.recurseSubmodules to\n> turn this off (especially because you plan to turn the defaul to\n> \"check\") and not giving the command line option would give a more\n> pleasant end-user experience, I suspect.\n\nI though about going another way and adding another new value\nto the enum, such that\n\n git push --recurse-submodules=sameRefSpecButNoCheck \\\n origin HEAD:refs/for/master\n\nworks for Gerrit users.\n\nIf such a thing exists, then the check is the safest-but-inconvenient\noption (in all setups I'd think), and users can switch to either\nsameRefSpecButNoCheck (Gerrit) or on-demand (github et al)\n"},{"id":"309605","messageId":"CAGZ79kaQOrOAnoa9w++wz2aEhwAc1x0bSTCbeXUPedqV7cW06g@mail.gmail.com","threadId":"44880","inReplyTo":"xmqqr341b4vm.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] transport submodules: correct error message","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-18T00:21:37Z","receivedAt":"2017-01-18T00:21:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jan 17, 2017 at 4:15 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> On Tue, Jan 17, 2017 at 3:42 PM, Junio C Hamano wrote:\n>>> Stefan Beller writes:\n>>>\n>>>> Trying to push with --recurse-submodules=on-demand would run into\n>>>> the same problem. To fix this issue\n>>>> 1) specifically mention that we looked for branches on the remote.\n>>>\n>>> That makes an incorrect statement (\"not found on any remote\"---we\n>>> did not inspect all of the said remote, only heads and tags) into an\n>>> irrelevant statement (\"not found on any remote branch\"---the end\n>>> user would say \"so what? I know it exists there, it's just that not\n>>> all remote refs have corresponding tracking ref locally on our side\").\n>>\n>> eh. So to be correct we need to tell the user we did not find any match on\n>> a \"remote-tracking branch\" as the gitglossary puts it.\n>\n> I think the updated text is already \"correct\". I am pointing out\n> that it may be correct but not very helpful to the users.\n>\n>>> where remote tracking information is\n>>> incomplete if you only look at heads and refs, in the sense that we\n>>> no longer suggest ineffective workaround.\n>>\n>> s/ineffective/an effective/ ?\n>\n> Even though I make many typoes, I meant ineffective in this case.\n> \"The old message suggested workaround that would not help. You no\n> longer give that workaround that does not work.\"\n\nOh, I misunderstood the original as I lumped on-demand and check\ntogether mentally, because in the Gerrit world they behave similar.\n(Both error out; in the on-demand case the server produces the failure\nmessage, though)\n\n>\n>>> If that is the case, perhaps configuring push.recurseSubmodules to\n>>> turn this off (especially because you plan to turn the defaul to\n>>> \"check\") and not giving the command line option would give a more\n>>> pleasant end-user experience, I suspect.\n>>\n>> I though about going another way and adding another new value\n>> to the enum, such that\n>>\n>> git push --recurse-submodules=sameRefSpecButNoCheck \\\n>> origin HEAD:refs/for/master\n>>\n>> works for Gerrit users.\n>\n> It is unclear what that enum tells Git to do. Care to explain? How\n> is it different from \"no\"?\n\nIn those submodules, that are checked positively, blindly run\ngit -C ${sub} push ${ref_spec} and do not double check again,\nwhich we currently do in the on-demand case.\n"},{"id":"309606","messageId":"xmqqr341b4vm.fsf@gitster.mtv.corp.google.com","threadId":"44880","inReplyTo":"CAGZ79kYu1Y2pwk9+kbSrMxwP3S0n8FMW6f4wdEE0mrACqrOPNA@mail.gmail.com","subject":"Re: [PATCH] transport submodules: correct error message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-18T00:15:09Z","receivedAt":"2017-01-18T00:22:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Tue, Jan 17, 2017 at 3:42 PM, Junio C Hamano wrote:\n>> Stefan Beller writes:\n>>\n>>> Trying to push with --recurse-submodules=on-demand would run into\n>>> the same problem. To fix this issue\n>>> 1) specifically mention that we looked for branches on the remote.\n>>\n>> That makes an incorrect statement (\"not found on any remote\"---we\n>> did not inspect all of the said remote, only heads and tags) into an\n>> irrelevant statement (\"not found on any remote branch\"---the end\n>> user would say \"so what? I know it exists there, it's just that not\n>> all remote refs have corresponding tracking ref locally on our side\").\n>\n> eh. So to be correct we need to tell the user we did not find any match on\n> a \"remote-tracking branch\" as the gitglossary puts it.\n\nI think the updated text is already \"correct\". I am pointing out\nthat it may be correct but not very helpful to the users.\n\n>> where remote tracking information is\n>> incomplete if you only look at heads and refs, in the sense that we\n>> no longer suggest ineffective workaround.\n>\n> s/ineffective/an effective/ ?\n\nEven though I make many typoes, I meant ineffective in this case.\n\"The old message suggested workaround that would not help. You no\nlonger give that workaround that does not work.\"\n\n>> If that is the case, perhaps configuring push.recurseSubmodules to\n>> turn this off (especially because you plan to turn the defaul to\n>> \"check\") and not giving the command line option would give a more\n>> pleasant end-user experience, I suspect.\n>\n> I though about going another way and adding another new value\n> to the enum, such that\n>\n> git push --recurse-submodules=sameRefSpecButNoCheck \\\n> origin HEAD:refs/for/master\n>\n> works for Gerrit users.\n\nIt is unclear what that enum tells Git to do. Care to explain? How\nis it different from \"no\"?\n"}]}