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

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Move all the parsing code related to diffing merges into new
>> parse_diff_merge_opts() function.
>>
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>>  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.

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

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