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

[Junio C Hamano <gitster@pobox.com>]

David Aguilar <davvid@gmail.com> 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:
>> 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 ;-).

>
>>  {
>>  	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).

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