On 07/30, Johannes Schindelin wrote:
> Hi Thomas & Eric,
>
> 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@xxxxxxxxx> 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?
> > > > > 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
