Re: [PATCH v2 3/4] add--helper: implement interactive status command

From: Junio C Hamano <gitster@pobox.com>
Date: May 16, 2017, 04:39 UTC
Message-ID: <xmqqinl12z5d.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1494907234-28903-4-git-send-email-bnmvco@gmail.com>

Daniel Ferreira <bnmvco@gmail.com> writes:

Show 8 quoted lines

> +	if (!q->nr)
> +		return;
> +
> +	for (i = 0; i < q->nr; i++) {
> +		struct diff_filepair *p;
> +		p = q->queue[i];
> +		diff_flush_stat(p, options, &stat);
> +	}

Commenting just on the part that interacts with the diff machinery, without/before carefully reading the remainder of the patches.

I suspect that refactoring this part out of diff_flush() into a helper function "compute_diffstat()", like this:

diff --git a/diff.c b/diff.c
index 74283d9001..a42ff42e92 100644
--- a/diff.c
+++ b/diff.c
@@ -4770,12 +4770,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)

and then exporting that function and "struct diffstat_t" to your
helper, may make it a better API, rather than having the callers to
call diff_flush_stat() for each and every path.