git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Robert Dailey <rcdailey.lists@gmail.com>
Cc: Ævar Arnfjörð Bjarmason <avarab@gmail.com>,
	Eckhard Maaß <eckhard.s.maass@googlemail.com>,
	Git <git@vger.kernel.org>
Subject: Re: Merge commit diff results are confusing and inconsistent
Date: Tue, 7 May 2019 09:44:36 -0700
Message-ID: <CABPp-BECj___HneAYviE3SB=wU6OTcBi3S=+Un1sP6L4WJ7agA@mail.gmail.com> (raw)
In-Reply-To: <CAHd499BkdpsA2BdB0Hsv3xXzpMyMzW8CSuYf2gQX0Jf7OoYBGw@mail.gmail.com>

On Tue, May 7, 2019 at 7:12 AM Robert Dailey <rcdailey.lists@gmail.com> wrote:
> On Mon, May 6, 2019 at 6:52 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

> The majority use case I'm interested in is seeing net-positive changes
> that happen in merge commits. Normally I take for granted that merge
> commits have nothing meaningful in them (meaningful here defined as
> something unexpected for a merge commit). But what if someone makes a
> poor decision and does some crazy refactoring in 1 file and amends it
> into a merge commit? Let's also say that these changes are done to a
> file that wasn't modified in any parent (say a unrelated.txt next to
> your color.txt). Since neither parent cares about that file for the
> purposes of the merge, I am trying to make sense of a revision
> specification that can be used to see what they did to that file.

An ability to find evil merges/cherry-picks/reverts by diffing a merge
commit to an auto-merge of some sort is something I am planning to
tackle; I'm tracking it at
https://bugs.chromium.org/p/git/issues/detail?id=12.  I'm working on
filter-repo first, then the merge rewrite, and then if I'm not
distracted by other stuff (a big if), then I'll tackle this.  Both
those other projects that are first in the list are rather large,
though, so it may be a while...

> Even ignoring that issue, the more concerning observation of mine is
> that `diff @^!` produces any output at all. If you exclude both
> parents, why do I see a diff for parent 2 (I see the complete diff of
> the branch that was merged in)?

I think using ^! on merge commits with diff ought to be an error,
personally.  Not that I'd want to get into the battle of figuring out
if someone did figure out what it means and find a valid use for it to
determine if we need to deprecate or provide alternate syntax or who
knows what for it.  I know it can be explained to be a combined diff
of some sort, but that's it.  ^! as a post-fix operator on a non-merge
commit makes sense to me and is useful, even though it's an ugly hack
and hideous usability-wise from a teaching or explaining perspective.
Without digging into the code in detail, I wouldn't for the life of me
be able to explain how ^! will work on merge commits with diff.

> Again, thank you for your example, you definitely made things very
> clear for me. I see where the confusion is. And I think --cc is a good
> way to get more context. At this point I'm just concerned about the
> @^! behavior with merge commits & diff.

I am too, though my suggestion for that case is: please, for all that
is good in the world, DON'T USE THAT.  EVER.  Whatever it might do, I
certainly don't want to waste any brain cycles supporting it or making
sure it still works.  diff is an endpoint operation and people should
pass endpoints to diff, not ranges (with only two exceptions I can
think of: '^!' on non-merge commits and usage of the super confusing
and ugly but also necessary '...' for diff against a merge-base.)

Honestly, I am tempted to make git throw a warning whenever folks use
a range operator with diff other than the exceptions noted above.
Hmm...

Elijah

  parent reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-03 15:55 Robert Dailey
2019-05-03 19:12 ` Eckhard Maaß
2019-05-06 15:38   ` Robert Dailey
2019-05-06 16:52     ` Eckhard Maaß
2019-05-06 23:52     ` Ævar Arnfjörð Bjarmason
2019-05-07 14:10       ` Robert Dailey
2019-05-07 14:41         ` Robert Dailey
2019-05-07 14:58         ` Denton Liu
2019-05-07 15:55           ` Eckhard Maaß
2019-05-07 15:26         ` Ævar Arnfjörð Bjarmason
2019-05-07 16:44         ` Elijah Newren [this message]
2019-05-11 14:08         ` Philip Oakley

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CABPp-BECj___HneAYviE3SB=wU6OTcBi3S=+Un1sP6L4WJ7agA@mail.gmail.com' \
    --to=newren@gmail.com \
    --cc=avarab@gmail.com \
    --cc=eckhard.s.maass@googlemail.com \
    --cc=git@vger.kernel.org \
    --cc=rcdailey.lists@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git