threads / patch / 29944

patchpush: Provide situational hints for non-fast-forward errors

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

## tl;dr

28 messages between Mar 13, 2012 and Mar 19, 2012. Diffs are folded; open one to read it.

replies: 27people: 5as markdown or json

Christopher Tiwald· Mar 13, 2012, 23:22 UTC · lore

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(-)
Show changes to 8 files +112 −14

Documentation/config.txt, advice.c, advice.h, builtin/push.c, builtin/send-pack.c, cache.h, environment.c, transport.c

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
Junio C Hamano· Mar 14, 2012, 04:27 UTC · re: Christopher Tiwald · lore

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

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

Zbigniew Jędrzejewski-Szmek· Mar 14, 2012, 12:14 UTC · re: Junio C Hamano · lore

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

On Tue, Mar 13, 2012 at 09:27:08PM -0700, Junio C Hamano wrote:
Show 5 quoted lines
> 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.

Show 27 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.
Zbyszek
Matthieu Moy· Mar 14, 2012, 13:00 UTC · re: Zbigniew Jędrzejewski-Szmek · lore

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

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/
Zbigniew Jędrzejewski-Szmek· Mar 14, 2012, 14:27 UTC · re: Matthieu Moy · lore

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

On Wed, Mar 14, 2012 at 02:00:38PM +0100, Matthieu Moy wrote:
Show 14 quoted lines
> 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.

Show 11 quoted lines
> 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
Christopher Tiwald· Mar 14, 2012, 16:40 UTC · re: Zbigniew Jędrzejewski-Szmek · lore

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

On Wed, Mar 14, 2012 at 03:27:52PM +0100, Zbigniew Jędrzejewski-Szmek wrote:
Show 18 quoted lines
> 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.

Show 15 quoted lines
> 
> > 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

Clemens Buchacher· Mar 15, 2012, 08:54 UTC · re: Zbigniew Jędrzejewski-Szmek · lore

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

On Wed, Mar 14, 2012 at 03:27:52PM +0100, Zbigniew Jędrzejewski-Szmek wrote:
Show 18 quoted lines
> 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.

Junio C Hamano· Mar 15, 2012, 18:06 UTC · re: Clemens Buchacher · lore

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

Clemens Buchacher <drizzd@aon.at> writes:
Show 13 quoted lines
> 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.
Matthieu Moy· Mar 16, 2012, 08:19 UTC · re: Junio C Hamano · lore

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

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/
Christopher Tiwald· Mar 14, 2012, 14:48 UTC · re: Junio C Hamano · lore

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

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.

Show 44 quoted lines
> 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.

Show 32 quoted lines
>  * 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

Christopher Tiwald· Mar 14, 2012, 14:53 UTC · re: Christopher Tiwald · lore

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

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

Matthieu Moy· Mar 14, 2012, 09:06 UTC · re: Christopher Tiwald · lore

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

Christopher Tiwald <christiwald@gmail.com> writes:
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.
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.

Show 6 quoted lines
> +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.

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

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/
Christopher Tiwald· Mar 14, 2012, 15:52 UTC · re: Matthieu Moy · lore

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

On Wed, Mar 14, 2012 at 10:06:52AM +0100, Matthieu Moy wrote:
Show 7 quoted lines
> 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.

Show 12 quoted lines
> 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

Junio C Hamano· Mar 14, 2012, 17:26 UTC · re: Matthieu Moy · lore

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

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.

Junio C Hamano· Mar 16, 2012, 05:36 UTC · re: Junio C Hamano · lore

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

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(-)
Show changes to 7 files +44 −77

Documentation/config.txt, advice.c, advice.h, builtin/push.c, builtin/send-pack.c, cache.h, transport.c

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
Clemens Buchacher· Mar 16, 2012, 09:10 UTC · re: Junio C Hamano · lore

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

On Thu, Mar 15, 2012 at 10:36:22PM -0700, Junio C Hamano wrote:
Show 20 quoted lines
>
> +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.

Show 15 quoted lines
> -	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.

Junio C Hamano· Mar 16, 2012, 12:03 UTC · re: Clemens Buchacher · lore

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

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.
Christopher Tiwald· Mar 16, 2012, 17:20 UTC · re: Junio C Hamano · lore

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

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

Junio C Hamano· Mar 16, 2012, 18:07 UTC · re: Christopher Tiwald · lore

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

Christopher Tiwald <christiwald@gmail.com> writes:
Show 6 quoted lines
> 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.
Junio C Hamano· Mar 16, 2012, 22:00 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 7 quoted lines
> 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.
Clemens Buchacher· Mar 16, 2012, 21:41 UTC · re: Junio C Hamano · lore

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

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

Junio C Hamano· Mar 16, 2012, 21:53 UTC · re: Clemens Buchacher · lore

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

Clemens Buchacher <drizzd@aon.at> writes:
Show 20 quoted lines
> 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:

Show 6 quoted lines
> +	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.

Clemens Buchacher· Mar 16, 2012, 22:01 UTC · re: Junio C Hamano · lore

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

On Fri, Mar 16, 2012 at 02:53:42PM -0700, Junio C Hamano wrote:
Show 24 quoted lines
> 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...
Junio C Hamano· Mar 16, 2012, 22:10 UTC · re: Clemens Buchacher · lore

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

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.
Zbigniew Jędrzejewski-Szmek· Mar 17, 2012, 17:10 UTC · re: Junio C Hamano · lore

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

On 03/16/2012 06:36 AM, Junio C Hamano wrote:
Show 19 quoted lines
> +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(-)
Show changes to builtin/push.c +11 −14
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 --------
Christopher Tiwald· Mar 17, 2012, 18:46 UTC · re: Zbigniew Jędrzejewski-Szmek · lore

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

On Sat, Mar 17, 2012 at 06:10:35PM +0100, Zbigniew Jędrzejewski-Szmek wrote:
Show 9 quoted lines
>  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

Zbigniew Jędrzejewski-Szmek· Mar 17, 2012, 19:42 UTC · re: Christopher Tiwald · lore

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

On 03/17/2012 07:46 PM, Christopher Tiwald wrote:
Show 24 quoted lines
> 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
Junio C Hamano· Mar 19, 2012, 00:15 UTC · re: Christopher Tiwald · lore

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

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.
Show 9 quoted lines
> 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.

← back to recent threads