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

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

[Christopher Tiwald]

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:

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

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.

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.

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.

Signed-off-by: Christopher Tiwald <christiwald@gmail.com>
Based-on-patch-by: Junio C Hamano <gitster@pobox.com>
---
This is a reroll of jc/advise-push-default (2011-12-18). Apologies if
it is out of band. I saw some chatter recently about 1.7.10.rc*. I
wasn't sure if that meant "Don't submit patches at this point in the
cycle" or "Feel free to submit patches, but they might not be
acknowledged for a while." Mostly, I wanted to move this topic out of
stalled in the "What's Cooking" emails. I'm happy to resubmit at a
better time.

The patch is based on jc/advise-push-default and attempts to
implement Peff's logic in [1]. It would also require a change if the
push default behavior changes [2], but I think the core logic is
sound. `git push` should be smart enough to distinguish different
types of non-fast-forward updates and advise accordingly regardless of
its default.

[1] http://thread.gmane.org/gmane.comp.version-control.git/187079/focus=187447
[2] http://thread.gmane.org/gmane.comp.version-control.git/192547/focus=192694

 Documentation/config.txt |   15 ++++++++++
 advice.c                 |    6 ++++
 advice.h                 |    3 ++
 builtin/push.c           |   72 ++++++++++++++++++++++++++++++++++++++++++----
 builtin/send-pack.c      |    2 +-
 cache.h                  |    9 ++++--
 environment.c            |    2 +-
 transport.c              |   17 ++++++++---
 8 files changed, 112 insertions(+), 14 deletions(-)

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.
 --
 
 core.fileMode::
diff --git a/advice.c b/advice.c
index 01130e5..608e90d 100644
--- a/advice.c
+++ b/advice.c
@@ -6,6 +6,9 @@ int advice_commit_before_merge = 1;
 int advice_resolve_conflict = 1;
 int advice_implicit_identity = 1;
 int advice_detached_head = 1;
+int advice_pull_before_push = 1;
+int advice_use_upstream = 1;
+int advice_checkout_pull_push = 1;
 
 static struct {
 	const char *name;
@@ -17,6 +20,9 @@ static struct {
 	{ "resolveconflict", &advice_resolve_conflict },
 	{ "implicitidentity", &advice_implicit_identity },
 	{ "detachedhead", &advice_detached_head },
+	{ "pullbeforepush", &advice_pull_before_push },
+	{ "useupstream", &advice_use_upstream },
+	{ "checkoutpullpush", &advice_checkout_pull_push }
 };
 
 void advise(const char *advice, ...)
diff --git a/advice.h b/advice.h
index 7bda45b..ac07a44 100644
--- a/advice.h
+++ b/advice.h
@@ -9,6 +9,9 @@ extern int advice_commit_before_merge;
 extern int advice_resolve_conflict;
 extern int advice_implicit_identity;
 extern int advice_detached_head;
+extern int advice_use_upstream;
+extern int advice_pull_before_push;
+extern int advice_checkout_pull_push;
 
 int git_default_advice_config(const char *var, const char *value);
 void advise(const char *advice, ...);
diff --git a/builtin/push.c b/builtin/push.c
index d315475..0fecf06 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -24,6 +24,7 @@ static int progress = -1;
 static const char **refspec;
 static int refspec_nr;
 static int refspec_alloc;
+static int default_matching_used;
 
 static void add_refspec(const char *ref)
 {
@@ -95,6 +96,9 @@ static void setup_default_push_refspecs(struct remote *remote)
 {
 	switch (push_default) {
 	default:
+	case PUSH_DEFAULT_UNSPECIFIED:
+		default_matching_used = 1;
+		/* fallthru */
 	case PUSH_DEFAULT_MATCHING:
 		add_refspec(":");
 		break;
@@ -114,6 +118,59 @@ static void setup_default_push_refspecs(struct remote *remote)
 	}
 }
 
+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."
+};
+
+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."
+};
+
+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."
+};
+
+static void advise_pull_before_push(void)
+{
+	int i;
+
+	if (!advice_pull_before_push)
+		return;
+	for (i = 0; i < ARRAY_SIZE(message_advice_pull_before_push); i++)
+		advise(message_advice_pull_before_push[i]);
+}
+
+static void advise_use_upstream(void)
+{
+	int i;
+
+	if (!advice_use_upstream)
+		return;
+	for (i = 0; i < ARRAY_SIZE(message_advice_use_upstream); i++)
+		advise(message_advice_use_upstream[i]);
+}
+
+static void advise_checkout_pull_push(void)
+{
+	int i;
+
+	if (!advice_checkout_pull_push)
+		return;
+	for (i = 0; i < ARRAY_SIZE(message_advice_checkout_pull_push); i++)
+		advise(message_advice_checkout_pull_push[i]);
+}
+
 static int push_with_options(struct transport *transport, int flags)
 {
 	int err;
@@ -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();
+	}
+
 	if (!err)
 		return 0;
 
