git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Elijah Newren <newren@gmail.com>
To: "brian m. carlson" <sandals@crustytoothpaste.net>,
	Elijah Newren <newren@gmail.com>,
	Git Mailing List <git@vger.kernel.org>, Jeff King <peff@peff.net>
Subject: Re: [PATCH] log,diff-tree: add --combined-with-paths options for merges with renames
Date: Fri, 25 Jan 2019 08:27:36 -0800	[thread overview]
Message-ID: <CABPp-BH-7iuSYmxA080UUbuKSEy32-T58oDKNajmFyeFWfauEA@mail.gmail.com> (raw)
In-Reply-To: <20190125021950.GV423984@genre.crustytoothpaste.net>

Hi,

On Thu, Jan 24, 2019 at 6:19 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On Thu, Jan 24, 2019 at 08:46:54AM -0800, Elijah Newren wrote:
> > The critical part of the patch is the few line change to
> > show_raw_diff(), the rest is plumbing to set that up.
> >
> > This patch was based out of Peff's suggestion[1] to fix diff-tree to
> > do what I needed rather than bending fast-export to cover my usecase;
> > I've been running with a hacky version of this patch for a while and
> > finally cleaned it up.
> >
> > [1] https://public-inbox.org/git/20181114071454.GB19904@sigill.intra.peff.net/
> >
> > As an alternative, I considered perhaps trying to sell it as a bugfix
> > (how often do people use -M, -c, and --raw together and have renames
> > in merge commits -- can I just change the format to include the old
> > names), but was worried that since diff-tree is plumbing and that the
> > format was documented to not include original filename(s), that I'd be
> > breaking backward compatibility in an important way for someone and
> > thus opted for a new flag to get the behavior I needed.
> >
> > I did struggle a bit to come up with a name for the option; if others
> > have better suggestions, I'm happy to switch.
>
> Maybe --all-names? You should definitely let other people chime in on
> this as well; it should be obvious by now that I'm no expert on naming
> things.

--all-names may work.  Two possible worries:

1) Would people expect the output when using this option to be of the form:

::100644 100644 100644 fabadb8 cc95eb0 4866510 MM    describe.c
describe.c    describe.c

In other words, even if there are no renames involved, would they
expect us to show the filename from each side of the merge?  I was
trying to make the combined diff output format more similar to how we
handle non-merge commits; we only show more than one filename when a
rename is involved.

2) When looking through manpages I have often found an option that I
thought was related to what I wanted, only to discover that a short
semi-ambiguous option had a much different context in mind than I did
and thus performed some operation entirely unrelated to anything I was
interested in.  I suspect that this won't be a highly used option, as
such, a longer name to disambiguate seemed reasonable.


Of course --combined-with-paths isn't fully descriptive either, and
making it even longer might be annoying, so...  *shrug*

>
> > Range-diff:
> > 1:  29e9ddf532 = 1:  29e9ddf532 log,diff-tree: add --combined-with-paths options for merges with renames
> >
> >  Documentation/diff-format.txt      | 23 +++++++++++++---
> >  Documentation/git-diff-tree.txt    |  9 +++++--
> >  Documentation/rev-list-options.txt |  5 ++++
> >  combine-diff.c                     | 42 ++++++++++++++++++++++++++----
> >  diff.h                             |  1 +
> >  revision.c                         |  7 +++++
> >  revision.h                         |  1 +
> >  7 files changed, 78 insertions(+), 10 deletions(-)
>
> I think it might be nice to see a test for this option so that we avoid
> breaking it in the future. I'm also curious how this works with -z, and
> a test for that would be interesting as well (as well as illustrative of
> the format). For example, is it still unambiguous for machine parsing,
> even with oddly named files?

I'll add a test for both with and without -z.

  reply	other threads:[~2019-01-25 16:27 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-24 16:46 [PATCH] log,diff-tree: add --combined-with-paths options for merges with renames Elijah Newren
2019-01-25  2:19 ` brian m. carlson
2019-01-25 16:27   ` Elijah Newren [this message]
2019-01-25 14:45 ` Derrick Stolee
2019-01-25 16:30   ` Elijah Newren
2019-01-25 16:54 ` [PATCH v2] " Elijah Newren
2019-01-25 17:40   ` Derrick Stolee
2019-01-25 17:52     ` Elijah Newren
2019-01-25 19:51       ` Eric Sunshine
2019-01-26 22:07         ` Elijah Newren
2019-01-27  1:52       ` brian m. carlson
2019-01-26 22:18   ` [PATCH v3] log,diff-tree: add --combined-all-names option Elijah Newren
2019-02-04 20:07     ` [PATCH v4] " Elijah Newren
2019-02-04 21:20       ` Junio C Hamano
2019-02-05 15:51         ` Elijah Newren
2019-02-05 20:39           ` Junio C Hamano
2019-02-07 19:50             ` Elijah Newren
2019-02-07 20:25               ` Junio C Hamano
2019-02-07 22:10                 ` Elijah Newren
2019-02-07 23:31                   ` Junio C Hamano
2019-02-07 23:48                     ` Elijah Newren
2019-02-05  9:48       ` Johannes Schindelin
2019-02-05 15:54         ` Elijah Newren
2019-02-05 18:04           ` Junio C Hamano
2019-02-07 22:28       ` Junio C Hamano
2019-02-07 23:48         ` Elijah Newren
2019-02-08  1:12       ` [PATCH v5 0/2] add --combined-all-paths option to log and diff-tree Elijah Newren
2019-02-08  1:12         ` [PATCH v5 1/2] log,diff-tree: add --combined-all-paths option Elijah Newren
2019-02-08  4:00           ` Junio C Hamano
2019-02-08  6:52             ` Elijah Newren
2019-02-08 17:50               ` Junio C Hamano
2019-02-08  1:12         ` [PATCH v5 2/2] squash! " Elijah Newren
2019-02-08  4:14           ` Junio C Hamano
2019-02-08  6:48             ` Elijah Newren
2019-01-25 19:29 ` [PATCH] log,diff-tree: add --combined-with-paths options for merges with renames Junio C Hamano
2019-01-25 20:04   ` Elijah Newren
2019-01-25 22:21     ` Junio C Hamano
2019-01-26 22:12       ` Elijah Newren
2019-01-28  0:19         ` Junio C Hamano

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

  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-BH-7iuSYmxA080UUbuKSEy32-T58oDKNajmFyeFWfauEA@mail.gmail.com \
    --to=newren@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    --cc=sandals@crustytoothpaste.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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).