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

[Sonny Michaud <michaud.sonny@gmail.com>]

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