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

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

[Michael J Gruber]

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

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

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

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

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

[Michael J Gruber]

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

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

[Michael J Gruber]

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

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

[Tay Ray Chuan]

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,
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

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

[Michael J Gruber]

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:

> 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

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

[Jakub Narebski]

Michael J Gruber <git@drmicha.warpmail.net> writes:
> 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...

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

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

[Jeff King]

On Fri, Apr 23, 2010 at 10:09:52AM +0200, Michael J Gruber wrote:

> 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

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

[Jeff King]

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