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

[Christopher Tiwald <christiwald@gmail.com>]

On Mon, Mar 19, 2012 at 11:58:02AM -0700, Junio C Hamano wrote:
> 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. Give better resolution advice in three push scenarios:
> >
> > 1) If you push a non-fast-forward update to your current branch, you
> > should merge remote changes with 'git pull' before pushing again.
> 
> I have always found "update *to* your current branch" very strange
> phrasing (the earlier one said "to HEAD", but it amounts to the same
> thing).  You do not push *to* your branch.  You push your branch to
> somewhere else (namely, remote).  I would understand if it said "If your
> push of your current branch triggers a non-ff error, ...", though.

Ah. Yeah. I can see the problem with my phrasing now. How about something
like the following?

"If you push your current branch and it triggers a non-fast-forward
error, you should merge remote changes with 'git pull' before pushing
again."

> She never gets a chance to see the other checkout-pull-push message, does
> she?
> 
> >  There is one aspect about this patch about which I'm unsure: What to
> >  do with users who've set "advice.pushNonFastForward = false" already.
> 
> The change in this patch is merely clarifying what pushNonFastForward
> advise has already taught them ("Non-ff was rejected; the manual will tell
> you what you wanted to do") by dividing them into three categories and
> giving different advices to these categories.  As the user says he
> understood what he is doing, I think squelching all of them is a sane
> choice.

How about the something like the following fixup? This introduces two
changes to v2:

- It breaks the new advice into three config variables. Users
  who might benefit from the advice can't accidentally shut a message
  off before being confronted with the situation it's designed to
  advise.
- It leaves pushNonFastForward in place, and if a user sets
  'advice.pushNonFastForward = false', it'll disable all three pieces
  of advice.

--
Christopher Tiwald
--- 8< ---
Signed-off-by: Christopher Tiwald <christiwald@gmail.com>
---
 Documentation/config.txt |   19 +++++++++++++++----
 advice.c                 |    8 ++++++--
 advice.h                 |    4 +++-
 builtin/push.c           |    6 +++---
 4 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index a2329b5..fb386ab 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -137,13 +137,24 @@ advice.*::
 	can tell Git that you do not need help by setting these to 'false':
 +
 --
+	pushNonFastForward::
+		Set this variable to 'false' if you want to disable
+		'pushNonFFCurrent', 'pushNonFFDefault', and
+		'pushNonFFMatching' simultaneously.
 	pushNonFFCurrent::
 		Advice shown when linkgit:git-push[1] fails due to a
 		non-fast-forward update to the current branch.
-	pushNonFFOther::
-		Advice shown when linkgit:git-push[1] fails due to a
-		non-fast-forward update to a branch other than the
-		current one.
+	pushNonFFDefault::
+		Advice to set 'push.default' to 'upstream' or 'current'
+		when you ran linkgit:git-push[1] and pushed 'matching
+		refs' by default (i.e. you did not provide an explicit
+		refspec, and no 'push.default' configuration was set)
+		and it resulted in a non-fast-forward error.
+	pushNonFFMatching::
+		Advice shown when you ran linkgit:git-push[1] and pushed
+		'matching refs' explicitly (i.e. you used ':', or
+		specified a refspec that isn't your current branch) and
+		it resulted in a non-fast-forward error.
 	statusHints::
 		Directions on how to stage/unstage/add shown in the
 		output of linkgit:git-status[1] and the template shown
diff --git a/advice.c b/advice.c
index ee62e1b..a492eea 100644
--- a/advice.c
+++ b/advice.c
@@ -1,7 +1,9 @@
 #include "cache.h"
 
+int advice_push_nonfastforward = 1;
 int advice_push_non_ff_current = 1;
-int advice_push_non_ff_other = 1;
+int advice_push_non_ff_default = 1;
+int advice_push_non_ff_matching = 1;
 int advice_status_hints = 1;
 int advice_commit_before_merge = 1;
 int advice_resolve_conflict = 1;
@@ -12,8 +14,10 @@ static struct {
 	const char *name;
 	int *preference;
 } advice_config[] = {
+	{ "pushnonfastforward", &advice_push_nonfastforward },
 	{ "pushnonffcurrent", &advice_push_non_ff_current },
-	{ "pushnonffother", &advice_push_non_ff_other },
+	{ "pushnonffdefault", &advice_push_non_ff_default },
+	{ "pushnonffmatching", &advice_push_non_ff_matching },
 	{ "statushints", &advice_status_hints },
 	{ "commitbeforemerge", &advice_commit_before_merge },
 	{ "resolveconflict", &advice_resolve_conflict },
diff --git a/advice.h b/advice.h
index 98c675e..f3cdbbf 100644
--- a/advice.h
+++ b/advice.h
@@ -3,8 +3,10 @@
 
 #include "git-compat-util.h"
 
+extern int advice_push_nonfastforward;
 extern int advice_push_non_ff_current;
-extern int advice_push_non_ff_other;
+extern int advice_push_non_ff_default;
+extern int advice_push_non_ff_matching;
 extern int advice_status_hints;
 extern int advice_commit_before_merge;
 extern int advice_resolve_conflict;
diff --git a/builtin/push.c b/builtin/push.c
index 3de2737..a0ffbb3 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -138,21 +138,21 @@ static const char message_advice_checkout_pull_push[] =
 
 static void advise_pull_before_push(void)
 {
-	if (!advice_push_non_ff_current)
+	if (!advice_push_non_ff_current | !advice_push_nonfastforward)
 		return;
 	advise(_(message_advice_pull_before_push));
 }
 
 static void advise_use_upstream(void)
 {
-	if (!advice_push_non_ff_other)
+	if (!advice_push_non_ff_default | !advice_push_nonfastforward)
 		return;
 	advise(_(message_advice_use_upstream));
 }
 
 static void advise_checkout_pull_push(void)
 {
-	if (!advice_push_non_ff_other)
+	if (!advice_push_non_ff_matching | !advice_push_nonfastforward)
 		return;
 	advise(_(message_advice_checkout_pull_push));
 }
-- 
1.7.10.rc1.23.g2a051.dirty