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

From: David Aguilar <davvid@gmail.com>
Date: Dec 18, 2013, 05:25 UTC
Message-ID: <20131218052523.GB90546@gmail.com>
In-Reply-To: <1386286726-26653-1-git-send-email-zoltan.klinger@gmail.com>

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:

Show 11 quoted lines

> 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"?

Show 8 quoted lines

>  {
>  	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?

Show 11 quoted lines

> 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