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

From: Jeff King <peff@peff.net>
Date: Aug 4, 2020, 22:07 UTC
Message-ID: <20200804220740.GA2022650@coredump.intra.peff.net>
In-Reply-To: <87y2muxf5m.fsf@osv.gnss.ru>

On Wed, Aug 05, 2020 at 12:41:25AM +0300, Sergey Organov wrote:

Show 13 quoted lines

> >> 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