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

6 messages, 4 authors, 2016-06-15 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:53:18

Matthieu Moy [off-list ref] 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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:53:19

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

From: Clemens Buchacher <hidden>
Date: 2016-06-15 22:53:19

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.

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

From: Zbigniew Jędrzejewski-Szmek <hidden>
Date: 2016-06-15 22:53:19

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

From: Christopher Tiwald <hidden>
Date: 2016-06-15 22:53:19

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

From: Zbigniew Jędrzejewski-Szmek <hidden>
Date: 2016-06-15 22:53:19

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:
quoted
  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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help