Jeff King <peff@xxxxxxxx> 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
