David Aguilar <davvid@xxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html