threads / rfc / 23550

[RFC/RFD] wt-status: take advice.statusHints seriously

Subject: [RFC/RFD] wt-status: take advice.statusHints seriously

## tl;dr

9 messages between Apr 21, 2010 and Apr 26, 2010.

replies: 8people: 5as markdown or json

Michael J Gruber· Apr 21, 2010, 14:47 UTC · lore

Currently, status gives a lot of hints even when advice.statusHints is false. Change this so that all hints depend on the config varaible.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 wt-status.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/wt-status.c b/wt-status.c
index 8ca59a2..38076ee 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -625,7 +625,7 @@ void wt_status_print(struct wt_status *s)
 	if (s->show_untracked_files)
 		wt_status_print_untracked(s);
 	else if (s->commitable)
-		 fprintf(s->fp, "# Untracked files not listed (use -u option to show untracked files)\n");
+		 fprintf(s->fp, "# Untracked files not listed%s\n", advice_status_hints ? " (use -u option to show untracked files)" : "");
 
 	if (s->verbose)
 		wt_status_print_verbose(s);
@@ -635,15 +635,15 @@ void wt_status_print(struct wt_status *s)
 		else if (s->nowarn)
 			; /* nothing */
 		else if (s->workdir_dirty)
-			printf("no changes added to commit (use \"git add\" and/or \"git commit -a\")\n");
+			printf("no changes added to commit%s\n", advice_status_hints ? " (use \"git add\" and/or \"git commit -a\")" : "");
 		else if (s->untracked.nr)
-			printf("nothing added to commit but untracked files present (use \"git add\" to track)\n");
+			printf("nothing added to commit but untracked files present%s\n", advice_status_hints ? " (use \"git add\" to track)" : "");
 		else if (s->is_initial)
-			printf("nothing to commit (create/copy files and use \"git add\" to track)\n");
+			printf("nothing to commit%s\n", advice_status_hints ? " (create/copy files and use \"git add\" to track)" : "");
 		else if (!s->show_untracked_files)
-			printf("nothing to commit (use -u to show untracked files)\n");
+			printf("nothing to commit%s\n", advice_status_hints ? " (use -u to show untracked files)" : "");
 		else
-			printf("nothing to commit (working directory clean)\n");
+			printf("nothing to commit%s\n", advice_status_hints ? " (working directory clean)" : "");
 	}
 }
 
-- 
1.7.1.rc1.248.gcefbb
Junio C Hamano· Apr 22, 2010, 07:48 UTC · re: Michael J Gruber · lore

Re: [RFC/RFD] wt-status: take advice.statusHints seriously

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 18 quoted lines
> Currently, status gives a lot of hints even when advice.statusHints is
> false. Change this so that all hints depend on the config varaible.
>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  wt-status.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index 8ca59a2..38076ee 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -625,7 +625,7 @@ void wt_status_print(struct wt_status *s)
>  	if (s->show_untracked_files)
>  		wt_status_print_untracked(s);
>  	else if (s->commitable)
> -		 fprintf(s->fp, "# Untracked files not listed (use -u option to show untracked files)\n");
> +		 fprintf(s->fp, "# Untracked files not listed%s\n", advice_status_hints ? " (use -u option to show untracked files)" : "");

This is a good change, but because you are losing the grep-ability with this patch (i.e. "grep -e 'not listed (use -u'" no longer works), I'd prefer to see this long line split into:

		 fprintf(s->fp, "# Untracked files not listed%s\n",
			advice_status_hints
			? " (use -u option to show untracked files)" : "");
Likewise for all the other changes.                        
Thanks.
Michael J Gruber· Apr 22, 2010, 20:30 UTC · re: Junio C Hamano · lore

[PATCH 1/2] t7508: test advice.statusHints

edf563f (status: make "how to stage" messages optional, 2009-09-09) introduced advice.statusHints without tests. Add a few tests to describe and test the status quo.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t7508-status.sh |   45 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 45 insertions(+), 0 deletions(-)
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index 556d0fa..7409a06 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -69,6 +69,34 @@ test_expect_success 'status (2)' '
 '
 
 cat >expect <<\EOF
+# On branch master
+# Changes to be committed:
+#	new file:   dir2/added
+#
+# Changed but not updated:
+#	modified:   dir1/modified
+#
+# Untracked files:
+#	dir1/untracked
+#	dir2/modified
+#	dir2/untracked
+#	expect
+#	output
+#	untracked
+EOF
+
+git config advice.statusHints false
+
+test_expect_success 'status (advice.statusHints false)' '
+
+	git status >output &&
+	test_cmp expect output
+
+'
+
+git config --unset advice.statusHints
+
+cat >expect <<\EOF
  M dir1/modified
 A  dir2/added
 ?? dir1/untracked
@@ -115,6 +143,23 @@ test_expect_success 'status (status.showUntrackedFiles no)' '
 	test_cmp expect output
 '
 
