Re: [PATCH v4 05/21] range-diff: also show the diff between patches

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Jul 29, 2018, 21:45 UTC
Message-ID: <20180729214543.GD9955@hank.intra.tgummerer.com>
In-Reply-To: <CAPig+cTuD0+8etdMLu8FkFVxnXUM218taxU9in-fe3QXhDj5WQ@mail.gmail.com>

On 07/29, Eric Sunshine wrote:

Show 21 quoted lines

> On Sun, Jul 29, 2018 at 3:04 PM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> > On 07/21, Johannes Schindelin via GitGitGadget wrote:
> > > Just like tbdiff, we now show the diff between matching patches. This is
> > > a "diff of two diffs", so it can be a bit daunting to read for the
> > > beginner.
> > > [...]
> > > Note also: while tbdiff accepts the `--no-patches` option to suppress
> > > these diffs between patches, we prefer the `-s` option that is
> > > automatically supported via our use of diff_opt_parse().
> >
> > One slightly unfortunate thing here is that we don't show these
> > options in 'git range-diff -h', which would be nice to have.  I don't
> > know if that's possible in git right now, if it's not easily possible,
> > I definitely wouldn't want to delay this series for that, and we could
> > just add it to the list of possible future enhancements that other
> > people mentioned.
> 
> This issue is not specific to git-range-diff; it's shared by other
> commands which inherit diff options via diff_opt_parse(). For
> instance, "git log -h" doesn't show diff-related options either, yet
> it accepts them.

Fair enough, that makes sense.  Thanks for the pointer!

There's one more thing that I noticed here:

    git range-diff --no-patches
    fatal: single arg format requires a symmetric range

Which is a slightly confusing error message. In contrast git log does the following on an unrecognized argument:

    git log --no-patches
    fatal: unrecognized argument: --no-patches

which is a little better I think. I do however also thing the "fatal: single arg format requires a symmetric range" is useful when someone genuinely tries to use the single argument version of the command. So I don't know what a good solution for this would be.

Show 18 quoted lines

> > > diff --git a/range-diff.c b/range-diff.c
> > > @@ -300,6 +325,9 @@ static void output(struct string_list *a, struct string_list *b)
> > >                       printf("%d: %s ! %d: %s\n",
> > >                              b_util->matching + 1, short_oid(a_util),
> > >                              j + 1, short_oid(b_util));
> > > +                     if (!(diffopt->output_format & DIFF_FORMAT_NO_OUTPUT))
> >
> > Looking at this line, it looks like it would be easy to support
> > '--no-patches' as well, which may be slightly easier to understand that
> > '-s' to someone new to the command.  But again that can be added later
> > if someone actually cares about it.
> 
> What wasn't mentioned (but was implied) by the commit message is that
> "-s" is short for "--no-patch", which also comes for free via
> diff_opt_parse(). True, "--no-patch" isn't spelled exactly the same as
> "--no-patches", but git-range-diff isn't exactly a perfect tbdiff
> clone, so hopefully not a git problem. Moreover, "--no-patch" is
> internally consistent within the Git builtin commands.

Makes sense, thanks! "--no-patch" does make sense to me. There's still a lot of command line flags in git to learn for me, even after all this time using it ;) Might be nice to spell it out in the commit message for someone like me, especially as "--no-patches" is already mentioned. Though I guess most regulars here would know about "--no-patch", so maybe it's not worth it. Anyway that is definitely not worth another round here.