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