Subject: Re: [PATCH v1 01/27] revision: factor out parsing of diff-merge related options

From: Sergey Organov <[email-masked]>
Date: Thu, 03 Dec 2020 18:16:46 +0300

Junio C Hamano <[email-masked]> writes:

> Sergey Organov <[email-masked]> writes:
>
>> Move all the parsing code related to diffing merges into new
>> parse_diff_merge_opts() function.
>>
>> Signed-off-by: Sergey Organov <[email-masked]>
>> ---
>>  revision.c | 66 ++++++++++++++++++++++++++++++++----------------------
>>  1 file changed, 39 insertions(+), 27 deletions(-)
>>
>> diff --git a/revision.c b/revision.c
>> index aa6221204081..a09f4872acd7 100644
>> --- a/revision.c
>> +++ b/revision.c
>> @@ -2153,6 +2153,44 @@ static void add_message_grep(struct rev_info *revs, const char *pattern)
>>  	add_grep(revs, pattern, GREP_PATTERN_BODY);
>>  }
>>  
>> +static int parse_diff_merge_opts(struct rev_info *revs, const char **argv) {
>> +	int argcount;
>> +	const char *optarg;
>> +	const char *arg = argv[0];
>> +
>> +	if (!strcmp(arg, "-m")) {
>> +		/*
>> +		 * To "diff-index", "-m" means "match missing", and to the "log"
>> +		 * family of commands, it means "show full diff for merges". Set
>> +		 * both fields appropriately.
>> +		 */
>> +		revs->ignore_merges = 0;
>> +		revs->match_missing = 1;
>> +	} else if (!strcmp(arg, "-c")) {
>> +		revs->diff = 1;
>> +		revs->dense_combined_merges = 0;
>> +		revs->combine_merges = 1;
>> +	} else if (!strcmp(arg, "--cc")) {
>> +		revs->diff = 1;
>> +		revs->dense_combined_merges = 1;
>> +		revs->combine_merges = 1;
>> +	} else if (!strcmp(arg, "--no-diff-merges")) {
>> +		revs->ignore_merges = 1;
>> +	} else if (!strcmp(arg, "--combined-all-paths")) {
>> +		revs->diff = 1;
>> +		revs->combined_all_paths = 1;
>> +	} else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) {
>
> The reordering of if/else if/ cascade from the original, which
> serves no apparent purpose, makes reviewing a tad harder than
> necessary, but the conversion at this point seems wrong.

Sorry for the reordering, I didn't notice it when I cleared up the
series.

>
> The original allowed parse_long_opt() to potentially return 2
> (i.e. "diff-merges" and "off" appear as separate tokens on the
> command line), but the return value stored in argcount here is
> discarded, without affecting the return value from this function,
> which is fed back to the original in handle_revision_opt() below.

Nice catch!

>
>> +		if (!strcmp(optarg, "off")) {
>> +			revs->ignore_merges = 1;
>> +		} else {
>> +			die(_("unknown value for --diff-merges: %s"), optarg);
>> +		}
>
> To correct the above bug, it probably is sufficient to add
>
> 		return argcount;
>
> here.

Right, but not enough. "argcount" should also be set to 1 at the
beginning of the function, to avoid returning uninitialized value here.

Fixed for the next series.

Thanks,
-- Sergey Organov