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

Reply via email to