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

[Sergey Organov <sorganov@gmail.com>]

Elijah Newren <newren@gmail.com> writes:

> On Sat, Dec 5, 2020 at 11:44 AM Sergey Organov <sorganov@gmail.com> wrote:
>>
>> Elijah Newren <newren@gmail.com> writes:
>>
>> > On Fri, Dec 4, 2020 at 11:23 AM Sergey Organov <sorganov@gmail.com> wrote:
>> >>
>> >> Elijah Newren <newren@gmail.com> writes:
>> >>
>> >> > On Thu, Dec 3, 2020 at 11:48 AM Sergey Organov
>> >> > <sorganov@gmail.com> wrote:
>> >> >>
>> >> >> Elijah Newren <newren@gmail.com> writes:
>> >> >>
>> >> >> > On Sun, Nov 8, 2020 at 1:43 PM Sergey Organov
>> >> >> > <sorganov@gmail.com> wrote:
>> >> >> >>
>> >> >> >> 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?
>> >> >>
>> >> >> >
>> >> >> > 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?
>> >
>> >> >
>> >> > (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