Jeff King <peff@xxxxxxxx> writes:
> On Tue, Aug 04, 2020 at 11:56:25PM +0300, Sergey Organov wrote:
>
>> >> diff --git a/revision.c b/revision.c
>> >> index 669bc856694f..dcdff59bc36a 100644
>> >> --- a/revision.c
>> >> +++ b/revision.c
>> >> @@ -2323,10 +2323,31 @@ static int handle_revision_opt(struct
>> >> rev_info *revs, int argc, const char **arg
>> >> revs->diff = 1;
>> >> revs->diffopt.flags.recursive = 1;
>> >> revs->diffopt.flags.tree_in_recursive = 1;
>> >> - } else if (!strcmp(arg, "-m") || !strcmp(arg, "--diff-merges")) {
>> >> + } else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) {
>> >> revs->ignore_merges = 0;
>> >> + if (!strcmp(optarg, "off")) {
>> >> + revs->ignore_merges = 1;
>> >> + } else if (!strcmp(optarg, "all")) {
>> >> + revs->diff = 0;
>> >
>> > Should this be revs->ignore_merges = 0?
>>
>> It's 4 lines above, as it's in fact common for all the cases but the
>> first one.
>
> Ah, I missed that. That raises more questions, though. ;)
>
> For "-m" we do not need to set revs->diff; why do we need to do so
> here?
Good question!
I believe --diff-merges=all should reset revs->diff back to 0 to
entirely undo all the effects of '-c' and '--cc', provided those
appeared before on the command-line, that '-m' fails to do.
Admittedly, I didn't yet check what is the outcome of, say,
"git log -c -m". Is it = "-c", ="-m", or what?
Also, this makes 'all' not the entire synonym for '-m', and that's why I
removed 'all' from the second version of the patch ;-)
>
> For "--cc", we do not need to set revs->ignore_merges. Why do we need to
> do so here? We do need it set eventually, but I think setup_revisions()
> later handles that, and wants ignore_merges untouched to decide whether
> the user asked for it or not.
OK, I'll take further look at this for further changes, -- thanks for
telling!
My general approach though is that every of mutually exclusive options
should better explicitly set all the involved variables appropriately.
Thanks,
-- Sergey
