Re: [PATCH v1 00/27] git-log: implement new --diff-merge options

Elijah Newren [off-list ref] writes:

On Sat, Dec 5, 2020 at 11:44 AM Sergey Organov [off-list ref] wrote:

quoted

Elijah Newren [off-list ref] writes:

quoted

On Fri, Dec 4, 2020 at 11:23 AM Sergey Organov [off-list ref] wrote:

quoted

Elijah Newren [off-list ref] writes:

quoted

On Thu, Dec 3, 2020 at 11:48 AM Sergey Organov
[off-list ref] wrote:

quoted

Elijah Newren [off-list ref] writes:

quoted

On Sun, Nov 8, 2020 at 1:43 PM Sergey Organov
[off-list ref] wrote:

quoted

These patch series implement new set of options governing the
diff output
of merge commits, all under the umbrella of the single
--diff-merges=<mode>
option. Most of the new options being synonyms for
-m/-c/--cc options,
there is also additional functionality provided, allowing to get
the format
of "-p --first-parent" without change in history traversal that
--first-parent option causes.

The net result of these series are the following new options:

--diff-merges=   |  old equivalent
-----------------+----------------
first-parent     | --first-parent (only format implications)
separate         | -m
combined         | -c
dense-combined   | --cc

Interesting.  I have some local patches implementing another choice,
with the new flag --remerge-diff.  This flag will cause `git show` or
`git log` to automatically remerge the two parents in a
2-parent merge
commit, and then diff the merge commit against that automatic merge,
showing the result. Thus, the diff for a merge commit is
likely to be
empty if the merge was clean, and is likely to show the removal of
conflict markers if the merge was not clean.

I'm curious how it'd interact with this new option. Would it
also get
a name, e.g. --diff-merges=remerge-diff? Feels like a
mouthful, but I
can't come up with anything better.

Maybe, --diff-merges=remerge?

quoted

Also, I'm curious how it'd interact with another option I
added, named
--remerge-diff-only.  This latter option modifies revision traversal
in that it skips octopus merges, root commits, and single parent
commits IF no cherry-pick or revert information can be found.  If it
finds a 2-parent merge commit, it behaves like --remerge-diff.  If it
finds a 1-parent commit with cherry-pick or revert information, it'll
do an in memory repeat of that cherry-pick (or revert) and then diff
the actual commit against what the automatic cherry-pick would
perform.  Again, that likely means an empty diff if the automatic
cherry-pick was clean, and showing any changes made by the user to
complete the original cherry-pick (such as deleting conflict markers
and picking which chunks from which side to keep) if the automatic
cherry-pick was not clean.  (I suspect --remerge-diff-only is much
more likely to be used with `git show` than with `git log`.)  Anyway,
your changes seem to suggest that anything relating to how diffs for
merges are handled should be documented in the same section, but
--remerge-diff-only doesn't fit.  And it'd seem odd to have
--remerge-diff and --remerge-diff-only not show up in adjacently but
be split into separate sections.  Any ideas?

Sounds like commits limiting option to me. I think it could be named by
its limiting behavior only, say, --remerges. Then it will imply
--diff-merges=remerge, that'd allow user to re-define diff
format if she
needs to.

It is commit limiting, but the focus is more on the behavioral change
in how diffs are shown:
  * for 2-parent merges
  * for single-parent commits with cherry-pick or revert information
and acknowledging that since it has _altered_ the normal way of
showing diffs for a number of single-parent commits, that it'd be
confusing to show normal diffs of unaffected commits (how would you be
able to tell what type of diff you're looking at if both appear in the
log?).  Thus, it does commit limiting to only select commits which
will make use of the new diff type.

That's how you currently look at it.

For me it looks like pure commit limiting with these criteria might be
useful by itself, and with my suggestion one could then achieve it
using, say:

--remerge-diff-only --diff-merges=off

I see what you're saying, and I think there's some value in it.  But I
think there's something still missing.  For example, you suggest
getting the commit limiting I mention with

    --remerge-diff-only --diff-merges=off

But --diff-merges is only supposed to control _merge_ commits, which I
flagged as the big impedance mismatch for my new option.  Why would it
turn off diffs for non-merge commits like cherry-picks and rebases?

quoted

quoted

(I suspect it will be more common for folks to use the
--remerge-diff-only option, or whatever we end up calling it, with
`git show` where the commit limiting doesn't matter -- but I have used
it with log to go looking for "evil" reverts/cherry-picks that might
have occurred in history.)

What you describe is complex enough to doubt it could be entirely
described by option name, so shorter --evils might be better choice
in this case.

Overall, if you add --diff-merges=remerge as a new diff format, and then
--evils that implies the former, then it seems like all possible
use-cases will be covered, and you have short option name for the most
useful case.

Since you want things to have orthogonal subcomponents that can be
built up, let's assume we did make --remerge-diff-only be solely about
commit limiting.  In that case, --evils could be gotten by specifying
a combination of flags, and --evils would just be a shorthand.  What
are the flags that you would need to specify, though?  In particular,
you've only named two options above and they don't cover the necessary
behavior; a third is needed:

   --remerge-diff-only --diff-merges=remerge
--${DIFF_OPTION_NAME_FOR_CHERRY_PICKS_AND_REVERTS}=remerge

The first two aren't enough because --diff_merges only changes how
diffs for _merge_ commits are shown, and we need a flag for changing
how the single-parent cherry-pick and reverts are shown.

Yeah, I see your point. I didn't get it from the beginning that you want
yet another representation format for regular commits as well. However,
as far as I can tell, if --evils flag is active, you do consider
cherry-picks and reverts as kind of merges, that makes sense as they
actually /are/ expected to be results of specific /merge operation/,
even though they are not /merge commits/, so semantically they do have
second parent reference (to the original commit), even if a virtual one.

To further illustrate my point, reverts and cherry-picks could have been
implemented, for example, as merge commits with, say, 3-rd parent
pointing back to the original commit (not 2-nd parent, both to
differentiate from regular merges and to support cherry-picking of merge
commits.)

As a side-note, people rarely differentiate between
"merge-the-operation" and "merge-the-result" anyway, even when it leads
to confusion.

Overall, if we take the above into account, it seems to be fine if
--diff-merges does affect the representation or such "quasi-merge"
commits, for the purposes of --evils option.

Yeah, that could make sense.  We'd still need some additional flag.
One flag, perhaps --diff-merges=remerge, would be the one that would
_only_ affect actual two-parent merges (which I use as the default
instead of --cc with `git show`, and which I make implied by default
with `git log --patch` at $DAYJOB), and we'd need another name for
whatever turned those special diffs on for cherry-picks and reverts.
Further, the extra flag that turns on special diffs for cherry-picks
and reverts should turn off diffs entirely for any other single-parent
commits.  (I think it's just too confusing for any command to show
multiple single-parent commits with a mixture of different types of
diffs.)

I think you may safely pretend this special diff always produces empty
diff for regular commits, so there will be no need to "disable"
anything.

-- Sergey Organov