All of lore.kernel.org
 help / color / mirror / Atom feed
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.

  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.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.