-	if (nonfastforward && advice_push_nonfastforward) {
-		fprintf(stderr, _("To prevent you from losing history, non-fast-forward updates were rejected\n"
-				"Merge the remote changes (e.g. 'git pull') before pushing again.  See the\n"
-				"'Note about fast-forwards' section of 'git push --help' for details.\n"));
-	}
-
 	return 1;
 }
 
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 9df341c..09895b9 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -409,7 +409,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	int send_all = 0;
 	const char *receivepack = "git-receive-pack";
 	int flags;
-	int nonfastforward = 0;
+	int nonfastforward = NONFASTFORWARD_NONE;
 
 	argv++;
 	for (i = 1; i < argc; i++, argv++) {
diff --git a/cache.h b/cache.h
index e5e1aa4..14bc305 100644
--- a/cache.h
+++ b/cache.h
@@ -625,7 +625,8 @@ enum push_default_type {
 	PUSH_DEFAULT_NOTHING = 0,
 	PUSH_DEFAULT_MATCHING,
 	PUSH_DEFAULT_UPSTREAM,
-	PUSH_DEFAULT_CURRENT
+	PUSH_DEFAULT_CURRENT,
+	PUSH_DEFAULT_UNSPECIFIED
 };
 
 extern enum branch_track git_branch_track;
@@ -1008,7 +1009,6 @@ struct ref {
 	char *symref;
 	unsigned int force:1,
 		merge:1,
-		nonfastforward:1,
 		deletion:1;
 	enum {
 		REF_STATUS_NONE = 0,
@@ -1019,6 +1019,11 @@ struct ref {
 		REF_STATUS_REMOTE_REJECT,
 		REF_STATUS_EXPECTING_REPORT
 	} status;
+	enum {
+		NONFASTFORWARD_NONE = 0,
+		NONFASTFORWARD_HEAD,
+		NONFASTFORWARD_OTHER
+	} nonfastforward;
 	char *remote_status;
 	struct ref *peer_ref; /* when renaming */
 	char name[FLEX_ARRAY]; /* more */
diff --git a/environment.c b/environment.c
index c93b8f4..d7e6c65 100644
--- a/environment.c
+++ b/environment.c
@@ -52,7 +52,7 @@ enum safe_crlf safe_crlf = SAFE_CRLF_WARN;
 unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
 enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;
 enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
-enum push_default_type push_default = PUSH_DEFAULT_MATCHING;
+enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
 #ifndef OBJECT_CREATION_MODE
 #define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
 #endif
diff --git a/transport.c b/transport.c
index 181f8f2..23210d5 100644
--- a/transport.c
+++ b/transport.c
@@ -721,6 +721,10 @@ void transport_print_push_status(const char *dest, struct ref *refs,
 {
 	struct ref *ref;
 	int n = 0;
+	unsigned char head_sha1[20];
+	char *head;
+
+	head = resolve_refdup("HEAD", head_sha1, 1, NULL);
 
 	if (verbose) {
 		for (ref = refs; ref; ref = ref->next)
@@ -732,14 +736,19 @@ void transport_print_push_status(const char *dest, struct ref *refs,
 		if (ref->status == REF_STATUS_OK)
 			n += print_one_push_status(ref, dest, n, porcelain);
 
-	*nonfastforward = 0;
+	*nonfastforward = NONFASTFORWARD_NONE;
 	for (ref = refs; ref; ref = ref->next) {
 		if (ref->status != REF_STATUS_NONE &&
 		    ref->status != REF_STATUS_UPTODATE &&
 		    ref->status != REF_STATUS_OK)
 			n += print_one_push_status(ref, dest, n, porcelain);
-		if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD)
-			*nonfastforward = 1;
+		if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD &&
+		    *nonfastforward != NONFASTFORWARD_HEAD) {
+			if (!strcmp(head, ref->name))
+				*nonfastforward = NONFASTFORWARD_HEAD;
+			else
+				*nonfastforward = NONFASTFORWARD_OTHER;
+		}
 	}
 }
 
@@ -1008,7 +1017,7 @@ int transport_push(struct transport *transport,
 		   int refspec_nr, const char **refspec, int flags,
 		   int *nonfastforward)
 {
-	*nonfastforward = 0;
+	*nonfastforward = NONFASTFORWARD_NONE;
 	transport_verify_remote_names(refspec_nr, refspec);
 
 	if (transport->push) {
-- 
1.7.10.rc0.42.gefc97.dirty

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

[Junio C Hamano]

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?

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

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

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

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

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

> @@ -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?

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

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

[Matthieu Moy]

Christopher Tiwald <christiwald@gmail.com> writes:

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

Very good point.

Depending on the outcome of the discussion in the thread about
'push.default', you may want to suggest 'current' instead of upstream:
http://thread.gmane.org/gmane.comp.version-control.git/192547/focus=192694

Actually, if the user has 'push.default=matching', the least surprising
move from this value is 'push.default=current', that will push a subset
of what used to be pushed, and won't change the target branch.

> +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."
> +};

Your patch removes the _(...) around the string, which breaks the
internationalization.

> +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."
> +};

I'd give the full cut-and-paste ready command to set the variable, to
help the user who doesn't know what "configuration variable" really
means in the context of Git:

... you may want to run

  git config push.default upstream (or current, if you like my remark above)

to ask Git to push only the current branch from now on.

(I'd advise "git config" without "--global" here, because the user may
want to do something else in other repositories)

> +	for (i = 0; i < ARRAY_SIZE(message_advice_pull_before_push); i++)
> +		advise(message_advice_pull_before_push[i]);

I'm no expert in gettext, but I think the internationalization people
will have a hard time dealing with a single message split accross an
array.

Actually, I prefer the effect of a single advise() call (i.e. say
"hint:" just once, not for each line), but this part is subjective.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Zbigniew Jędrzejewski-Szmek]

On Tue, Mar 13, 2012 at 09:27:08PM -0700, Junio C Hamano wrote:
> Christopher Tiwald <christiwald@gmail.com> writes:
>  * 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"?

Hi,

I think that having three different config keys for the three
different advices makes sense, because the advices will be displayed
at different times. E.g. the user starts with the simplest one-branch
workflow, triggers the first alternative, reads "pullBeforePush" and
then disables the hint. Then the team upgrades the workflow to use
several branches and the user triggers the second alternative. At this
point, git should hint to "useUpstream". If the user disabled all the
non-FF hints at the first advice, she would miss the second, different
one, later.

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

Zbyszek

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

[Matthieu Moy]

Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:

> I think that having three different config keys for the three
> different advices makes sense, because the advices will be displayed
> at different times.

I don't think it really makes sense to be such fine-grained. We already
have 6 different advices, so an advance user who do not want them need
to set these 6 variables. I think we want to keep this number relatively
low.

The advice messages do not point explicitely to the way to disable them,
so users who know how to set advice.* are users who know a little about
configuration files, and who read the docs. Instead of having too
fine-grained configuration variables, we can have a better doc,
explaining shortly the 3 possible cases under advice.nonfastforward in
config.txt. The user who disable the advice can read the doc (I usually
think that "users don't read documentation" is a better assumption, but
since the user knows about the name of the variable, it is OK here).

Also, if I read correctly the patch, the old variable is left in the doc
and in advice.{c,h}, but is no longer used. This means old-timers who
have set it will see the message poping-up again after they upgrade,
which I think is inconveinient for them.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Zbigniew Jędrzejewski-Szmek]

On Wed, Mar 14, 2012 at 02:00:38PM +0100, Matthieu Moy wrote:
> Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:
> 
> > I think that having three different config keys for the three
> > different advices makes sense, because the advices will be displayed
> > at different times.
> 
> I don't think it really makes sense to be such fine-grained. We already
> have 6 different advices, so an advance user who do not want them need
> to set these 6 variables. I think we want to keep this number relatively
> low.
> 
> The advice messages do not point explicitely to the way to disable them,
> so users who know how to set advice.* are users who know a little about
> configuration files, and who read the docs. 

Elsewhere in this thread it was proposed to add an actual 'git config'
command to the advice.

> Instead of having too
> fine-grained configuration variables, we can have a better doc,
> explaining shortly the 3 possible cases under advice.nonfastforward in
> config.txt. The user who disable the advice can read the doc (I usually
> think that "users don't read documentation" is a better assumption, but
> since the user knows about the name of the variable, it is OK here).
> 
> Also, if I read correctly the patch, the old variable is left in the doc
> and in advice.{c,h}, but is no longer used. This means old-timers who
> have set it will see the message poping-up again after they upgrade,
> which I think is inconveinient for them.

So it seems that the old variable should be respected, not to annoy
"old-timers".

Zbyszek

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

[Christopher Tiwald]

On Tue, Mar 13, 2012 at 09:27:08PM -0700, Junio C Hamano wrote:
> An off-topic administrivia. Please do not try to deflect responses meant
> for you by setting Mail-Followup-To.

Thanks for catching this. Truth be told I downloaded a command line MUA
specifically to send this patch and read in others from this list. I've
been wrestling with the config and will wrestle it further.

> 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?
> 
> > 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?

These notes make sense and I will reroll v2 with them in mind, as well
as the other comments about the advice wording, sign-off line, and making
nonfastforward a switch, not quoted here.

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

After mulling over it, I tend to agree with this, but will address
further down the thread.

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

This was an oversight on my part. Given that the current error message
has been in place since 07436e4 in 2009 and doesn't seem to have caused
trouble (other than it not being applicable in some 'git push'
situations), I'll move the code in v2.

Thanks for the comments. They are much appreciated.

--
Christopher Tiwald

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

[Christopher Tiwald]

On Wed, Mar 14, 2012 at 10:48:03AM -0400, Christopher Tiwald wrote:
> <stuff>

Whoops. Also apologies for not following the correct To: and Cc:
convention in my most recent response.

--
Christopher Tiwald

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

[Christopher Tiwald]

On Wed, Mar 14, 2012 at 10:06:52AM +0100, Matthieu Moy wrote:
> Depending on the outcome of the discussion in the thread about
> 'push.default', you may want to suggest 'current' instead of upstream:
> http://thread.gmane.org/gmane.comp.version-control.git/192547/focus=192694
> 
> Actually, if the user has 'push.default=matching', the least surprising
> move from this value is 'push.default=current', that will push a subset
> of what used to be pushed, and won't change the target branch.

My only concern about 'push.default=current' vs. 'upstream' is the
case where a developer might push to a central shared repository, but
has a local branch tracking a remote branch with a different name.
That might be too deep in the edge-case weeds, but it seems like for
'centralized' git users, 'upstream' covers more cases without any
distraction to their workflows.

> Your patch removes the _(...) around the string, which breaks the
> internationalization.
> ...
> > +	for (i = 0; i < ARRAY_SIZE(message_advice_pull_before_push); i++)
> > +		advise(message_advice_pull_before_push[i]);
> 
> I'm no expert in gettext, but I think the internationalization people
> will have a hard time dealing with a single message split accross an
> array.
> 
> Actually, I prefer the effect of a single advise() call (i.e. say
> "hint:" just once, not for each line), but this part is subjective.

The lack of support for internationalization is an oversight. I'll
correct it in v2.

> I'd give the full cut-and-paste ready command to set the variable, to
> help the user who doesn't know what "configuration variable" really
> means in the context of Git.

Makes a lot of sense. I remember struggling with setting config
variables when I was new to git. I'll make that change.

--
Christopher Tiwald

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

[Christopher Tiwald]

On Wed, Mar 14, 2012 at 03:27:52PM +0100, Zbigniew Jędrzejewski-Szmek wrote:
> On Wed, Mar 14, 2012 at 02:00:38PM +0100, Matthieu Moy wrote:
> > Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:
> > 
> > > I think that having three different config keys for the three
> > > different advices makes sense, because the advices will be displayed
> > > at different times.
> > 
> > I don't think it really makes sense to be such fine-grained. We already
> > have 6 different advices, so an advance user who do not want them need
> > to set these 6 variables. I think we want to keep this number relatively
> > low.
> > 
> > The advice messages do not point explicitely to the way to disable them,
> > so users who know how to set advice.* are users who know a little about
> > configuration files, and who read the docs. 
> 
> Elsewhere in this thread it was proposed to add an actual 'git config'
> command to the advice.

After considering it, I tend to agree that three different config keys
is overkill. I feel like users who disable advice are doing it because
they find the messages annoying, not because they've mastered that
particular situation and no longer need the reminder. Forcing them to
disable three different options to get an advice-less 'git push' seems
like it'd just be irritating.

I could be wrong about that. Perhaps users who graduate workflows as you
described above are more common? I don't disable any advice locally and
thus can't speak well to what motivates that decision.
> 
> > Instead of having too
> > fine-grained configuration variables, we can have a better doc,
> > explaining shortly the 3 possible cases under advice.nonfastforward in
> > config.txt. The user who disable the advice can read the doc (I usually
> > think that "users don't read documentation" is a better assumption, but
> > since the user knows about the name of the variable, it is OK here).
> > 
> > Also, if I read correctly the patch, the old variable is left in the doc
> > and in advice.{c,h}, but is no longer used. This means old-timers who
> > have set it will see the message poping-up again after they upgrade,
> > which I think is inconveinient for them.
> 
> So it seems that the old variable should be respected, not to annoy
> "old-timers".

I hadn't considered users who already have the variable set. I'll
correct for that. I'll also attempt to improve the doc for
advice.nonfastforward.

--
Christopher Tiwald

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

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> I'm no expert in gettext, but I think the internationalization people
> will have a hard time dealing with a single message split accross an
> array.

My original patch on this topic predates the i18n adjustment we made to
advice infrastructure in 23cb5bf (i18n of multi-line advice messages,
2011-12-22), so that is an understandable oversight.

Thanks for catching this.

> Actually, I prefer the effect of a single advise() call (i.e. say
> "hint:" just once, not for each line), but this part is subjective.

The way advice.c::error_resolve_conflict() uses multi-line advice message
should be a good template.  The choice between "hint" for once or for
every line can later be adjusted in advice.c::advice() if we want to and
such a change will convert all the users of advice API consistently.

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

[Clemens Buchacher]

On Wed, Mar 14, 2012 at 03:27:52PM +0100, Zbigniew Jędrzejewski-Szmek wrote:
> On Wed, Mar 14, 2012 at 02:00:38PM +0100, Matthieu Moy wrote:
> > Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:
> > 
> > > I think that having three different config keys for the three
> > > different advices makes sense, because the advices will be displayed
> > > at different times.
> > 
> > I don't think it really makes sense to be such fine-grained. We already
> > have 6 different advices, so an advance user who do not want them need
> > to set these 6 variables. I think we want to keep this number relatively
> > low.
> > 
> > The advice messages do not point explicitely to the way to disable them,
> > so users who know how to set advice.* are users who know a little about
> > configuration files, and who read the docs. 
> 
> Elsewhere in this thread it was proposed to add an actual 'git config'
> command to the advice.

The proposed command does not turn off the advice. It only changes
push.default. The advice about push.default is effectively disabled once
they change push.default, but the other warnings are still in effect.

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

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> On Wed, Mar 14, 2012 at 03:27:52PM +0100, Zbigniew Jędrzejewski-Szmek wrote:
>> On Wed, Mar 14, 2012 at 02:00:38PM +0100, Matthieu Moy wrote:
>> ...
>> > The advice messages do not point explicitely to the way to disable them,
>> > so users who know how to set advice.* are users who know a little about
>> > configuration files, and who read the docs. 
>> 
>> Elsewhere in this thread it was proposed to add an actual 'git config'
>> command to the advice.
>
> The proposed command does not turn off the advice. It only changes
> push.default. The advice about push.default is effectively disabled once
> they change push.default, but the other warnings are still in effect.

True.

I looked to see if some existing message that is triggered by advice.* has
an extra comment at the end to suggest setting advice.* to false to
decline seeing the advice in the future, as it feels like a sensible thing
to do and also I vaguely recalled us actually doing such a patch, but I do
not seem to be able to find such a message in the current codebase.

Nothing from a quick "git log --no-merges --grep=advice --grep=advise"
pops at me telling that we used to have instructions on how to decline but
we deliberately removed them, so I probably is misremembering things.

We do mention them in git-config(1), but it may be hard to match the
variables to situations from the description there UNLESS the user already
understands what the annoying "I know what I am doing, no need for this
advice anymore" advice is about.

Oh, wait.  Perhaps the advice messages are designed to be declined only by
the user who do understand, so perhaps it is a *good* think that we do not
mention how to squelch in the message.  In a twisted way, the logic sort
of makes sense.

I dunno.

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

[Junio C Hamano]

Here is what I'll queue on top of your patch in 'pu', based on the review
comments in the thread.

This message is primarily to make sure everybody is on the same page,
and ask eyeballs of people to make sure that I did not screw-up.

 Documentation/config.txt |   19 ++----------
 advice.c                 |    6 ----
 advice.h                 |    3 --
 builtin/push.c           |   76 +++++++++++++++++++++-------------------------
 builtin/send-pack.c      |    2 +-
 cache.h                  |    5 ++-
 transport.c              |   10 +++---
 7 files changed, 44 insertions(+), 77 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 50d9249..6e86681 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -138,8 +138,8 @@ advice.*::
 +
 --
 	pushNonFastForward::
-		Advice shown when linkgit:git-push[1] refuses
-		non-fast-forward refs.
+		Advice shown when linkgit:git-push[1] fails due to a
+		non-fast-forward update.
 	statusHints::
 		Directions on how to stage/unstage/add shown in the
 		output of linkgit:git-status[1] and the template shown
@@ -158,21 +158,6 @@ 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.
 --
 
 core.fileMode::
diff --git a/advice.c b/advice.c
index 608e90d..01130e5 100644
--- a/advice.c
+++ b/advice.c
@@ -6,9 +6,6 @@ int advice_commit_before_merge = 1;
 int advice_resolve_conflict = 1;
 int advice_implicit_identity = 1;
 int advice_detached_head = 1;
-int advice_pull_before_push = 1;
-int advice_use_upstream = 1;
-int advice_checkout_pull_push = 1;
 
 static struct {
 	const char *name;
@@ -20,9 +17,6 @@ static struct {
 	{ "resolveconflict", &advice_resolve_conflict },
 	{ "implicitidentity", &advice_implicit_identity },
 	{ "detachedhead", &advice_detached_head },
-	{ "pullbeforepush", &advice_pull_before_push },
-	{ "useupstream", &advice_use_upstream },
-	{ "checkoutpullpush", &advice_checkout_pull_push }
 };
 
 void advise(const char *advice, ...)
diff --git a/advice.h b/advice.h
index ac07a44..7bda45b 100644
--- a/advice.h
+++ b/advice.h
@@ -9,9 +9,6 @@ extern int advice_commit_before_merge;
 extern int advice_resolve_conflict;
 extern int advice_implicit_identity;
 extern int advice_detached_head;
-extern int advice_use_upstream;
-extern int advice_pull_before_push;
-extern int advice_checkout_pull_push;
 
 int git_default_advice_config(const char *var, const char *value);
 void advise(const char *advice, ...);
diff --git a/builtin/push.c b/builtin/push.c
index 0fecf06..d7587d7 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -118,57 +118,45 @@ static void setup_default_push_refspecs(struct remote *remote)
 	}
 }
 
-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."
-};
-
-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."
-};
-
-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."
-};
+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.");
 
 static void advise_pull_before_push(void)
 {
-	int i;
-
-	if (!advice_pull_before_push)
+	if (!advice_push_nonfastforward)
 		return;
-	for (i = 0; i < ARRAY_SIZE(message_advice_pull_before_push); i++)
-		advise(message_advice_pull_before_push[i]);
+	advise(_(message_advice_pull_before_push));
 }
 
 static void advise_use_upstream(void)
 {
-	int i;
-
-	if (!advice_use_upstream)
+	if (!advice_push_nonfastforward)
 		return;
-	for (i = 0; i < ARRAY_SIZE(message_advice_use_upstream); i++)
-		advise(message_advice_use_upstream[i]);
+	advise(_(message_advice_use_upstream));
 }
 
 static void advise_checkout_pull_push(void)
 {
-	int i;
-
-	if (!advice_checkout_pull_push)
+	if (!advice_push_nonfastforward)
 		return;
-	for (i = 0; i < ARRAY_SIZE(message_advice_checkout_pull_push); i++)
-		advise(message_advice_checkout_pull_push[i]);
+	advise(_(message_advice_checkout_pull_push));
 }
 
 static int push_with_options(struct transport *transport, int flags)
