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

On 07/30, Johannes Schindelin wrote:

Hi Thomas & Eric,

On Sun, 29 Jul 2018, Thomas Gummerer wrote:

quoted

On 07/29, Eric Sunshine wrote:

quoted

On Sun, Jul 29, 2018 at 3:04 PM Thomas Gummerer [off-list ref] wrote:

quoted

On 07/21, Johannes Schindelin via GitGitGadget wrote:

quoted

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?

quoted

quoted

quoted

quoted

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