Re: [PATCH v3] difftool: Change prompt to display the number of files in the diff queue
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 18, 2013, 06:06 UTC
- Message-ID
- <xmqqmwjyk810.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <20131218052523.GB90546@gmail.com>
David Aguilar <davvid@gmail.com> writes:
Show 18 quoted lines
> 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 ;-).
Show 11 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?;-) A bit of slack is fine, but 50 might be excessive (more than twice as big as necessary).
Show 15 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?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.