Re: [PATCH] transport submodules: correct error message
Stefan Beller writes: > Trying to push with --recurse-submodules=on-demand would run into > the same problem. To fix this issue > 1) specifically mention that we looked for branches on the remote. That makes an incorrect statement ("not found on any remote"---we did not inspect all of the said remote, only heads and tags) into an irrelevant statement ("not found on any remote branch"---the end user would say "so what? I know it exists there, it's just that not all remote refs have corresponding tracking ref locally on our side"). Perhaps it may be an improvement. > 2) advertise pushing without recursing into submodules. ("Use this >command to make the error message go away") Not mentioning "on-demand" may be an improvement for those who do use set-up like Dave has, where remote tracking information is incomplete if you only look at heads and refs, in the sense that we no longer suggest ineffective workaround. But would it be an improvement to suggest --no-recurse-submodules? This issue seems like a property of the particular set-up, as opposed to being a one-off issue. The next, subsequent and probably all future pushes from that repository will have the same issue because the root cause is not due to the relative position of commits we have locally vs the tips of remote, but due to the way remote tracking is set-up, no? If that is the case, perhaps configuring push.recurseSubmodules to turn this off (especially because you plan to turn the defaul to "check") and not giving the command line option would give a more pleasant end-user experience, I suspect. > > While at it, remove some empty lines, as they blow up the error message. > > Reported-by: Dave Borowitz > Signed-off-by: Stefan Beller > --- > transport.c | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/transport.c b/transport.c > index 3e8799a611..2445bf0dca 100644 > --- a/transport.c > +++ b/transport.c > @@ -883,14 +883,14 @@ static void die_with_unpushed_submodules(struct > string_list *needs_pushing) > int i; > > fprintf(stderr, _("The following submodule paths contain changes that > can\n" > - "not be found on any remote:\n")); > + "not be found on any remote branch:\n")); > for (i = 0; i < needs_pushing->nr; i++) > fprintf(stderr, " %s\n", needs_pushing->items[i].string); > - fprintf(stderr, _("\nPlease 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\n")); > + fprintf(stderr, _("\nSuppress submodule checks via\n" > + " git push --no-recurse-submodules\n" > + "or cd to the path and use\n" > + " git push\n" > + "to push them to a remote.\n")); > > string_list_clear(needs_pushing, 0);
Re: [PATCH] transport submodules: correct error message
On Tue, Jan 17, 2017 at 3:42 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> Trying to push with --recurse-submodules=on-demand would run into >> the same problem. To fix this issue >> 1) specifically mention that we looked for branches on the remote. > > That makes an incorrect statement ("not found on any remote"---we > did not inspect all of the said remote, only heads and tags) into an > irrelevant statement ("not found on any remote branch"---the end > user would say "so what? I know it exists there, it's just that not > all remote refs have corresponding tracking ref locally on our side"). eh. So to be correct we need to tell the user we did not find any match on a "remote-tracking branch" as the gitglossary puts it. > > Perhaps it may be an improvement. > >> 2) advertise pushing without recursing into submodules. ("Use this >>command to make the error message go away") > > Not mentioning "on-demand" may be an improvement for those who do > use set-up like Dave has, Daves setup is not a special setup; it is just git@next (later than 1863e05af5) in a repo that happens to have submodules; using Gerrit as the code review system. > where remote tracking information is > incomplete if you only look at heads and refs, in the sense that we > no longer suggest ineffective workaround. s/ineffective/an effective/ ? Yes. I think this is a problem in general with Git and Gerrit, as they have different assumptions of the refs/ namespace. (You can obtain all Gerrit changes in flight via fetching the refs/changes/* similar to you pushing all git.git working branches not to all repositories, but only https://github.com/gitster/git/) > But would it be an improvement to suggest --no-recurse-submodules? Well it is certainly better than the current situation, the user DID use --recurse-submodules but still git refuses and advises to use --recurse-submodules, which is understandable with background knowledge, but very confusing at least. The hint for --no-recurse-submodules can be understood as: "I told you there is something fishy with submodules, now you can just ignore this warning and murk up your history as you like." which let's me question the correctness of assumptions in 1863e05af5^2. > This issue seems like a property of the particular set-up, as > opposed to being a one-off issue. The next, subsequent until Dave got the changes in the submodules reviewed and they appear on a target branch, then fetching them, then the warning will be gone. > and probably > all future pushes from that repository will have the same issue > because the root cause is not due to the relative position of > commits we have locally vs the tips of remote, but due to the way > remote tracking is set-up, no? Yes; ideally we'd have a remote-tracking ref for each change pushed to Gerrit. > > If that is the case, perhaps configuring push.recurseSubmodules to > turn this off (especially because you plan to turn the defaul to > "check") and not giving the command line option would give a more > pleasant end-user experience, I suspect. I though about going another way and adding another new value to the enum, such that git push --recurse-submodules=sameRefSpecButNoCheck \ origin HEAD:refs/for/master works for Gerrit users. If such a thing exists, then the check is the safest-but-inconvenient option (in all setups I'd think), and users can switch to either sameRefSpecButNoCheck (Gerrit) or on-demand (github et al)
Re: [PATCH] transport submodules: correct error message
Stefan Beller writes: > On Tue, Jan 17, 2017 at 3:42 PM, Junio C Hamano wrote: >> Stefan Beller writes: >> >>> Trying to push with --recurse-submodules=on-demand would run into >>> the same problem. To fix this issue >>> 1) specifically mention that we looked for branches on the remote. >> >> That makes an incorrect statement ("not found on any remote"---we >> did not inspect all of the said remote, only heads and tags) into an >> irrelevant statement ("not found on any remote branch"---the end >> user would say "so what? I know it exists there, it's just that not >> all remote refs have corresponding tracking ref locally on our side"). > > eh. So to be correct we need to tell the user we did not find any match on > a "remote-tracking branch" as the gitglossary puts it. I think the updated text is already "correct". I am pointing out that it may be correct but not very helpful to the users. >> where remote tracking information is >> incomplete if you only look at heads and refs, in the sense that we >> no longer suggest ineffective workaround. > > s/ineffective/an effective/ ? Even though I make many typoes, I meant ineffective in this case. "The old message suggested workaround that would not help. You no longer give that workaround that does not work." >> If that is the case, perhaps configuring push.recurseSubmodules to >> turn this off (especially because you plan to turn the defaul to >> "check") and not giving the command line option would give a more >> pleasant end-user experience, I suspect. > > I though about going another way and adding another new value > to the enum, such that > > git push --recurse-submodules=sameRefSpecButNoCheck \ > origin HEAD:refs/for/master > > works for Gerrit users. It is unclear what that enum tells Git to do. Care to explain? How is it different from "no"?
Re: [PATCH] transport submodules: correct error message
On Tue, Jan 17, 2017 at 4:15 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> On Tue, Jan 17, 2017 at 3:42 PM, Junio C Hamano wrote: >>> Stefan Beller writes: >>> Trying to push with --recurse-submodules=on-demand would run into the same problem. To fix this issue 1) specifically mention that we looked for branches on the remote. >>> >>> That makes an incorrect statement ("not found on any remote"---we >>> did not inspect all of the said remote, only heads and tags) into an >>> irrelevant statement ("not found on any remote branch"---the end >>> user would say "so what? I know it exists there, it's just that not >>> all remote refs have corresponding tracking ref locally on our side"). >> >> eh. So to be correct we need to tell the user we did not find any match on >> a "remote-tracking branch" as the gitglossary puts it. > > I think the updated text is already "correct". I am pointing out > that it may be correct but not very helpful to the users. > >>> where remote tracking information is >>> incomplete if you only look at heads and refs, in the sense that we >>> no longer suggest ineffective workaround. >> >> s/ineffective/an effective/ ? > > Even though I make many typoes, I meant ineffective in this case. > "The old message suggested workaround that would not help. You no > longer give that workaround that does not work." Oh, I misunderstood the original as I lumped on-demand and check together mentally, because in the Gerrit world they behave similar. (Both error out; in the on-demand case the server produces the failure message, though) > >>> If that is the case, perhaps configuring push.recurseSubmodules to >>> turn this off (especially because you plan to turn the defaul to >>> "check") and not giving the command line option would give a more >>> pleasant end-user experience, I suspect. >> >> I though about going another way and adding another new value >> to the enum, such that >> >> git push --recurse-submodules=sameRefSpecButNoCheck \ >> origin HEAD:refs/for/master >> >> works for Gerrit users. > > It is unclear what that enum tells Git to do. Care to explain? How > is it different from "no"? In those submodules, that are checked positively, blindly run git -C ${sub} push ${ref_spec} and do not double check again, which we currently do in the on-demand case.