@@ -192,19 +180,23 @@ static int push_with_options(struct transport *transport, int flags)
 		error(_("failed to push some refs to '%s'"), transport->url);
 
 	err |= transport_disconnect(transport);
+	if (!err)
+		return 0;
 
-	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;
 	}
 
-	if (!err)
-		return 0;
-
 	return 1;
 }
 
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 09895b9..9df341c 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -409,7 +409,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	int send_all = 0;
 	const char *receivepack = "git-receive-pack";
 	int flags;
-	int nonfastforward = NONFASTFORWARD_NONE;
+	int nonfastforward = 0;
 
 	argv++;
 	for (i = 1; i < argc; i++, argv++) {
diff --git a/cache.h b/cache.h
index 14bc305..427b600 100644
--- a/cache.h
+++ b/cache.h
@@ -1020,9 +1020,8 @@ struct ref {
 		REF_STATUS_EXPECTING_REPORT
 	} status;
 	enum {
-		NONFASTFORWARD_NONE = 0,
-		NONFASTFORWARD_HEAD,
-		NONFASTFORWARD_OTHER
+		NON_FF_HEAD = 1,
+		NON_FF_OTHER
 	} nonfastforward;
 	char *remote_status;
 	struct ref *peer_ref; /* when renaming */
diff --git a/transport.c b/transport.c
index 23210d5..7864007 100644
--- a/transport.c
+++ b/transport.c
@@ -736,18 +736,18 @@ void transport_print_push_status(const char *dest, struct ref *refs,
 		if (ref->status == REF_STATUS_OK)
 			n += print_one_push_status(ref, dest, n, porcelain);
 
-	*nonfastforward = NONFASTFORWARD_NONE;
+	*nonfastforward = 0;
 	for (ref = refs; ref; ref = ref->next) {
 		if (ref->status != REF_STATUS_NONE &&
 		    ref->status != REF_STATUS_UPTODATE &&
 		    ref->status != REF_STATUS_OK)
 			n += print_one_push_status(ref, dest, n, porcelain);
 		if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD &&
-		    *nonfastforward != NONFASTFORWARD_HEAD) {
+		    *nonfastforward != NON_FF_HEAD) {
 			if (!strcmp(head, ref->name))
-				*nonfastforward = NONFASTFORWARD_HEAD;
+				*nonfastforward = NON_FF_HEAD;
 			else
-				*nonfastforward = NONFASTFORWARD_OTHER;
+				*nonfastforward = NON_FF_OTHER;
 		}
 	}
 }
