"Daniel Ferreira via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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.
