threads / patch / 32264

patchstatus: respect advice.statusHints for ahead/behind advice

Subject: [PATCH] status: respect advice.statusHints for ahead/behind advice

## tl;dr

5 messages between Dec 3, 2012 and Dec 4, 2012. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Jeff King· Dec 3, 2012, 06:16 UTC · lore

If the user has unset advice.statusHints, we already suppress the "use git reset to..." hints in each stanza. The new "use git push to publish..." hint is the same type of hint. Let's respect statusHints for it, rather than making the user set yet another advice flag.

Signed-off-by: Jeff King <peff@peff.net>
---
On top of mm/status-push-pull-advise.

I left "git checkout" alone, though I'd also like to turn it off there, too. Should it get a separate advice option there? Should it simply respect statusHints (it seems odd because I know that "status" there means "git status", not "hints about the status of your repo")?

 builtin/checkout.c |  2 +-
 remote.c           | 17 ++++++++++-------
 remote.h           |  2 +-
 wt-status.c        |  2 +-
 4 files changed, 13 insertions(+), 10 deletions(-)
Show changes to 4 files +13 −10

builtin/checkout.c, remote.c, remote.h, wt-status.c

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 781295b..28146d1 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -544,7 +544,7 @@ static void report_tracking(struct branch_info *new)
 	struct strbuf sb = STRBUF_INIT;
 	struct branch *branch = branch_get(new->name);
 
-	if (!format_tracking_info(branch, &sb))
+	if (!format_tracking_info(branch, &sb, 1))
 		return;
 	fputs(sb.buf, stdout);
 	strbuf_release(&sb);
diff --git a/remote.c b/remote.c
index 9c19689..176a777 100644
--- a/remote.c
+++ b/remote.c
@@ -1617,7 +1617,7 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)
 /*
  * Return true when there is anything to report, otherwise false.
  */
-int format_tracking_info(struct branch *branch, struct strbuf *sb)
+int format_tracking_info(struct branch *branch, struct strbuf *sb, int advice)
 {
 	int num_ours, num_theirs;
 	const char *base;
@@ -1633,8 +1633,9 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
 			   "Your branch is ahead of '%s' by %d commits.\n",
 			   num_ours),
 			base, num_ours);
-		strbuf_addf(sb,
-			_("  (use \"git push\" to publish your local commits)\n"));
+		if (advice)
+			strbuf_addf(sb,
+				_("  (use \"git push\" to publish your local commits)\n"));
 	} else if (!num_ours) {
 		strbuf_addf(sb,
 			Q_("Your branch is behind '%s' by %d commit, "
@@ -1643,8 +1644,9 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
 			       "and can be fast-forwarded.\n",
 			   num_theirs),
 			base, num_theirs);
-		strbuf_addf(sb,
-			_("  (use \"git pull\" to update your local branch)\n"));
+		if (advice)
+			strbuf_addf(sb,
+				_("  (use \"git pull\" to update your local branch)\n"));
 	} else {
 		strbuf_addf(sb,
 			Q_("Your branch and '%s' have diverged,\n"
@@ -1655,8 +1657,9 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
 			       "respectively.\n",
 			   num_theirs),
 			base, num_ours, num_theirs);
-		strbuf_addf(sb,
-			_("  (use \"git pull\" to merge the remote branch into yours)\n"));
+		if (advice)
+			strbuf_addf(sb,
+				_("  (use \"git pull\" to merge the remote branch into yours)\n"));
 	}
 	return 1;
 }
diff --git a/remote.h b/remote.h
index 251d8fd..ac504a5 100644
--- a/remote.h
+++ b/remote.h
@@ -153,7 +153,7 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs);
 
 /* Reporting of tracking info */
 int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs);
