From: Junio C Hamano <gitster@pobox•com>
To: Jeff King <peff@peff•net>
Cc: git@vger•kernel.org
Subject: Re: [PATCH v2 4/6] add --simplify-combined-diff option
Date: Tue, 29 Jul 2014 13:02:39 -0700 [thread overview]
Message-ID: <xmqqd2co2asg.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <20140729175712.GD31181@peff.net> (Jeff King's message of "Tue, 29 Jul 2014 13:57:12 -0400")
Jeff King <peff@peff•net> writes:
> When showing a combined diff, we are comparing two (or more)
> parents to a final state, and some of these states may be
> the same. Here's a table of the possible contents for a
> given path (for two parents, but we will generalize to more
> in a moment; we also omit isomorphic cases where the parents
> are swapped):
>
> case | M | P1 | P2
> -------------------
> 1 | A | A | A
> 2 | A | A | B
> 3 | A | B | B
> 4 | A | B | C
>
> In case 1, the path was not relevant to the merge at all,
> and we omit it as uninteresting. In case 2, we did resolve
> the path, but in favor of one side. We also consider this
> uninteresting and do not show the diff.
>
> In case 4, we had a real content-level merge, and the
> combined diff is interesting. We show it.
>
> That leaves case 3, in which both parents are the same, but
> the merge picks a new content. This should be rare in
> normal merges, though it could happen if you updated an
> unrelated file due to a resolution elsewhere (i.e., an evil
> merge that crosses a file boundary). But it happens
> frequently in the fake merge commits we create for stashes,
> in which one parent is the base of the stash and the other
> is the index (in which case it simply means that the index
> entry for the path was not touched).
>
> Right now, we treat it the same as case 4, and show a normal
> combined diff. However, the result is harder to read, and
> the combined nature of the diff gives no extra information;
> every marker in the combined diff will be identical for both
> parents.
>
> This patch adds a new option, "--simplify-combined-diff",
> which converts this case into a normal, non-combined diff.
> It would not be correct to simply omit it, because there
> really is an interesting change going from B..A. It's just
> that there are not two interesting changes, which the
> combined diff would show.
>
> When generalizing this to more than two parents, we have two
> options:
>
> 1. Either simply to a single parent content, or not at all.
>
> 2. Omit parents whose contents are duplicates of other
> parents.
>
> For a case like "A B B C", option (2) would still result in
> a combined diff, but one with fewer sources. However, it
> would also be ambiguous. The parents in a combined diff are
> marked only by their position, so omitting a position means
> that a reader can no longer tell which line goes with which
> parent.
>
> Instead, we choose option (1). Either you get the full
> combined diff, or you get a normal non-combined diff.
Very nicely analyzed. The changes to the code looked also good from
a cursory read.
Thanks.
next prev parent reply other threads:[~2014-07-29 20:02 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-07-29 11:53 [PATCH 0/2] improving "git stash list -p" Jeff King
2014-07-29 12:00 ` [PATCH 1/2] add --simplify-combined-diff option Jeff King
2014-07-29 13:00 ` Jeff King
2014-07-29 12:03 ` [PATCH 2/2] stash: default listing to "--cc --simplify-combined-diff" Jeff King
2014-07-29 12:07 ` [RFC/PATCH 3/2] stash: show combined diff with "stash show" Jeff King
2014-07-29 18:13 ` Junio C Hamano
2014-07-31 0:17 ` Jeff King
2014-07-31 0:28 ` Junio C Hamano
2014-07-29 17:53 ` [PATCH v2 0/6] improving "git stash list -p" Jeff King
2014-07-29 17:53 ` [PATCH v2 1/6] revision: drop useless string offset when parsing "--pretty" Jeff King
2014-07-29 17:54 ` [PATCH v2 2/6] pretty: treat "--format=" as an empty userformat Jeff King
2014-07-29 17:56 ` [PATCH v2 3/6] pretty: make empty userformats truly empty Jeff King
2014-07-29 17:57 ` [PATCH v2 4/6] add --simplify-combined-diff option Jeff King
2014-07-29 20:02 ` Junio C Hamano [this message]
2014-07-29 17:57 ` [PATCH v2 5/6] stash: default listing to "--cc --simplify-combined-diff" Jeff King
2014-07-29 18:58 ` Junio C Hamano
2014-07-30 19:43 ` Junio C Hamano
2014-07-31 0:09 ` Jeff King
2014-08-12 23:30 ` Keller, Jacob E
2014-07-29 17:58 ` [PATCH v2 6/6] stash: show combined diff with "stash show" Jeff King
Reply instructions:
You may reply publicly 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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqd2co2asg.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=peff@peff$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox