git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Mar 14, 2012, 04:27 UTC
Message-ID
<7vobrzst7n.fsf@alter.siamese.dyndns.org>
In-Reply-To
<20120313232256.GA49626@democracyinaction.org>

An off-topic administrivia. Please do not try to deflect responses meant for you by setting Mail-Followup-To.

Christopher Tiwald <christiwald@gmail.com> writes:
> Pushing a non-fast-forward update to a remote repository will result in
> an error, but the hint text doesn't provide the correct resolution in
> every case. Three scenarios may arise depending on your workflow, each
> with a different resolution:

Are we sure there are only three, or is this just "we do not say anything concrete, but at least we know common three cases, and there may be more"?

I am mostly interested in making sure that we do not give a bad advice. Giving an advice that is mostly accurate and relevant for 95% of the time is perfectly fine, as long as following the advice in the remaining 5% does not result in a disaster.

> 1) If you push a non-fast-forward update to HEAD, you should merge
> remote changes with 'git pull' before pushing again.

You said "to HEAD", but I think you meant the case you push your current branch (i.e. HEAD) to update any ref on the other side. In other words, the push does not have to be "*to*" HEAD over there. Am I mistaken?

Show 6 quoted lines
> 2) If you push to a shared repository others push to, and your local
> tracking branches are not kept up to date, the 'matching refs' default
> will generate non-fast-forward errors on outdated branches. If this is
> your workflow, the 'matching refs' default is not for you. Consider
> setting the 'push.default' configuration variable to 'upstream' to
> ensure only your checked-out branch is pushed.
OK.
> 3) If you push with explicit ref matching (e.g. 'git push ... topic:topic')
> while checked out on another branch (e.g. 'master'), the correct
> resolution is checking out the local branch, issuing git pull, and
> merging remote changes before pushing again.

Or you may have misspelled the source side of the refspec and tried to push a wrong branch.

> Make nonfastforward an enum and teach transport.c to detect the
> scenarios described above. Give situation-specific resolution advice
> when pushes are rejected due to non-fast-forward updates. Finally,
> update other instances of nonfastforward to use the proper enum option.

I think the overall direction of the implemention is good, modulo minor design nits.

 * I do not particularly find NONFASTFORWARD_NONE that is defined to be 0
   a useful readability measure. Plain vanilla constant 0 says that there
   is nothing magical going on to the readers clearly already.
 * Also NONFASTFORWARD_FROTZ is way too long.  Wouldn't NONFF_FROTZ be
   sufficient and clear?
 * I can see there are three kinds of advices, but I do not see why users
   need to acknowledge that they understand them one by one with separate
   advice configuration.  Isn't it better to have only one variable, "OK,
   I know how to deal with a failed push due to non-fast-forward"?
> Signed-off-by: Christopher Tiwald <christiwald@gmail.com>
> Based-on-patch-by: Junio C Hamano <gitster@pobox.com>
These two lines are chronologically swapped.
> ---
> This is a reroll of jc/advise-push-default (2011-12-18).

I lost track of this topic during the last round. Thanks for picking it up.

Show 23 quoted lines
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index c081657..50d9249 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -158,6 +158,21 @@ advice.*::
>  		Advice shown when you used linkgit:git-checkout[1] to
>  		move to the detach HEAD state, to instruct how to create
>  		a local branch after the fact.
> +	pullBeforePush::
> +		Advice shown when you ran linkgit:git-push[1] and pushed
> +		a non-fast-forward update to HEAD, instructing you to
> +		linkgit:git-pull[1] before pushing again.
> +	useUpstream::
> +		Advice to set 'push.default' to 'upstream' when you ran
> +		linkgit:git-push[1] and pushed 'matching refs' by default
> +		(i.e. you did not have any explicit refspec on the command
> +		line, and no 'push.default' configuration was set) and it
> +		resulted in a non-fast-forward error.
> +	checkoutPullPush::
> +		Advice shown when you ran linkgit:git-push[1] and pushed
> +		a non-fast-forward update to a non-HEAD branch, instructing
> +		you to checkout the branch and run linkgit:git-pull[1]
> +		before pushing again.

I would prefer to see these consolidated into a single advice.pushNonFF variable, but I may be missing why it could be a good idea to allow them turned off selectively.

