Re: [PATCH] push: Provide situational hints for non-fast-forward errors

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 16, 2012, 12:03 UTC
Message-ID: <7v3998kb0x.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20120316091019.GB22273@ecki>

Clemens Buchacher <drizzd@aon.at> writes:

Show 29 quoted lines

> On Thu, Mar 15, 2012 at 10:36:22PM -0700, Junio C Hamano wrote:
>>
>> +static const char message_advice_pull_before_push[] =
>> +	N_("Update was rejected because the tip of your current branch is behind\n"
>> +	   "the remote. Merge the remote changes (e.g. 'git pull') before\n"
>> +	   "pushing again. See the 'Note about fast-forwards' section of\n"
>> +	   "'git push --help' for details.");
>> +
>> +
>> +static const char message_advice_use_upstream[] =
>> +	N_("Some of your local branches were stale with respect to their\n"
>> +	   "remote counterparts. If you did not intend to push these branches,\n"
>> +	   "you may want to set the 'push.default' configuration variable to\n"
>> +	   "'current' or 'upstream' to push only the current branch.");
>> +
>> +static const char message_advice_checkout_pull_push[] =
>> +	N_("Updates were rejected because the tip of some of your branches are\n"
>> +	   "behind the remote. Check out the branch and merge the remote\n"
>> +	   "changes (e.g. 'git pull') before pushing again. See the\n"
>> +	   "'Note about fast-forwards' section of 'git push --help'\n"
>> +	   "for details.");
>
> The first sentence of the above two warnings state the same thing, but
> in different ways. Yet the difference does not reflect the different
> situations. They should be the same, or maybe the first one should be
> changed to the following variant of the second:
>
>  "Updates were rejected because the tip of some of your branches are
>  behind the remote branches with matching names."

That defeats the whole point of Christpher's patch and suggestion by Peff in the original discussion.

They apply to two different situations. If your current branch is behind, you get the first one, if your current branch is *NOT* behind, but some others are, you get the second one. The suggested solutions are different.

Read each of them in isolation, imagining that you just saw your action resulted in a corresponding error condition. I thought they are clear enough (that is why I sent the pach), but the wording may still need to be polished, and updates are welcome.

> We should not give advise_use_upstream if the user specified git push
> --all. The advice_checkout_pull_push would make more sense in that case.

Yeah, "default_matching_used" variable should be looked at somewhere around that, but I *think* the approach Christpher and Peff took (and I agree with them) is to help solving the immediate problem the user has and can address. Deal with the current branch first (which would solve "the current branch is behind" problem). The next push may then show that other branches are behind, and at that time the other advice will tell him how to deal with it ("check out and fix them, and then push").

Again, updates are welcome.