+cat >expect <<EOF
+# On branch master
+# Changes to be committed:
+#	new file:   dir2/added
+#
+# Changed but not updated:
+#	modified:   dir1/modified
+#
+# Untracked files not listed (use -u option to show untracked files)
+EOF
+git config advice.statusHints false
+test_expect_success 'status -uno (advice.statusHints false)' '
+	git status -uno >output &&
+	test_cmp expect output
+'
+git config --unset advice.statusHints
+
 cat >expect << EOF
  M dir1/modified
 A  dir2/added
-- 
1.7.1.rc1.248.gcefbb
Michael J Gruber· Apr 22, 2010, 20:30 UTC · re: Junio C Hamano · lore

[PATCH 2/2] wt-status: take advice.statusHints seriously

Currently, status gives a lot of hints even when advice.statusHints is false. Change this so that all hints depend on the config variable.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 t/t7508-status.sh |    2 +-
 wt-status.c       |   21 +++++++++++++++------
 2 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index 7409a06..1301ec8 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -151,7 +151,7 @@ cat >expect <<EOF
 # Changed but not updated:
 #	modified:   dir1/modified
 #
-# Untracked files not listed (use -u option to show untracked files)
+# Untracked files not listed
 EOF
 git config advice.statusHints false
 test_expect_success 'status -uno (advice.statusHints false)' '
diff --git a/wt-status.c b/wt-status.c
index 8ca59a2..d44486c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -625,7 +625,9 @@ void wt_status_print(struct wt_status *s)
 	if (s->show_untracked_files)
 		wt_status_print_untracked(s);
 	else if (s->commitable)
-		 fprintf(s->fp, "# Untracked files not listed (use -u option to show untracked files)\n");
+		fprintf(s->fp, "# Untracked files not listed%s\n",
+			advice_status_hints
+			? " (use -u option to show untracked files)" : "");
 
 	if (s->verbose)
 		wt_status_print_verbose(s);
@@ -635,15 +637,22 @@ void wt_status_print(struct wt_status *s)
 		else if (s->nowarn)
 			; /* nothing */
 		else if (s->workdir_dirty)
-			printf("no changes added to commit (use \"git add\" and/or \"git commit -a\")\n");
+			printf("no changes added to commit%s\n",
+				advice_status_hints
+				? " (use \"git add\" and/or \"git commit -a\")" : "");
 		else if (s->untracked.nr)
-			printf("nothing added to commit but untracked files present (use \"git add\" to track)\n");
+			printf("nothing added to commit but untracked files present%s\n",
+				advice_status_hints
+				? " (use \"git add\" to track)" : "");
 		else if (s->is_initial)
-			printf("nothing to commit (create/copy files and use \"git add\" to track)\n");
+			printf("nothing to commit%s\n", advice_status_hints
+				? " (create/copy files and use \"git add\" to track)" : "");
 		else if (!s->show_untracked_files)
-			printf("nothing to commit (use -u to show untracked files)\n");
+			printf("nothing to commit%s\n", advice_status_hints
+				? " (use -u to show untracked files)" : "");
 		else
-			printf("nothing to commit (working directory clean)\n");
+			printf("nothing to commit%s\n", advice_status_hints
+				? " (working directory clean)" : "");
 	}
 }
 
-- 
1.7.1.rc1.248.gcefbb
Tay Ray Chuan· Apr 23, 2010, 04:15 UTC · re: Michael J Gruber · lore

Re: [PATCH 2/2] wt-status: take advice.statusHints seriously

Hi,

On Fri, Apr 23, 2010 at 4:30 AM, Michael J Gruber <git@drmicha.warpmail.net> wrote:

Show 5 quoted lines
>                else if (s->untracked.nr)
> -                       printf("nothing added to commit but untracked files present (use \"git add\" to track)\n");
> +                       printf("nothing added to commit but untracked files present%s\n",
> +                               advice_status_hints
> +                               ? " (use \"git add\" to track)" : "");

while we're at it, perhaps we could put the hints on its own line, with a "hint: " prefix:

  nothing added to commit but untracked files present
  hint: use "git add" to track

This way, we give future git usability hackers the space to elaborate further on why a certain flag or command was recommended.

-- 
Cheers,
Ray Chuan
Michael J Gruber· Apr 23, 2010, 08:09 UTC · re: Tay Ray Chuan · lore

Re: [PATCH 2/2] wt-status: take advice.statusHints seriously

Tay Ray Chuan venit, vidit, dixit 23.04.2010 06:15:
Show 11 quoted lines
> Hi,
> 
> On Fri, Apr 23, 2010 at 4:30 AM, Michael J Gruber
> <git@drmicha.warpmail.net> wrote:
>>                else if (s->untracked.nr)
>> -                       printf("nothing added to commit but untracked files present (use \"git add\" to track)\n");
>> +                       printf("nothing added to commit but untracked files present%s\n",
>> +                               advice_status_hints
>> +                               ? " (use \"git add\" to track)" : "");
> 
> while we're at it, perhaps we could put the hints on its own line,