@@ -1017,7 +1017,7 @@ int transport_push(struct transport *transport,
 		   int refspec_nr, const char **refspec, int flags,
 		   int *nonfastforward)
 {
-	*nonfastforward = NONFASTFORWARD_NONE;
+	*nonfastforward = 0;
 	transport_verify_remote_names(refspec_nr, refspec);
 
 	if (transport->push) {
-- 
1.7.10.rc1.22.g07e85

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

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> Oh, wait.  Perhaps the advice messages are designed to be declined only by
> the user who do understand, so perhaps it is a *good* think that we do not
> mention how to squelch in the message.  In a twisted way, the logic sort
> of makes sense.

I'd be against having a detailed message with the cut-and-paste ready
command to decline the message, as the messages would become long and
annoying, so people would disable it too early.

But having a short mention like "(to squelch this message, set
advice.bla)", short enough not to be disturbing, and vague enough to
force people to read the docs.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

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

[Clemens Buchacher]

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.

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

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

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

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

[Christopher Tiwald]

On Fri, Mar 16, 2012 at 05:03:58AM -0700, Junio C Hamano wrote:
> > 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.

Yeah, this was how I interpretted Peff's original suggestion. It seemed
like a nice compromise between advice that was inapplicable and advice
that was too complex ("There are 3 different non-ff errors in your push.
Here are the four resolution processes required to fix them...").

Thanks for the additional patching. The language / logic changes make
sense. One quick, slightly-off-topic question: I'd like
to take another crack at the patch's commit message, to implement
some of your language suggestions and clean it up further. Is it
reasonable for me to wait a few days for additional comments or
updates, squash together these fixups into a single v2 patch (assuming
one patch is a logical unit for it), then resubmit?

Just wanted to clarify the workflow,

--
Christopher Tiwald

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

[Junio C Hamano]

Christopher Tiwald <christiwald@gmail.com> writes:

> One quick, slightly-off-topic question: I'd like to take another crack
> at the patch's commit message, to implement some of your language
> suggestions and clean it up further. Is it reasonable for me to wait a
> few days for additional comments or updates, squash together these
> fixups into a single v2 patch (assuming one patch is a logical unit for
> it), then resubmit?

Surely.

After reading the fix-up patch again, I actually have a couple of
comments/reservations myself.

 (1) I suggested (and the fix-up patch does so) to use a single existing
     advice configuration, but if you read the description in my response
     to Clemens carefully, you may realize that at least the configuration
     for "Here is how to deal with your current branch" and "Here is how
     for the rest of your branch" might be better if they are separate
     variables. The user may fix current branch (i.e. "pull then push"),
     set the advice.pushNonFastForward to false thinking that he learned
     everything there to know about non fast-forward, and then get another
     failure from "git push" because other branches are still behind, but
     with my "fix-up" patch, we would no longer give advice to him.

 (2) The advice to "Your current branch is OK but you are also pushing
     others that do not fast-forward" only talks about "check out, pull
     and then push", but an equally plausible solution may be "don't push
     other branches---you are not working on them right now".  Both of our
     versions have this issue.

     I didn't trace the logic flow, though. If this advice is issued only
     to the user who explicitly said he wants to push these other branches
     (e.g. has "push.default = matching" in the config and gave no command
     line options, or gave refspec on the command line to tell us to push
     these other branches), then the wording is OK.

     But for the purpose of helping people who may be surprised by the
     current "matching" default, I think we should detect this very narrow
     case:

     - The user did not give us any refspec from the command line; and

     - The user does not have push.default set to matching (either the
       user does not have any push.default, or it is set to something
       else); and

     - The remote.$name.push would not push the branch other than the
       current branch.

     When these three conditions hold, we can be sure that the user worked
     on more than one branch and did "git push $there" without telling us
     what to push, and we defaulted to push "matching" and failed on stale
     branches that the user hasn't been working on.  In that case, "don't
     push other branches---perhaps push.default needs to be set" may be a
     far more appropriate advice.

     So, the third case may have to be split further into two.

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

[Clemens Buchacher]

On Fri, Mar 16, 2012 at 05:03:58AM -0700, Junio C Hamano wrote:
> Clemens Buchacher <drizzd@aon.at> writes:
> 
> > 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.

Sorry if I did not express myself well. I should have deleted the first
message. I was not talking about the case where the current branch is
rejected. I mean the two cases where other branches are rejected.

And the suggested solutions may be different for those too. I did not
mean to object to that either. I only object to those two sentences,
which basically say the same thing in different ways:

 "Some of your local branches were stale with respect to their\n"
 "remote counterparts.

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


> > 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, I am not talking about the current branch situation at all. I
don't understand what you mean here.

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

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> On Fri, Mar 16, 2012 at 05:03:58AM -0700, Junio C Hamano wrote:
>> Clemens Buchacher <drizzd@aon.at> writes:
>> ...
>> >> +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.");
>> >
> ...
> Sorry if I did not express myself well. I should have deleted the first
> message. I was not talking about the case where the current branch is
> rejected. I mean the two cases where other branches are rejected.

Oh, I see.  My reply ended up being very similar, though ;-)

These two apply to two different situations.  In the code, you can see
we switch between them like this:

> +	case NON_FF_OTHER:
>  		if (default_matching_used)
>  			advise_use_upstream();
>  		else
>  			advise_checkout_pull_push();
> +		break;

This distinguishes the two cases _why_ you ended up pushing branches that
are not your current branch.  default_matching_used is set (eh, at least,
"designed to be set"; the patch is based on my oooold patch whose details
I do not recall offhand) only when the user said either "git push" or "git
push $there" without explicit pathspec (i.e. "git push $there other" does
not set it to true) and we end up using the "matching" semantics as it is
the current built-in default.

