[PATCH] transport submodules: correct error message

[PATCH] transport submodules: correct error message

[Stefan Beller]

When push.recurseSubmodules is set to "check" or "on-demand", the transport
layer tries to determine if a submodule needs pushing. This check is done
by walking all remote refs that are known.

For remotes we only store the refs/heads/* (and tags), which doesn't
include all commits. In e.g. Gerrit commits often end up at refs/changes/*
(that we do not store) when pushing to refs/for/master (which we also do
not store). So a workflow such as the following still fails:

    $ git -C <submodule> push origin HEAD:refs/for/master
    $ git push origin HEAD:refs/for/master
    The following submodule paths contain changes that can
    not be found on any remote:
      submodule

    Please try

        git push --recurse-submodules=on-demand

    or cd to the path and use

        git push

    to push them to a remote.

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.
    2) advertise pushing without recursing into submodules. ("Use this
       command to make the error message go away")

While at it, remove some empty lines, as they blow up the error message.

Reported-by: Dave Borowitz <dborowitz@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 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);
 
-- 
2.11.0.297.g298debce27

Re: [PATCH] transport submodules: correct error message

[Junio C Hamano]

Stefan Beller <sbeller@google.com> 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 <dborowitz@google.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  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

[Stefan Beller]

On Tue, Jan 17, 2017 at 3:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> 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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Tue, Jan 17, 2017 at 3:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> 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

[Stefan Beller]

On Tue, Jan 17, 2017 at 4:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> On Tue, Jan 17, 2017 at 3:42 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Stefan Beller <sbeller@google.com> 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.