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

From: Sonny Michaud <michaud.sonny@gmail.com>
Date: Aug 31, 2017, 21:49 UTC
Message-ID: <a46f972b-e83f-a08c-72ab-bfd1e7c4b310@gmail.com>
In-Reply-To: <xmqqshggvien.fsf@gitster.mtv.corp.google.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:

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