If you tried to push other branch because you weren't aware of the
matching default, you get the first advice, if you explicitly tried to
push other branch, 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 think they are clear
enough (that is why I sent the pach), but the wording may still need to be
polished, and updates are welcome.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> After reading the fix-up patch again, I actually have a couple of
> comments/reservations myself.
>
>  (1) I suggested (and the fix-up patch does so) to use a single existing
>      advice configuration, but ...
>
>  (2) The advice to "Your current branch is OK but you are also pushing

Well, (2) is untrue.  Between message-advice-checkout-pull-push and
message-advice-use-upstream, we already capture this difference just fine.

Sorry about the noise.

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

[Clemens Buchacher]

On Fri, Mar 16, 2012 at 02:53:42PM -0700, Junio C Hamano wrote:
> Clemens Buchacher <drizzd@aon.at> writes:
> 
> > On Fri, Mar 16, 2012 at 05:03:58AM -0700, Junio C Hamano wrote:
> >> Clemens Buchacher <drizzd@aon.at> writes:
> >> ...
> >> >> +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.");
> >> >
> > ...
> > Sorry if I did not express myself well. I should have deleted the first
> > message. I was not talking about the case where the current branch is
> > rejected. I mean the two cases where other branches are rejected.
> 
> Oh, I see.  My reply ended up being very similar, though ;-)

