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

Jeff King [off-list ref] writes:

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

quoted

quoted

quoted

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