Show 13 quoted lines
> diff --git a/builtin/push.c b/builtin/push.c
> index d315475..0fecf06 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
>  	}
>  }
>  
> +static const char *message_advice_pull_before_push[] = {
> +	"To prevent you from losing history, non-fast-forward updates to HEAD",
> +	"were rejected. Merge the remote changes (e.g. 'git pull') before",
> +	"pushing again. See the 'Note about fast-forwards' section of",
> +	"'git push --help' for details."
> +};

This again says "*to* HEAD". If this should be "a non-fast-forward update to send the current branch was rejected" as I suspected above, the message needs to be rephrased accordingly.

Show 7 quoted lines
> +static const char *message_advice_use_upstream[] = {
> +	"By default, git pushes all branches that have a matching counterpart",
> +	"on the remote. In this case, some of your local branches were stale",
> +	"with respect to their remote counterparts. If you did not intend to",
> +	"push these branches, you may want to set the 'push.default'",
> +	"configuration variable to 'upstream' to push only the current branch."
> +};

If you drop everything up to and including "In this case, ", the advice message still teaches exactly what the user needs to learn.

Show 7 quoted lines
> +static const char *message_advice_checkout_pull_push[] = {
> +	"To prevent you from losing history, your non-fast-forward branch",
> +	"updates were rejected. Checkout the branch and merge the remote",
> +	"changes (e.g. 'git pull') before pushing again. See the",
> +	"'Note about fast-forwards' section of 'git push --help' for",
> +	"details."
> +};
OK.
Show 12 quoted lines
> @@ -136,15 +193,18 @@ static int push_with_options(struct transport *transport, int flags)
>  
>  	err |= transport_disconnect(transport);
>  
> +	if (nonfastforward == NONFASTFORWARD_HEAD) {
> +		advise_pull_before_push();
> +	} else if (nonfastforward == NONFASTFORWARD_OTHER) {
> +		if (default_matching_used)
> +			advise_use_upstream();
> +		else
> +			advise_checkout_pull_push();
> +	}
		
I suspect that we may find more cases not just three, so
	switch (nonfastforward) {
	default:
        	break;
	case NONFF_HEAD:
        	advice_pull_before_push();
		break;
	case NONFF_OTHER:
		...
	}
would be a more forward-looking way to write it.

Also, shouldn't we be doing this only when err is true, or is it too defensive?

Show 5 quoted lines
>  	if (!err)
>  		return 0;
>  
> -	if (nonfastforward && advice_push_nonfastforward) {
> -		fprintf(stderr, _("To prevent you from losing history,...

That is, I am wondering why your "more detailed diag & advice" code is not here, i.e. after "if (!err) return 0".

Previous: Christopher TiwaldNext: Zbigniew Jędrzejewski-Szmek
Message 2 of 28 in “push: Provide situational hints for non-fast-forward errors”
  1. push: Provide situational hints for non-fast-forward errorsChristopher Tiwald, Mar 13, 2012
  2. Junio C HamanoMar 14, 2012
  3. Zbigniew Jędrzejewski-SzmekMar 14, 2012
  4. Matthieu MoyMar 14, 2012
  5. Zbigniew Jędrzejewski-SzmekMar 14, 2012
  6. Christopher TiwaldMar 14, 2012
  7. Clemens BuchacherMar 15, 2012
  8. Junio C HamanoMar 15, 2012
  9. Matthieu MoyMar 16, 2012
  10. Christopher TiwaldMar 14, 2012
  11. Christopher TiwaldMar 14, 2012
  12. Matthieu MoyMar 14, 2012
  13. Christopher TiwaldMar 14, 2012
  14. Junio C HamanoMar 14, 2012
  15. Junio C HamanoMar 16, 2012
  16. Clemens BuchacherMar 16, 2012
  17. Junio C HamanoMar 16, 2012
  18. Christopher TiwaldMar 16, 2012
  19. Junio C HamanoMar 16, 2012
  20. Junio C HamanoMar 16, 2012
  21. Clemens BuchacherMar 16, 2012
  22. Junio C HamanoMar 16, 2012
  23. Clemens BuchacherMar 16, 2012
  24. Junio C HamanoMar 16, 2012
  25. push: Provide situational hints for non-fast-forward errorsZbigniew Jędrzejewski-Szmek, Mar 17, 2012
  26. Christopher TiwaldMar 17, 2012
  27. Zbigniew Jędrzejewski-SzmekMar 17, 2012
  28. Junio C HamanoMar 19, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.