threads / patch / 46630

patchAdd stash entry count summary to short status output

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

## tl;dr

7 messages between Aug 20, 2017 and Sep 1, 2017. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Sonny Michaud· Aug 20, 2017, 04:53 UTC · lore
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(+)
Show changes to 2 files +17 −1

t/t7508-status.sh, wt-status.c

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
Sonny Michaud· Aug 22, 2017, 15:25 UTC · re: Sonny Michaud · lore

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

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
Sonny Michaud· Aug 24, 2017, 15:29 UTC · re: Sonny Michaud · lore

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

On 08/22/2017 11:25 AM, Sonny Michaud wrote:
Show 7 quoted lines
> 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.
Junio C Hamano· Aug 24, 2017, 19:29 UTC · re: Sonny Michaud · lore

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

Sonny Michaud <michaud.sonny@gmail.com> writes:
Show 10 quoted lines
> 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.

Junio C Hamano· Aug 24, 2017, 20:07 UTC · re: Sonny Michaud · lore

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

Sonny Michaud <michaud.sonny@gmail.com> writes:
Show 13 quoted lines
> 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.

Sonny Michaud· Aug 31, 2017, 21:49 UTC · re: Junio C Hamano · lore

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

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:
Show 20 quoted lines
> 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!
Show 9 quoted lines
>
> 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

Jonathan Nieder· Sep 1, 2017, 03:11 UTC · re: Sonny Michaud · lore

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

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?

[...]
Show 9 quoted lines
> --- 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?

[...]
Show 13 quoted lines
> --- 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

← back to recent threads