I don't think I'm the only one to turn sour at every encounter with the phrase "while we're at it". I did fix one extraneous space in code "while I was at it", yes, but:

Show 8 quoted lines
> with a "hint: " prefix:
> 
>   nothing added to commit but untracked files present
>   hint: use "git add" to track
> 
> This way, we give future git usability hackers the space to elaborate
> further on why a certain flag or command was recommended.
> 

I'm suggesting a change in (output) behaviour (hint vs. no hint), which one may even consider to be a bug fix in terms of matching the obvious expections related to advice.statusHints false.

You're suggesting a different presentation of the output. In fact, I noticed inconsistent capitalisation in the hints which should be adjusted, maybe together with the presentation.

But I deem both changes (overlapping though) unrelated.

Cheers, Michael

Jakub Narebski· Apr 23, 2010, 18:31 UTC · re: Michael J Gruber · lore

Re: [PATCH 2/2] wt-status: take advice.statusHints seriously

Michael J Gruber <git@drmicha.warpmail.net> writes:
Show 15 quoted lines
> Tay Ray Chuan venit, vidit, dixit 23.04.2010 06:15:
>> 
>> On Fri, Apr 23, 2010 at 4:30 AM, Michael J Gruber
>> <git@drmicha.warpmail.net> wrote:
>>>                else if (s->untracked.nr)
>>> -                       printf("nothing added to commit but untracked files present (use \"git add\" to track)\n");
>>> +                       printf("nothing added to commit but untracked files present%s\n",
>>> +                               advice_status_hints
>>> +                               ? " (use \"git add\" to track)" : "");
>> 
>> while we're at it, perhaps we could put the hints on its own line,
> 
> I don't think I'm the only one to turn sour at every encounter with the
> phrase "while we're at it". I did fix one extraneous space in code
> "while I was at it", yes, but:
I agre that it might be a good idea, but it is not something to do
"while we're at it".
 
Tay Ray Chuan probably meant that he "while at it" (while reviewing
this patch) had this idea...
Show 18 quoted lines
>> with a "hint: " prefix:
>> 
>>   nothing added to commit but untracked files present
>>   hint: use "git add" to track
>> 
>> This way, we give future git usability hackers the space to elaborate
>> further on why a certain flag or command was recommended.
>> 
> 
> I'm suggesting a change in (output) behaviour (hint vs. no hint), which
> one may even consider to be a bug fix in terms of matching the obvious
> expections related to advice.statusHints false.
> 
> You're suggesting a different presentation of the output. In fact, I
> noticed inconsistent capitalisation in the hints which should be
> adjusted, maybe together with the presentation.
> 
> But I deem both changes (overlapping though) unrelated.

Perhaps git should introduce helper commands, similar to 'die', 'warn' and 'error', but for hints and advices, e.g. hint(bool, "<string>") which would print advice on its own line (like 'die' and 'warn') with appropriate prefix if advices, and e.g. 'advice', which would return either empty string or string with advice (a bit like 'error').

-- 
Jakub Narebski
Poland
ShadeHawk on #git
Jeff King· Apr 26, 2010, 19:10 UTC · re: Michael J Gruber · lore

Re: [PATCH 2/2] wt-status: take advice.statusHints seriously

On Fri, Apr 23, 2010 at 10:09:52AM +0200, Michael J Gruber wrote:
Show 16 quoted lines
> Tay Ray Chuan venit, vidit, dixit 23.04.2010 06:15:
> > Hi,
> > 
> > On Fri, Apr 23, 2010 at 4:30 AM, Michael J Gruber
> > <git@drmicha.warpmail.net> wrote:
> >>                else if (s->untracked.nr)
> >> -                       printf("nothing added to commit but untracked files present (use \"git add\" to track)\n");
> >> +                       printf("nothing added to commit but untracked files present%s\n",
> >> +                               advice_status_hints
> >> +                               ? " (use \"git add\" to track)" : "");
> > 
> > while we're at it, perhaps we could put the hints on its own line,
> 
> I don't think I'm the only one to turn sour at every encounter with the
> phrase "while we're at it". I did fix one extraneous space in code
> "while I was at it", yes, but:

Just my 2 cents, but I find "while we're at it" comments are taken much better when accompanied by a patch (either squashable or to be applied on top). :)

-Peff
Jeff King· Apr 26, 2010, 19:10 UTC · re: Michael J Gruber · lore

Re: [PATCH 2/2] wt-status: take advice.statusHints seriously

On Thu, Apr 22, 2010 at 10:30:20PM +0200, Michael J Gruber wrote:
> Currently, status gives a lot of hints even when advice.statusHints is
> false. Change this so that all hints depend on the config variable.

I didn't bother with them because they aren't actually wasting screen real estate, unlike the other hints. But I don't really care either way, so it's fine by me.

Though I wonder if you should simply suppress the line entirely. It is totally redundant with the information presented above. But like I said, I don't care too much.

-Peff

← back to recent threads