From: Slavica Djukic <slavicadj.ip2018@gmail.com>
Date: Fri, 22 Feb 2019 09:03:49 GMT
Subject: Re: [PATCH v5 01/10] diff: export diffstat interface
Message-ID: <0f6b3dc7-eccf-3352-22a8-97ef052a6ada@gmail.com>
In-Reply-To: <xmqq4l8xuiy9.fsf@gitster-ct.c.googlers.com>

Hello Junio,

Thank you for suggestions and for taking time to look at
this patch series.

On 21-Feb-19 6:53 PM, Junio C Hamano wrote:
> "Daniel Ferreira via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> @@ -6001,12 +5985,7 @@ void diff_flush(struct diff_options *options)
>>   	    dirstat_by_line) {
>>   		struct diffstat_t diffstat;
>>   
>> -		memset(&diffstat, 0, sizeof(struct diffstat_t));
>> -		for (i = 0; i < q->nr; i++) {
>> -			struct diff_filepair *p = q->queue[i];
>> -			if (check_pair_status(p))
>> -				diff_flush_stat(p, options, &diffstat);
>> -		}
>> +		compute_diffstat(options, &diffstat);
>>   		if (output_format & DIFF_FORMAT_NUMSTAT)
>>   			show_numstat(&diffstat, options);
>>   		if (output_format & DIFF_FORMAT_DIFFSTAT)
> In the post-context of this hunk there are series of "if we are
> showing this kind of diffstat, pass &diffstat to a helper that shows
> it" calls, and this piece of code itself is guarded by "if we are
> showing any of these kinds of diffstat, enter this block".  So a
> helper function that computes necessary data in &diffstat upfront
> does make sense and makes the code readable quite a lot.
>
> But...
>
>
>> +void compute_diffstat(struct diff_options *options, struct diffstat_t *diffstat)
>> +{
>> +	int i;
>> +	struct diff_queue_struct *q = &diff_queued_diff;
> ... as a reusable helper, it would make a saner API if you did not
> to hardcode the dependency to the singleton diff_queued_diff
> (i.e. instead, pass a pointer to struct diff_queue_struct as a
> parameter---the caller has it as 'q' at the callsite).
>
> Other than that, makes sense to me; thanks.
>
> Here is a meta question, which is mostly meant to those who use
> gitgitgadget@.  The person who wanted to send this copy of the patch
> this time (i.e. Slavica, not Daniel) is not shown anywhere on the
> header of the e-mail, so the response to the message will not go to
> Slavica at all, even though it probably is visible by monitoring
> where gitgitgadget@ delivers (i.e. the PR comments).
>
> Is that desirable as a reviewee?  I manually added Slavica's address
> taken from the S-o-b: line to this response just in case, but if it
> is not desirable, I'll stop doing so.


Since gitgitgadget puts reviewer's e-mails in PR comments and send
e-mails (to me) as well, you don't need to add manually my address.

But thanks for thinking about this and doing it just in case
I don't get e-mails.


>
> Thanks.
>