From: Sergey Organov <sorganov@gmail.com>
Date: Tue, 04 Aug 2020 22:15:27 GMT
Subject: Re: [PATCH v2 0/7] making log --first-parent imply -m
Message-ID: <87y2muvz0g.fsf@osv.gnss.ru>
In-Reply-To: <20200804220740.GA2022650@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> On Wed, Aug 05, 2020 at 12:41:25AM +0300, Sergey Organov wrote:
>
>> >> 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.
>
> Making it counteract "--cc" makes sense, but revs->diff is used for much
> more than that. So "--cc" sets revs->diff to 1, but so do many other
> unrelated options (e.g., "--full-diff" for one).
>
> I think to do it right you'd need to have this part of the code just set
> a single enum variable, like:
>
>   ...
>   else if (!strcmp(arg, "--cc")) {
>           revs->diff_merges = DIFF_MERGES_DENSE_COMBINED;
>   } else if (!strcmp(arg, "-m")) {
>           revs->diff_merges = DIFF_MERGES_ALL_PARENTS;
>   }
>   ...etc...
>
> and then later resolve that into the set of flags we need:
>
>   switch (revs->diff_merges) {
>   case DIFF_MERGES_DENSE_COMBINED:
> 	revs->diff = 1;
> 	revs->dense_combined_merges = 1;
> 	revs->combine_merges = 1;
> 	revs->ignore_merges = 0;
> 	break;
>   case DIFF_MERGES_ALL_PARENTS:
> 	revs->ignore_merges = 0;
> 	break;
>   ...etc...
>   }
>
> it may even be that we can get rid of the separate combine_merges and
> dense_combined_merges flag and just have callers look at
> rev.diff_merges, which would simplify the code even further. But that
> cleanup could also come later on top.

Sounds like a plan! I like it.

Thanks,
-- Sergey