Re: [PATCH v5 01/10] diff: export diffstat interface

[Junio C Hamano <gitster@pobox.com>]

"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.

Thanks.