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