Subject: Re: [PATCH v2 0/7] making log --first-parent imply -m

From: Sergey Organov <[email-masked]>
Date: Wed, 05 Aug 2020 00:41:25 +0300

Jeff King <[email-masked]> 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