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

[Slavica Djukic]

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