Re: [PATCH v3] difftool: Change prompt to display the number of files in the diff queue
From: David Aguilar <hidden>
Date: 2016-06-15 22:59:28
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 hunk ↗ jump to hunk
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"?
{
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?
quoted hunk ↗ jump to hunk
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? These are tiny nitpicky style notes; it looks good otherwise. Thanks, -- David