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.
> Admittedly, I didn't yet check what is the outcome of, say,
> "git log -c -m". Is it = "-c", ="-m", or what?
Having looked at the code, I'm pretty sure it behaves like "-c". IMHO
that is a bug and the two should be mutually exclusive (i.e., override
each other). Changing that would not be strictly backwards, but I think
it may be OK under the notion that the current behavior is nonsense.
-Peff
