[PATCH 0/1] Add stash entry count summary to short status output

[PATCH 0/1] Add stash entry count summary to short status output

[Sonny Michaud]

this patch adds a header in the same style as the one provided by the --branch option when the user supplies both --show-stash and --short.  My attempt at creating a new test is broken[1], and I assume my changes could (and, likely need to) be improved.  Any assistance would be greatly appreciated; I just wanted to get started by hacking first and asking questions later!                         
                                                                               
Thanks,                                                                        
Sonny                                                                          
                                                                               
1. https://travis-ci.org/sonnym/git/builds/266428128

--
From 9e9ffca5c4ed7dda34cad416c3eb68dc94a78b7e Mon Sep 17 00:00:00 2001         
From: Sonny Michaud <michaud.sonny@gmail.com>                                  
Date: Sat, 19 Aug 2017 23:46:15 -0400                                          
Subject: [PATCH 1/1] status: learn to show stash in short output               

This patch extends the functionality of the recently introduced                
--show-stash option to the status command, providing a header similar to       
the one displayed when using the --branch option.                              
---                                                                            
 t/t7508-status.sh |  5 +++++                                                  
 wt-status.c       | 12 ++++++++++++                                           
 2 files changed, 17 insertions(+)                                             

diff --git a/t/t7508-status.sh b/t/t7508-status.sh                             
index 43d19a9b2..734001bc6 100755                                              
--- a/t/t7508-status.sh                                                        
+++ b/t/t7508-status.sh                                                        
@@ -1619,6 +1619,11 @@ test_expect_success 'show stash info with "--show-stash"' '                                                                             
        test_i18ngrep "^Your stash currently has 1 entry$" expected_with_stash 
 '                                                                             
                                                                               
+test_expect_success 'show stash info with "--show-stash" and "--short"' '     
+       git status --show-stash --short >expected_with_stash &&                
+       test_i18ngrep "Stash entries: 2" expected_with_stash                   
+'                                                                             
+                                                                              
 test_expect_success 'no stash info with "--show-stash --no-show-stash"' '     
        git status --show-stash --no-show-stash >expected_without_stash &&     
        test_cmp expected_default expected_without_stash                       
diff --git a/wt-status.c b/wt-status.c                                         
index 77c27c511..651bb01f0 100644                                              
--- a/wt-status.c                                                              
+++ b/wt-status.c                                                              
@@ -1827,6 +1827,15 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)                                                                          
        fputc(s->null_termination ? '\0' : '\n', s->fp);                       
 }                                                                             
                                                                               
+static void wt_shortstatus_print_stash_summary(struct wt_status *s)           
+{                                                                             
+       int stash_count = 0;                                                   
+                                                                              
+       for_each_reflog_ent("refs/stash", stash_count_refs, &stash_count);     
+       if (stash_count > 0)                                                   
+    color_fprintf(s->fp, color(WT_STATUS_HEADER, s), "## Stash entries: %d", stash_count);                                                                    
+}                                                                             
+                                                                              
 static void wt_shortstatus_print(struct wt_status *s)                         
 {                                                                             
        struct string_list_item *it;                                           
@@ -1834,6 +1843,9 @@ static void wt_shortstatus_print(struct wt_status *s)    
        if (s->show_branch)                                                    
                wt_shortstatus_print_tracking(s);                              
                                                                               
+       if (s->show_stash)                                                     
+               wt_shortstatus_print_stash_summary(s);                         
+                                                                              
        for_each_string_list_item(it, &s->change) {                            
                struct wt_status_change_data *d = it->util;                    
                                                                               
--                                                                             
2.14.0

Re: [PATCH 0/1] Add stash entry count summary to short status output

[Sonny Michaud]

I am just bumping this thread; I presume there is something amiss with 
my submissions, so if someone can let me know how to fix any issues, I 
will gladly re-submit the patch.

Thanks!

- Sonny

Re: [PATCH 0/1] Add stash entry count summary to short status output

[Sonny Michaud]

On 08/22/2017 11:25 AM, Sonny Michaud wrote:
> I am just bumping this thread; I presume there is something amiss with 
> my submissions, so if someone can let me know how to fix any issues, I 
> will gladly re-submit the patch.
>
> Thanks!
>
> - Sonny

Bumping again for visibility.

Re: [PATCH 0/1] Add stash entry count summary to short status output

[Junio C Hamano]

Sonny Michaud <michaud.sonny@gmail.com> writes:

> On 08/22/2017 11:25 AM, Sonny Michaud wrote:
>> I am just bumping this thread; I presume there is something amiss
>> with my submissions, so if someone can let me know how to fix any
>> issues, I will gladly re-submit the patch.
>>
>> Thanks!
>>
>> - Sonny
>
> Bumping again for visibility.

It's probably not due to "something amiss" but nobody was interested
in what the patch tries to achieve and had nothing useful to say on
it (hence everybody was quiet).

Some other people may have skipped seeing only [0/1] without [1/1],
as reading only the cover letter without an accompanying patch will
be waste of their time for busy reviewers---by the time [1/1] comes
it is likely they forgot what [0/1] said and they need to go back
and read it again, so they would likely have said "I'll read both
when [1/1] appears on the list", moved on, and then have forgotten
about it.  I am in that camp.

Re: [PATCH 0/1] Add stash entry count summary to short status output

[Junio C Hamano]

Sonny Michaud <michaud.sonny@gmail.com> writes:

> diff --git a/wt-status.c b/wt-status.c                                         
> index 77c27c511..651bb01f0 100644                                              
> --- a/wt-status.c                                                              
> +++ b/wt-status.c                                                              
> @@ -1827,6 +1827,15 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)                                                                          
>         fputc(s->null_termination ? '\0' : '\n', s->fp);                       
>  }                                                                             
>                                                                                
> +static void wt_shortstatus_print_stash_summary(struct wt_status *s)           
> +{                                                                             
> +       int stash_count = 0;                                                   
> +                                                                              
> +       for_each_reflog_ent("refs/stash", stash_count_refs, &stash_count);     

A singleton instance of this in wt_longstatus_print_stash_summary()
thing was OK, but let's not duplicate and spread the badness.  Have
a simple there-liner helper function "static int stash_count(void);"
that does the above and returns the stash_count, and use it from
both places.

> +       if (stash_count > 0)                                                   
> +    color_fprintf(s->fp, color(WT_STATUS_HEADER, s), "## Stash entries: %d", stash_count);                                                                    

That's a funny way to indent (dedent?) a body of an if() statement.

Don't scripts that read this output (I notice that this is also
called by wt_porcelain_print() function) expect that entries that
are led by "##" are about the current branch and its tracking
information?  