-int format_tracking_info(struct branch *branch, struct strbuf *sb);
+int format_tracking_info(struct branch *branch, struct strbuf *sb, int advice);
 
 struct ref *get_local_heads(void);
 /*
diff --git a/wt-status.c b/wt-status.c
index 2a9658b..b48c8cf 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -755,7 +755,7 @@ static void wt_status_print_tracking(struct wt_status *s)
 	if (prefixcmp(s->branch, "refs/heads/"))
 		return;
 	branch = branch_get(s->branch + 11);
-	if (!format_tracking_info(branch, &sb))
+	if (!format_tracking_info(branch, &sb, advice_status_hints))
 		return;
 
 	for (cp = sb.buf; (ep = strchr(cp, '\n')) != NULL; cp = ep + 1)
-- 
1.7.12.4.42.ge2b5b43
Matthieu Moy· Dec 3, 2012, 08:49 UTC · re: Jeff King · lore

Re: [PATCH] status: respect advice.statusHints for ahead/behind advice

Jeff King <peff@peff.net> writes:
Show 5 quoted lines
> If the user has unset advice.statusHints, we already
> suppress the "use git reset to..." hints in each stanza. The
> new "use git push to publish..." hint is the same type of
> hint. Let's respect statusHints for it, rather than making
> the user set yet another advice flag.
Sorry, I should have made this in the patch.
With or without my suggestion below,
Acknowledged-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> I left "git checkout" alone, though I'd also like to turn it off there,
> too. Should it get a separate advice option there? Should it simply
> respect statusHints (it seems odd because I know that "status" there
> means "git status", not "hints about the status of your repo")?

I'd respect the same statusHint indeed. It's the same message, so I think the same category of users will want to disable it. The name "statusHint" may not be the ideal one, but if you see the messages in "git checkout" as an excerpt of "git status", it actually makes sense.

As a nice side effect, it also makes the code/patch simpler (no need to add the "advice" argument).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Junio C Hamano· Dec 3, 2012, 17:11 UTC · re: Jeff King · lore

Re: [PATCH] status: respect advice.statusHints for ahead/behind advice

Jeff King <peff@peff.net> writes:
Show 14 quoted lines
> If the user has unset advice.statusHints, we already
> suppress the "use git reset to..." hints in each stanza. The
> new "use git push to publish..." hint is the same type of
> hint. Let's respect statusHints for it, rather than making
> the user set yet another advice flag.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> On top of mm/status-push-pull-advise.
>
> I left "git checkout" alone, though I'd also like to turn it off there,
> too. Should it get a separate advice option there? Should it simply
> respect statusHints (it seems odd because I know that "status" there
> means "git status", not "hints about the status of your repo")?

I agree that we should have a toggle to turn it off and I think it is OK to reuse the same "hints about the status" option for this purpose. It is not like there is a released version that already gives the advice (possibly with some other option to turn it off) and you are changing the behaviour of "checkout" by suddenly making it honor statusHints advice.

So let's do a lot simpler patch instead.
-- >8 --
From: Jeff King <peff@peff.net>
Date: Mon, 3 Dec 2012 01:16:57 -0500

If the user has unset advice.statusHints, we already suppress the "use git reset to..." hints in each stanza. The new "use git push to publish..." hint is the same type of hint. Let's respect statusHints for it, rather than making the user set yet another advice flag.

Signed-off-by: Jeff King <peff@peff.net>
Acked-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 remote.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)
Show changes to remote.c +9 −6
diff --git a/remote.c b/remote.c
index 9c19689..18dc8ec 100644
--- a/remote.c
+++ b/remote.c
@@ -1633,8 +1633,9 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
 			   "Your branch is ahead of '%s' by %d commits.\n",
 			   num_ours),
 			base, num_ours);
-		strbuf_addf(sb,
-			_("  (use \"git push\" to publish your local commits)\n"));
+		if (advice_status_hints)
+			strbuf_addf(sb,
+				_("  (use \"git push\" to publish your local commits)\n"));
 	} else if (!num_ours) {
 		strbuf_addf(sb,
 			Q_("Your branch is behind '%s' by %d commit, "
@@ -1643,8 +1644,9 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
 			       "and can be fast-forwarded.\n",
 			   num_theirs),
 			base, num_theirs);
-		strbuf_addf(sb,
-			_("  (use \"git pull\" to update your local branch)\n"));
+		if (advice_status_hints)
+			strbuf_addf(sb,
+				_("  (use \"git pull\" to update your local branch)\n"));
 	} else {
 		strbuf_addf(sb,
 			Q_("Your branch and '%s' have diverged,\n"
@@ -1655,8 +1657,9 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
 			       "respectively.\n",
 			   num_theirs),
 			base, num_ours, num_theirs);
-		strbuf_addf(sb,
-			_("  (use \"git pull\" to merge the remote branch into yours)\n"));
+		if (advice_status_hints)
+			strbuf_addf(sb,
+				_("  (use \"git pull\" to merge the remote branch into yours)\n"));
 	}
 	return 1;
 }
-- 
1.8.0.1.420.g52a5207
Jeff King· Dec 3, 2012, 18:32 UTC · re: Junio C Hamano · lore

Re: [PATCH] status: respect advice.statusHints for ahead/behind advice

On Mon, Dec 03, 2012 at 09:11:32AM -0800, Junio C Hamano wrote:
Show 17 quoted lines
> > On top of mm/status-push-pull-advise.
> >
> > I left "git checkout" alone, though I'd also like to turn it off there,
> > too. Should it get a separate advice option there? Should it simply
> > respect statusHints (it seems odd because I know that "status" there
> > means "git status", not "hints about the status of your repo")?
> 
> I agree that we should have a toggle to turn it off and I think it
> is OK to reuse the same "hints about the status" option for this
> purpose.  It is not like there is a released version that already
> gives the advice (possibly with some other option to turn it off)
> and you are changing the behaviour of "checkout" by suddenly making
> it honor statusHints advice.
> 
> So let's do a lot simpler patch instead.
> 
> -- >8 --
Perfect, thanks.
-Peff
Matthieu Moy· Dec 4, 2012, 09:17 UTC · re: Junio C Hamano · lore

Re: [PATCH] status: respect advice.statusHints for ahead/behind advice

Junio C Hamano <gitster@pobox.com> writes:
Show 11 quoted lines
> So let's do a lot simpler patch instead.
>
> -- >8 --
> From: Jeff King <peff@peff.net>
> Date: Mon, 3 Dec 2012 01:16:57 -0500
>
> If the user has unset advice.statusHints, we already
> suppress the "use git reset to..." hints in each stanza. The
> new "use git push to publish..." hint is the same type of
> hint. Let's respect statusHints for it, rather than making
> the user set yet another advice flag.
You may want to squash this on top:
-- >8 --
>From 87a438bd06dd689bd37949e762690ab5dbfc5ff8 Mon Sep 17 00:00:00 2001
From: Matthieu Moy <Matthieu.Moy@imag.fr>
Date: Tue, 4 Dec 2012 10:15:03 +0100
Subject: [PATCH] document that statusHints affects git checkout
---
 Documentation/config.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Show changes to Documentation/config.txt +3 −2
diff --git a/Documentation/config.txt b/Documentation/config.txt
index e70216d..bf8f911 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -160,9 +160,10 @@ advice.*::
                it resulted in a non-fast-forward error.
        statusHints::
                Show directions on how to proceed from the current
-               state in the output of linkgit:git-status[1] and in
+               state in the output of linkgit:git-status[1], in
                the template shown when writing commit messages in
-               linkgit:git-commit[1].
+               linkgit:git-commit[1], and in the help message shown
+               by linkgit:git-checkout[1] when switching branch.
        commitBeforeMerge::
                Advice shown when linkgit:git-merge[1] refuses to
                merge to avoid overwriting local changes.
-- 
1.8.0.319.g8abfee4



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

← back to recent threads