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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 21, 2019, 17:53 UTC
Message-ID: <xmqq4l8xuiy9.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <d839f0c082ece5298647f8dfbfd0566d59c58172.1550662887.git.gitgitgadget@gmail.com>

"Daniel Ferreira via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 14 quoted lines

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