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

[Clemens Buchacher <drizzd@aon.at>]

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."

I like that you changed the advice to 'current' _or_ 'upstream'. But
maybe the variable name should change from message_advice_use_upstream
to message_advice_push_default.

> -	if (nonfastforward == NONFASTFORWARD_HEAD) {
> +	switch (nonfastforward) {
> +	default:
> +		break;
> +	case NON_FF_HEAD:
>  		advise_pull_before_push();
> -	} else if (nonfastforward == NONFASTFORWARD_OTHER) {
> +		break;
> +	case NON_FF_OTHER:
>  		if (default_matching_used)
>  			advise_use_upstream();
>  		else
>  			advise_checkout_pull_push();
> +		break;
>  	}

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.

Actually, if the user decides that matching branches is indeed the
default they want to use, advise_checkout_pull_push would still be
helpful. So I think advise_checkout_pull_push should be given in any
case, while advise_use_upstream should be added if push.default=matching
and the user did not say git push --all.