Re: [PATCH v3] difftool: Change prompt to display the number of files in the diff queue

David Aguilar [off-list ref] writes:

Thanks for the patch, and sorry for the late response.
I have just a couple of notes below...

On Fri, Dec 06, 2013 at 10:38:46AM +1100, Zoltan Klinger wrote:

quoted

diff --git a/diff.c b/diff.c
index e34bf97..a7d5a47 100644
--- a/diff.c
+++ b/diff.c

@@ -2899,11 +2899,16 @@ static void run_external_diff(const char *pgm,
 			      struct diff_filespec *one,
 			      struct diff_filespec *two,
 			      const char *xfrm_msg,
-			      int complete_rewrite)
+			      int complete_rewrite,
+			      struct diff_options *o)

Very minor nit -- "o" is a very terse variable name.
Maybe "opts"?

The diff-options parameter passed around in the callchain has always
been "o" throughout this file from the beginning of time, though ;-).

quoted

 {
 	const char *spawn_arg[10];
 	int retval;
 	const char **arg = &spawn_arg[0];
+	struct diff_queue_struct *q = &diff_queued_diff;
+	const char *env[3] = { NULL };
+	char env_counter[50];
+	char env_total[50];

Hard-coded 50; what's the length of the maximum signed int?

;-) A bit of slack is fine, but 50 might be excessive (more than
twice as big as necessary).

quoted

diff --git a/diff.h b/diff.h
index e342325..42bd34c 100644
--- a/diff.h
+++ b/diff.h

@@ -164,6 +164,8 @@ struct diff_options {
 	diff_prefix_fn_t output_prefix;
 	int output_prefix_length;
 	void *output_prefix_data;
+
+	int diff_path_counter;
 };

Since these are already "diff_options" it seems redundant to call
the struct entry the "diff_path_counter" when "path_count"
should be specific enough.  Would it make sense to rename it?

Yeah, makes sense.  diff_options->diff_path_counter++ sounds awful,
while options->path_count++ looks quite tame and reasonable.

These are tiny nitpicky style notes; it looks good otherwise.

Thanks.