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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Thomas,

On Mon, 30 Jul 2018, Thomas Gummerer wrote:

> On 07/30, Johannes Schindelin wrote:
> > 
> > On Sun, 29 Jul 2018, Thomas Gummerer wrote:
> > 
> > > On 07/29, Eric Sunshine wrote:
> > > > 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.
> > 
> > I immediately thought of testing for a leading `-` of the remaining
> > argument, but I could imagine that somebody enterprisey uses
> > 
> > 	git range-diff -- -my-first-attempt...-my-second-attempt
> > 
> > and I do not really want to complexify the code... Ideas?
> 
> Good point.  I can't really come up with a good option right now
> either.  It's not too bad, as users just typed the command, so it
> should be easy enough to see from the previous line what went wrong.
> 
> One potential option may be to turn "die(_("single arg format requires
> a symmetric range"));" into an 'error()', and show the usage?  I think
> that may be nice anyway, as "symmetric range" may not be immediately
> obvious to everyone, but together with the usage it may be clearer?

Agreed. Will be made so.

Ciao,
Dscho

> 
> > > > > > 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.
> > 
> > Sure, but not many users learn from reading the commit history...
> > 
> > :-)
> > 
> > Ciao,
> > Dscho
>