This patch would break these script by adding this new line using
the same "##" leader.

Re: [PATCH 0/1] Add stash entry count summary to short status output

[Sonny Michaud]

Junio,

I appreciate you taking the time to look over this patch.  I have some
comments below before re-submitting an updated version.

On 08/24/2017 04:07 PM, Junio C Hamano wrote:
> Sonny Michaud <michaud.sonny@gmail.com> writes:
>
>> diff --git a/wt-status.c b/wt-status.c                                         
>> index 77c27c511..651bb01f0 100644                                              
>> --- a/wt-status.c                                                              
>> +++ b/wt-status.c                                                              
>> @@ -1827,6 +1827,15 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)                                                                          
>>         fputc(s->null_termination ? '\0' : '\n', s->fp);                       
>>  }                                                                             
>>                                                                                
>> +static void wt_shortstatus_print_stash_summary(struct wt_status *s)           
>> +{                                                                             
>> +       int stash_count = 0;                                                   
>> +                                                                              
>> +       for_each_reflog_ent("refs/stash", stash_count_refs, &stash_count);     
> A singleton instance of this in wt_longstatus_print_stash_summary()
> thing was OK, but let's not duplicate and spread the badness.  Have
> a simple there-liner helper function "static int stash_count(void);"
> that does the above and returns the stash_count, and use it from
> both places.
That is reasonable.
>> +       if (stash_count > 0)                                                   
>> +    color_fprintf(s->fp, color(WT_STATUS_HEADER, s), "## Stash entries: %d", stash_count);                                                                    
> That's a funny way to indent (dedent?) a body of an if() statement.
My bad, I can clean that up!
>
> Don't scripts that read this output (I notice that this is also
> called by wt_porcelain_print() function) expect that entries that
> are led by "##" are about the current branch and its tracking
> information?  
>
> This patch would break these script by adding this new line using
> the same "##" leader.
>
I did not consider porcelain output;  it looks like the headers are
stripped from it, though, so this might not be an issue.  Do you think
this is a worthwhile path to continue on?

Thanks,
Sonny

Re: [PATCH 0/1] Add stash entry count summary to short status output

[Jonathan Nieder]

Hi Sonny,

Sonny Michaud wrote:

> Any assistance would be greatly appreciated; I just wanted to get
> started by hacking first and asking questions later!                         

Welcome to the Git project, and sorry for the silence before.  Getting
to see what people are working on is one of the nice things about
being subscribed to this list. :)

> Subject: [PATCH 0/1] Add stash entry count summary to short status output

As Junio mentioned, this subject line may discourage casual readers.
In a one-patch series like this one, there's no need for a cover
letter: the patch's commit message should speak for itself anyway.

See the DISCUSSION section of "git help format-patch" for more details
on sending a patch for review.

You can put [RFC/PATCH] to make it clear that a patch is still a work
in progress and you're looking for early comments on it.

> this patch adds a header in the same style as the one provided by
> the --branch option when the user supplies both --show-stash and
> --short.

For next time, this information belongs in the commit message.

[...]
> This patch extends the functionality of the recently introduced
> --show-stash option to the status command, providing a header similar to
> the one displayed when using the --branch option.

Can you include an example of output in the commit message?  Also,
can you say more about the workflow this fits in?  What information
does a person expect in the output, and how does this change help a
person do what they are trying to do?

[...]
> --- a/t/t7508-status.sh
> +++ b/t/t7508-status.sh
> @@ -1619,6 +1619,11 @@ test_expect_success 'show stash info with "--show-stash"' '
>         test_i18ngrep "^Your stash currently has 1 entry$" expected_with_stash
>  '
>
> +test_expect_success 'show stash info with "--show-stash" and "--short"' '
> +       git status --show-stash --short >expected_with_stash &&
> +       test_i18ngrep "Stash entries: 2" expected_with_stash

Interesting.  The idea makes sense.

I have the same questions as Junio about how this affects --porcelain
output.

Also, could this patch also include some documentation in
Documentation/git-status.txt about the new behavior?

[...]
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1827,6 +1827,15 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
>         fputc(s->null_termination ? '\0' : '\n', s->fp);
>  }
>
> +static void wt_shortstatus_print_stash_summary(struct wt_status *s)
> +{
> +       int stash_count = 0;
> +
> +       for_each_reflog_ent("refs/stash", stash_count_refs, &stash_count);
> +       if (stash_count > 0)
> +    color_fprintf(s->fp, color(WT_STATUS_HEADER, s), "## Stash entries: %d", stash_count);

See the "For C programs" section of Documentation/SubmittingPatches
for some formatting hints.

In the "pu" branch, there is some experimental support for helping
with that.  If you merge in the bw/git-clang-format branch from there,
you can test your patch for style errors (or for errors in that support
:)) by running "make style".

Thanks and hope that helps,
Jonathan