I am afraid we are still not talking about the same thing...

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

[Junio C Hamano]

Clemens Buchacher <drizzd@aon.at> writes:

> I am afraid we are still not talking about the same thing...

Then I give up and shut up, at least for now, as it would not help you if
I rephrase what I said in a different way.

It's your turn to try explaining what is different better.

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

[Zbigniew Jędrzejewski-Szmek]

On 03/16/2012 06:36 AM, 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.");

Hi,

Clemens' observation that there are unnecessary differences between
"message_advice_use_upstream" and "message_advice_checkout_pull_push"
is valid. There also was a grammatical error in message_advice_checkout_pull_push
("the tip ... are behind") and some tense/number inconsistencies.

I think the following can be squashed into 'fixup push-non-ff advice':

- always start with "Updates were rejected", i.e. explain what is why
  git is talking
- consistently use present tense to talk about stuff which is still true
- mention that branches to be pushed can be specified (add
  " explicitly specify branches to push or" in
  "you may want to set the 'push.default' configuration variable")
- use the simpler "tip of your branch is behind the remote" instead of the more 
  complicated and longer "some of your branches are stale with respect to their 
  remote counterparts".
- resolve the "tip ... are" problem by using singular and talking about
  a single branch. This way there is no conflict with the following 
  sentence which talks about checking out a single branch.
- rewrap the text to 72 lines (standard TeX paragraph width).
  (One line is 73 characters, but it seems better than the 
  alternative which makes the text take an extra line).

[I know that this mixes whitespace/layout changes with the rest, but the texts were mostly rewritten anyway.]

Zbyszek

------ 8< --------
From ef8d15494d518df809e4a822af0d0e1c4008c91e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>
Date: Sat, 17 Mar 2012 18:00:42 +0100
Subject: [PATCH] fixup! fixup push-non-ff advice

---
 builtin/push.c |   25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 511a3ba..4c5b52b 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -142,24 +142,21 @@ static void setup_default_push_refspecs(struct remote *remote)
 }
 
 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.");
-
+	N_("Update was rejected because the tip of your current branch is behind the\n"
+	   "remote. Merge the remote changes (e.g. 'git pull') before pushing again.\n"
+	   "See the 'Note about fast-forwards' in '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.");
+	N_("Updates were rejected because a tip of your branch is behind the remote.\n"
+	   "If you did not intend to push that branch, you may want to explicitly\n"
+	   "specify branches to push or set the 'push.default' configuration variable"
+	   "to 'current' or 'upstream' to always 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.");
+	N_("Updates were rejected because a tip of your branch is behind the remote.\n"
+	   "Check out this branch and merge the remote changes (e.g. 'git pull')\n"
+	   "before pushing again.\n"
+	   "See the 'Note about fast-forwards' in 'git push --help' for details.");
 
 static void advise_pull_before_push(void)
 {
-- 
1.7.10.rc0.162.g5dce3

------ >8 --------

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

[Christopher Tiwald]

On Sat, Mar 17, 2012 at 06:10:35PM +0100, Zbigniew Jędrzejewski-Szmek wrote:
>  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.");
> +	N_("Updates were rejected because a tip of your branch is behind the remote.\n"
> +	   "If you did not intend to push that branch, you may want to explicitly\n"
> +	   "specify branches to push or set the 'push.default' configuration variable"
> +	   "to 'current' or 'upstream' to always push only the current branch.");

I prefer the "Some of your local..." language to "Updates were
rejected..." as a reader, but I think you're right about providing the
reason git rejected the push up front.

My concern about this particular message is "tip of your branch is behind
the remote" reads to me like my _current_ branch is the offender, when
that cannot be the case (it'd hit message_advice_pull_before_push
first). Maybe something like this might make it clearer?

"Updates were rejected because a pushed branch tip is behind its remote
counterpart. If you did not intend to push that branch, you may want to
explicitly specify branches to push or set the 'push.default' configuration
variable to 'current' or 'upstream' to always push only the current branch."

--
Christopher Tiwald

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

[Zbigniew Jędrzejewski-Szmek]

On 03/17/2012 07:46 PM, Christopher Tiwald wrote:
> On Sat, Mar 17, 2012 at 06:10:35PM +0100, Zbigniew Jędrzejewski-Szmek wrote:
>>   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.");
>> +	N_("Updates were rejected because a tip of your branch is behind the remote.\n"
>> +	   "If you did not intend to push that branch, you may want to explicitly\n"
>> +	   "specify branches to push or set the 'push.default' configuration variable"
>> +	   "to 'current' or 'upstream' to always push only the current branch.");
>
> I prefer the "Some of your local..." language to "Updates were
> rejected..." as a reader, but I think you're right about providing the
> reason git rejected the push up front.
>
> My concern about this particular message is "tip of your branch is behind
> the remote" reads to me like my _current_ branch is the offender, when
> that cannot be the case (it'd hit message_advice_pull_before_push
> first). Maybe something like this might make it clearer?
>
> "Updates were rejected because a pushed branch tip is behind its remote
> counterpart. If you did not intend to push that branch, you may want to
> explicitly specify branches to push or set the 'push.default' configuration
> variable to 'current' or 'upstream' to always push only the current branch."
Yeah, that's better.

Zbyszek

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

[Junio C Hamano]

Christopher Tiwald <christiwald@gmail.com> writes:

> I prefer the "Some of your local..." language to "Updates were
> rejected..." as a reader, but I think you're right about providing the
> reason git rejected the push up front.

Ok.

> My concern about this particular message is "tip of your branch is behind
> the remote" reads to me like my _current_ branch is the offender, when
> that cannot be the case (it'd hit message_advice_pull_before_push
> first). Maybe something like this might make it clearer?
>
> "Updates were rejected because a pushed branch tip is behind its remote
> counterpart. If you did not intend to push that branch, you may want to
> explicitly specify branches to push or set the 'push.default' configuration
> variable to 'current' or 'upstream' to always push only the current branch."

Sounds good.