range-diff: add a --no-patch option to show a summary

Add a --no-patch option which shows which changes got removed, added or moved etc., without showing the diff associated with them.

This allows for using range-diff as a poor man's "shortlog" for force-pushed branches to see what changed without getting into the details of what specifically. E.g. diffing the latest forced-push to "pu" gives us:

    $ ./git-range-diff --no-patch b58974365b...711aaa392f | head -n 10
     -:  ---------- >  1:  b613de67c4 diff: differentiate error handling in parse_color_moved_ws
    28:  c731affab0 !  2:  23c4bbe28e build: link with curl-defined linker flags
     -:  ---------- >  3:  14f74d5907 git-worktree.txt: correct linkgit command name
     -:  ---------- >  4:  29d51e214c sequencer.c: remove a stray semicolon
     -:  ---------- >  5:  b7845cebc0 tree-walk.c: fix overoptimistic inclusion in :(exclude) matching
     -:  ---------- >  6:  1a550529b1 t/t7510-signed-commit.sh: Add %GP to custom format checks
     -:  ---------- >  7:  1e690847d1 t/t7510-signed-commit.sh: add signing subkey to Eris Discordia key
     9:  d13ecb7d81 !  8:  d8ad847421 Add a base implementation of SHA-256 support
    10:  3f0382eef8 =  9:  cdae1d391c sha256: add an SHA-256 implementation using libgcrypt
    11:  2422fd4227 = 10:  7d81aa0857 hash: add an SHA-256 implementation using OpenSSL

That would print a total of 44 lines of output, but the full range-diff output with --patch is 460 lines.

I thought of implementing --stat too. It would be neat if passing DIFF_FORMAT_DIFFSTAT just worked, but using that shows the underlying implementation details of how range-diff works, instead of a useful diffstat. So I'll leave that to a future change. Such a feature should be something like a textual summary of the --patch output itself, e.g.:

N hunks, X insertions(+), Y deletions(-)

This change doesn't update git-format-patch with a --no-patch option. That can be added later similar to how format-patch first learned --range-diff, and then --creation-factor in 8631bf1cdd ("format-patch: add --creation-factor tweak for --range-diff", 2018-07-22). I don't see why anyone would want this for format-patch, it pretty much defeats the point of range-diff.

---
 Documentation/git-range-diff.txt |  4 +++
 builtin/log.c                    |  2 +-
 builtin/range-diff.c             |  5 +++-
 log-tree.c                       |  2 +-
 range-diff.c                     |  6 ++++-
 range-diff.h                     |  1 +
 t/t3206-range-diff.sh            | 45 ++++++++++++++++++++++++++++++++
 7 files changed, 61 insertions(+), 4 deletions(-)

index f693930fdb..d0473cbcb1 100644
@@ -57,6 +57,10 @@ to revert to color all lines according to the outer diff markers
 	See the ``Algorithm`` section below for an explanation why this is
 	needed.
 
+--no-patch::
+	Don't show the range-diff itself, only which patches are the
+	same or were added or removed etc.
+
 <range1> <range2>::
 	Compare the commits specified by the two ranges, where
 	`<range1>` is considered an older version of `<range2>`.

index 061d4fd864..e063bcf2dd 100644
@@ -1096,7 +1096,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	if (rev->rdiff1) {
 		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
 		show_range_diff(rev->rdiff1, rev->rdiff2,
-				rev->creation_factor, 1, &rev->diffopt);
+				rev->creation_factor, 1, 1, &rev->diffopt);
 	}
 }
 

index f01a0be851..70c2761751 100644
@@ -16,11 +16,14 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 	int creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
 	struct diff_options diffopt = { NULL };
 	int simple_color = -1;
+	int patch = 1;
 	struct option options[] = {
 		OPT_INTEGER(0, "creation-factor", &creation_factor,
 			    N_("Percentage by which creation is weighted")),
 		OPT_BOOL(0, "no-dual-color", &simple_color,
 			    N_("use simple diff colors")),
+		OPT_BOOL('p', "patch", &patch,
+			 N_("show patch output")),
 		OPT_END()
 	};
 	int i, j, res = 0;

@@ -92,7 +95,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 	}
 
 	res = show_range_diff(range1.buf, range2.buf, creation_factor,
-			      simple_color < 1, &diffopt);
+			      simple_color < 1, patch, &diffopt);
 
 	strbuf_release(&range1);
 	strbuf_release(&range2);

index 7a83e99250..843e3ef83b 100644
@@ -762,7 +762,7 @@ void show_log(struct rev_info *opt)
 		next_commentary_block(opt, NULL);
 		fprintf_ln(opt->diffopt.file, "%s", opt->rdiff_title);
 		show_range_diff(opt->rdiff1, opt->rdiff2,
-				opt->creation_factor, 1, &opt->diffopt);
+				opt->creation_factor, 1, 1, &opt->diffopt);
 
 		memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));
 	}

index bd8083f2d1..c1bfa593ce 100644
@@ -436,6 +436,7 @@ static struct strbuf *output_prefix_cb(struct diff_options *opt, void *data)
 
 int show_range_diff(const char *range1, const char *range2,
 		    int creation_factor, int dual_color,
+		    int patch,
 		    struct diff_options *diffopt)
 {
 	int res = 0;

@@ -453,7 +454,10 @@ int show_range_diff(const char *range1, const char *range2,
 		struct strbuf indent = STRBUF_INIT;
 
 		memcpy(&opts, diffopt, sizeof(opts));
-		opts.output_format = DIFF_FORMAT_PATCH;
+		if (patch)
+			opts.output_format = DIFF_FORMAT_PATCH;
+		else
+			opts.output_format = DIFF_FORMAT_NO_OUTPUT;
 		opts.flags.suppress_diff_headers = 1;
 		opts.flags.dual_color_diffed_diffs = dual_color;
 		opts.output_prefix = output_prefix_cb;

index 190593f0c7..99bbc1cd9f 100644
@@ -7,6 +7,7 @@
 
 int show_range_diff(const char *range1, const char *range2,
 		    int creation_factor, int dual_color,
+		    int patch,
 		    struct diff_options *diffopt);
 
 #endif

2.19.1.930.g4563a0d9d0

This option existed in the very first version[1] of range-diff (then called branch-diff) implemented by Dscho, although it was called --no-patches (with an "es"), which it inherited from tbdiff. I think someone (possibly me) pointed out that --no-patch (sans "es") would be more consistent with existing Git options. I don't recall why Dscho removed the option during the re-rolls, but the explanation may be in that thread.

I was also wondering if --summarize or --summary-only might be a better name, describing the behavior at a higher level, but since there is precedent for --no-patch (or --no-patches in tbdiff), perhaps the name is fine as is.

The patch itself looks okay.

[1]: https://public-inbox.org/git/8bc517e35d4842f8d9d98f3b99adb9475d6db2d2.1525361419.git.johannes.schindelin@gmx.de/

Thanks for digging. Big thread, not going to re-read it now. I'd just like to have this.

I think we should aim to keep a 1=1 mapping between range-diff and log/show options when possible, even though the output might have a slightly different flavor as my 4th paragraph discussing a potential --stat talks about.

E.g. I can imagine that range-diff --no-patch --stat --summary would not show the patch, but a stat as described there, plus e.g. a "create mode..." if applicable.

This change implements only a tiny fraction of that, but it would be very neat if we supported more stuff, and showed it in range-diff-y way, e.g. some compact format showing:

    1 file changed, 3->2 insertions(+), 10->9 deletions(-)
    create mode 100(6 -> 7)44 new-executable

Hi,

In my hands, the well-documented -s option works (see e.g.
https://git-scm.com/docs/git-diff#git-diff--s), although I have to admit that the git-range-diff manual does not talk about the diff-options.

And for the record, for me, git range-diff A...B --no-patch *already* works.

Ciao,
Dscho

Neither of those works for me without my patch. E.g.

    ./git-range-diff -s 711aaa392f...a5ba8f2101
    ./git-range-diff --no-patch 711aaa392f...a5ba8f2101

This is on current next, 2.19.1.1182.g4ecb1133ce. What version are you on?

Hi Ævar,

I tried it with git version 2.19.0.windows.1.

To verify, I repeated this with next (git version 2.19.1.1215.g8438c0b2453a):

./git range-diff -s 711aaa392f...a5ba8f2101
fatal: unrecognized argument: --output-indicator-new=>
error: could not parse log for 'a5ba8f2101..711aaa392f'

Which means that something broke rather dramatically between v2.19.0.windows.1 and 8438c0b2453a.

Ciao,
Dscho

Hi Ævar,

Nevermind, this was solved by passing --exec-path=$PWD. And *now* I can reproduce your finding.

Ciao,
Dscho

Hi Ævar,

I've bisected this breakage down to 73a834e9e279 (range-diff: relieve callers of low-level configuration burden, 2018-07-22). Eric, that's one of your commits.

Ciao,
Dscho

Me again,

Okay, so I would really like to point you to this particular paragraph in the manual page of git range-diff (just below
https://git-scm.com/docs/git-range-diff#git-range-diff-ltbasegtltrev1gtltrev2gt):

git range-diff also accepts the regular diff options (see linkgit:git-diff[1]), most notably the --color=[<when>] and --no-color options. These options are used when generating the "diff between patches", i.e. to compare the author, commit message and diff of corresponding old/new commits. There is currently no means to tweak the diff options passed to git log when generating those patches.

So this was quite intentional, in particular with an eye on --no-patch. Note also the commit message of c8c5e43ac3f9 (range-diff: also show the diff between patches, 2018-08-13):

Note also: while tbdiff accepts the --no-patches option to suppress these diffs between patches, we prefer the -s (or --no-patch) option that is automatically supported via our use of diff_opt_parse().

This was very, very intentional, as you can see, and it was pretty broken by 73a834e. This here patch papers over that breakage, sadly I have too much on my plate as it is, so I cannot wrap it up in a nice commit (nor add a regression test, but you did that, nor investigate what else is broken) and therefore I would be indebted if you could take this in your custody:

index 3dd2edda0176..014112ee401f 100644
@@ -433,7 +433,8 @@ int show_range_diff(const char *range1, const char *range2,
 		struct strbuf indent = STRBUF_INIT;
 
 		memcpy(&opts, diffopt, sizeof(opts));
-		opts.output_format = DIFF_FORMAT_PATCH;
+		if (!opts.output_format)
+			opts.output_format = DIFF_FORMAT_PATCH;
 		opts.flags.suppress_diff_headers = 1;
 		opts.flags.dual_color_diffed_diffs = dual_color;
 		opts.output_prefix = output_prefix_cb;

Ciao,
Dscho

As Johannes notes this --no-patch option I wanted to add is something we had already, but is it turns out it was broken.

So this is an entirely rewritten v3 (not bothering with the range-diff for it) which a) documents the output stability of stuff like --stat and the like (there isn't any) b) fixes the regression & adds a test.

I did try various other diff options and they all seem to work.

Ævar Arnfjörð Bjarmason (2):
range-diff doc: add a section about output stability
range-diff: fix regression in passing along diff options

 Documentation/git-range-diff.txt | 17 +++++++++++++++++
 range-diff.c                     |  3 ++-
 t/t3206-range-diff.sh            | 30 ++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+), 1 deletion(-)

2.19.1.930.g4563a0d9d0

The range-diff command is already advertised as porcelain, but let's make it really clear that the output is completely subject to change, particularly when it comes to diff options such as --stat. Right now that option doesn't work, but fixing that is the subject of a later change.

---
 Documentation/git-range-diff.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

index f693930fdb..bbd07a9be8 100644
@@ -78,6 +78,23 @@ between patches", i.e. to compare the author, commit message and diff of
 corresponding old/new commits. There is currently no means to tweak the
 diff options passed to `git log` when generating those patches.
 
+OUTPUT STABILITY
+----------------
+
+The output of the `range-diff` command is subject to change. It is
+intended to be human-readable porcelain output, not something that can
+be used across versions of Git to get a textually stable `range-diff`
+(as opposed to something like the `--stable` option to
+linkgit:git-patch-id[1]). There's also no equivalent of
+linkgit:git-apply[1] for `range-diff`, the output is not intended to
+be machine-readable.
+
+This is particularly true when passing in diff options. Currently some
+options like `--stat` can as an emergent effect produce output that's
+quite useless in the context of `range-diff`. Future versions of
+`range-diff` may learn to interpret such options in a manner specifc
+to `range-diff` (e.g. for `--stat` summarizing how the diffstat
+changed).
 
 CONFIGURATION
 -------------

2.19.1.930.g4563a0d9d0

A pair of comma around "as an ... effect" would make it a bit more readable. It also took me four reads before I convinced myself that the original wants to say "Some options may just do whatever they happen to do that result in pretty useless output".

In 73a834e9e2 ("range-diff: relieve callers of low-level configuration burden", 2018-07-22) we broke passing down options like --no-patch, --stat etc. Fix that regression, and add a test for some of these options being passed down.

As noted in a change leading up to this ("range-diff doc: add a section about output stability", 2018-11-07) the output is not meant to be stable. So this regression test will likely need to be tweaked once we get a "proper" --stat option.

See
https://public-inbox.org/git/[email protected]/
for a further explanation of the regression.

The quoting of "EOF" here mirrors that of an earlier test. Perhaps that should be fixed, but let's leave that up to a later cleanup change.

---
 range-diff.c          |  3 ++-
 t/t3206-range-diff.sh | 30 ++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

index bd8083f2d1..488844c0af 100644
@@ -453,7 +453,8 @@ int show_range_diff(const char *range1, const char *range2,
 		struct strbuf indent = STRBUF_INIT;
 
 		memcpy(&opts, diffopt, sizeof(opts));
-		opts.output_format = DIFF_FORMAT_PATCH;
+		if (!opts.output_format)
+			opts.output_format = DIFF_FORMAT_PATCH;
 		opts.flags.suppress_diff_headers = 1;
 		opts.flags.dual_color_diffed_diffs = dual_color;
 		opts.output_prefix = output_prefix_cb;

index 6aae364171..e497c1358f 100755
@@ -122,6 +122,36 @@ test_expect_success 'changed commit' '
 	test_cmp expected actual
 '
 
+test_expect_success 'changed commit with --no-patch diff option' '
+	git range-diff --no-color --no-patch topic...changed >actual &&
+	cat >expected <<-EOF &&
+	1:  4de457d = 1:  a4b3333 s/5/A/
+	2:  fccce22 = 2:  f51d370 s/4/A/
+	3:  147e64e ! 3:  0559556 s/11/B/
+	4:  a63e992 ! 4:  d966c5c s/12/B/
+	EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'changed commit with --stat diff option' '
+	git range-diff --no-color --stat topic...changed >actual &&
+	cat >expected <<-EOF &&
+	1:  4de457d = 1:  a4b3333 s/5/A/
+	     a => b | 0
+	     1 file changed, 0 insertions(+), 0 deletions(-)
+	2:  fccce22 = 2:  f51d370 s/4/A/
+	     a => b | 0
+	     1 file changed, 0 insertions(+), 0 deletions(-)
+	3:  147e64e ! 3:  0559556 s/11/B/
+	     a => b | 0
+	     1 file changed, 0 insertions(+), 0 deletions(-)
+	4:  a63e992 ! 4:  d966c5c s/12/B/
+	     a => b | 0
+	     1 file changed, 0 insertions(+), 0 deletions(-)
+	EOF
+	test_cmp expected actual
+'
+
 test_expect_success 'changed commit with sm config' '
 	git range-diff --no-color --submodule=log topic...changed >actual &&
 	cat >expected <<-EOF &&

2.19.1.930.g4563a0d9d0

Thanks both (Ævar and Dscho) for cleaning up my mess, and sorry for not responding sooner; I only just found time to read the discussion thread. One comment below...

    opts.output_format |= DIFF_FORMAT_PATCH;

Maybe I'm misunderstanding, but if you mean this on top:

    diff --git a/range-diff.c b/range-diff.c
    index 488844c0af..ea317f92f9 100644
    --- a/range-diff.c
    +++ b/range-diff.c
    @@ -453,8 +453,7 @@ int show_range_diff(const char *range1, const char *range2,
                    struct strbuf indent = STRBUF_INIT;

                    memcpy(&opts, diffopt, sizeof(opts));
    -               if (!opts.output_format)
    -                       opts.output_format = DIFF_FORMAT_PATCH;
    +               opts.output_format |= DIFF_FORMAT_PATCH;
                    opts.flags.suppress_diff_headers = 1;
                    opts.flags.dual_color_diffed_diffs = dual_color;
                    opts.output_prefix = output_prefix_cb;

Then the --stat test I've added here fails, because unlike "diff" the "--stat" (and others) will implicitly "--patch" and you need "--no-patch" as well (again, unlike with "diff").

Right now --stat is pretty useless, but it could be made to make sense, and at that point (and earlier) I think it would be confusing if "range-diff" had different semantics with no options v.s. one option like "--stat" v.s. "--stat -p" compared to "diff".

That is indeed what I mean.

This new --stat test also fails with Dscho's original git-range-diff implementation, even before 73a834e9e2 regressed the --no-patch case. The commit message seems to sell this patch as a pure regression-fix, so it feels wrong for it to be conflating a regression fix (--no-patch) with preparation for potential future improvements to other options (--stat, etc.).

I could see this as a two-patch series in which patch 1/2 fixes the regression (with, say, "|="), and patch 2/2 generalizes setting 'opts.output_format' for the future. Alternately, if left as a single patch, perhaps the commit message could be fleshed out to better explain that it is doing more than merely fixing a regression (since it wasn't at all obvious to me, even after digging into the code earlier to come up with "|=", or now when trying to understand your response).

Perhaps this sort of rationale, along with some explanatory examples could be added to the commit message to help the reader more fully understand the situation.

Thanks for working on this.

*Nod*

Yeah that makes sense. I did not see (but see now) that the --stat behavior was different now v.s. before your 73a834e9e2.

*Nod*

Addresses feedback on v3, especially Eric's suggestion to split out the behavior change (which I was not aware of) into a 3/3. Diff with v3:

1: 23295d7806 ! 1: 5399e57513 range-diff doc: add a section about output stability
@@ -29,11 +29,11 @@
+be machine-readable.
+
+This is particularly true when passing in diff options. Currently some
-+options like --stat can as an emergent effect produce output that's -+quite useless in the context of range-diff. Future versions of -+range-diff may learn to interpret such options in a manner specifc -+to range-diff (e.g. for --stat summarizing how the diffstat -+changed).
++options like --stat can, as an emergent effect, produce output ++that's quite useless in the context of range-diff. Future versions ++of range-diff may learn to interpret such options in a manner ++specific to range-diff (e.g. for --stat producing human-readable ++output which summarizes how the diffstat changed).

CONFIGURATION
-------------
2: b21bd273f5 ! 2: e56975df6c range-diff: fix regression in passing along diff options

    @@ -4,8 +4,10 @@
     
         In 73a834e9e2 ("range-diff: relieve callers of low-level configuration
         burden", 2018-07-22) we broke passing down options like --no-patch,
    -    --stat etc. Fix that regression, and add a test for some of these
    -    options being passed down.
    +    --stat etc.
    +
    +    Fix that regression, and add a test asserting the pre-73a834e9e2
    +    behavior for some of these diff options.
     
         As noted in a change leading up to this ("range-diff doc: add a
         section about output stability", 2018-11-07) the output is not meant
    @@ -14,7 +16,9 @@
     
         See
         https://public-inbox.org/git/[email protected]/
    -    for a further explanation of the regression.
    +    for a further explanation of the regression. The fix here is not the
    +    same as in Johannes's on-list patch, for reasons that'll be explained
    +    in a follow-up commit.
     
         The quoting of "EOF" here mirrors that of an earlier test. Perhaps
         that should be fixed, but let's leave that up to a later cleanup
    @@ -30,8 +34,7 @@
      
      		memcpy(&opts, diffopt, sizeof(opts));
     -		opts.output_format = DIFF_FORMAT_PATCH;
    -+		if (!opts.output_format)
    -+			opts.output_format = DIFF_FORMAT_PATCH;
    ++		opts.output_format |= DIFF_FORMAT_PATCH;
      		opts.flags.suppress_diff_headers = 1;
      		opts.flags.dual_color_diffed_diffs = dual_color;
      		opts.output_prefix = output_prefix_cb;
    @@ -55,20 +58,43 @@
     +'
     +
     +test_expect_success 'changed commit with --stat diff option' '
    ++	four_spaces="    " &&
     +	git range-diff --no-color --stat topic...changed >actual &&
     +	cat >expected <<-EOF &&
     +	1:  4de457d = 1:  a4b3333 s/5/A/
     +	     a => b | 0
     +	     1 file changed, 0 insertions(+), 0 deletions(-)
    ++	$four_spaces
     +	2:  fccce22 = 2:  f51d370 s/4/A/
     +	     a => b | 0
     +	     1 file changed, 0 insertions(+), 0 deletions(-)
    ++	$four_spaces
     +	3:  147e64e ! 3:  0559556 s/11/B/
     +	     a => b | 0
     +	     1 file changed, 0 insertions(+), 0 deletions(-)
    ++	$four_spaces
    ++	    @@ -10,7 +10,7 @@
    ++	      9
    ++	      10
    ++	     -11
    ++	    -+B
    ++	    ++BB
    ++	      12
    ++	      13
    ++	      14
     +	4:  a63e992 ! 4:  d966c5c s/12/B/
     +	     a => b | 0
     +	     1 file changed, 0 insertions(+), 0 deletions(-)
    ++	$four_spaces
    ++	    @@ -8,7 +8,7 @@
    ++	     @@
    ++	      9
    ++	      10
    ++	    - B
    ++	    + BB
    ++	     -12
    ++	     +B
    ++	      13
     +	EOF
     +	test_cmp expected actual
     +'

-: ---------- > 3: edfef733c7 range-diff: make diff option behavior (e.g. --stat) consistent

Ævar Arnfjörð Bjarmason (3):
range-diff doc: add a section about output stability
range-diff: fix regression in passing along diff options
range-diff: make diff option behavior (e.g. --stat) consistent

 Documentation/git-range-diff.txt | 17 +++++++++++++++++
 range-diff.c                     |  3 ++-
 t/t3206-range-diff.sh            | 31 +++++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 1 deletion(-)

2.19.1.1182.g4ecb1133ce

Hi Ævar,

For the record, I am fine with this iteration, too.

Ciao,
Dscho

The range-diff command is already advertised as porcelain, but let's make it really clear that the output is completely subject to change, particularly when it comes to diff options such as --stat. Right now that option doesn't work, but fixing that is the subject of a later change.

---
 Documentation/git-range-diff.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

index f693930fdb..8a6ea2c6c5 100644
@@ -78,6 +78,23 @@ between patches", i.e. to compare the author, commit message and diff of
 corresponding old/new commits. There is currently no means to tweak the
 diff options passed to `git log` when generating those patches.
 
+OUTPUT STABILITY
+----------------
+
+The output of the `range-diff` command is subject to change. It is
+intended to be human-readable porcelain output, not something that can
+be used across versions of Git to get a textually stable `range-diff`
+(as opposed to something like the `--stable` option to
+linkgit:git-patch-id[1]). There's also no equivalent of
+linkgit:git-apply[1] for `range-diff`, the output is not intended to
+be machine-readable.
+
+This is particularly true when passing in diff options. Currently some
+options like `--stat` can, as an emergent effect, produce output
+that's quite useless in the context of `range-diff`. Future versions
+of `range-diff` may learn to interpret such options in a manner
+specific to `range-diff` (e.g. for `--stat` producing human-readable
+output which summarizes how the diffstat changed).
 
 CONFIGURATION
 -------------

2.19.1.1182.g4ecb1133ce

In 73a834e9e2 ("range-diff: relieve callers of low-level configuration burden", 2018-07-22) we broke passing down options like --no-patch, --stat etc.

Fix that regression, and add a test asserting the pre-73a834e9e2 behavior for some of these diff options.

As noted in a change leading up to this ("range-diff doc: add a section about output stability", 2018-11-07) the output is not meant to be stable. So this regression test will likely need to be tweaked once we get a "proper" --stat option.

See
https://public-inbox.org/git/[email protected]/
for a further explanation of the regression. The fix here is not the same as in Johannes's on-list patch, for reasons that'll be explained in a follow-up commit.

The quoting of "EOF" here mirrors that of an earlier test. Perhaps that should be fixed, but let's leave that up to a later cleanup change.

---
 range-diff.c          |  2 +-
 t/t3206-range-diff.sh | 53 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 1 deletion(-)

index bd8083f2d1..ea317f92f9 100644
@@ -453,7 +453,7 @@ int show_range_diff(const char *range1, const char *range2,
 		struct strbuf indent = STRBUF_INIT;
 
 		memcpy(&opts, diffopt, sizeof(opts));
-		opts.output_format = DIFF_FORMAT_PATCH;
+		opts.output_format |= DIFF_FORMAT_PATCH;
 		opts.flags.suppress_diff_headers = 1;
 		opts.flags.dual_color_diffed_diffs = dual_color;
 		opts.output_prefix = output_prefix_cb;

2.19.1.1182.g4ecb1133ce

Make the behavior when diff options (e.g. "--stat") are passed consistent with how "diff" behaves.

Before 73a834e9e2 ("range-diff: relieve callers of low-level configuration burden", 2018-07-22) running range-diff with "--stat" would produce stat output and the diff output, as opposed to how "diff" behaves where once "--stat" is specified "--patch" also needs to be provided to emit the patch output.

As noted in a previous change ("range-diff doc: add a section about output stability", 2018-11-07) the "--stat" output with "range-diff" is useless at the moment.

But we should behave consistently with "diff" in anticipation of such output being useful in the future, because it would make for confusing UI if two "diff" and "range-diff" behaved differently when it came to how they interpret diff options.

The new behavior is also consistent with the existing documentation added in ba931edd28 ("range-diff: populate the man page", 2018-08-13). See "[...]also accepts the regular diff options[...]" in git-range-diff(1).

---
 range-diff.c          |  3 ++-
 t/t3206-range-diff.sh | 22 ----------------------
 2 files changed, 2 insertions(+), 23 deletions(-)

index ea317f92f9..72bde281f3 100644
@@ -453,7 +453,8 @@ int show_range_diff(const char *range1, const char *range2,
 		struct strbuf indent = STRBUF_INIT;
 
 		memcpy(&opts, diffopt, sizeof(opts));
-		opts.output_format |= DIFF_FORMAT_PATCH;
+		if (!opts.output_format)
+			opts.output_format |= DIFF_FORMAT_PATCH;
 		opts.flags.suppress_diff_headers = 1;
 		opts.flags.dual_color_diffed_diffs = dual_color;
 		opts.output_prefix = output_prefix_cb;

index ab44e085d5..9352f65280 100755
@@ -140,37 +140,15 @@ test_expect_success 'changed commit with --stat diff option' '
 	1:  4de457d = 1:  a4b3333 s/5/A/
 	     a => b | 0
 	     1 file changed, 0 insertions(+), 0 deletions(-)
-	$four_spaces
 	2:  fccce22 = 2:  f51d370 s/4/A/
 	     a => b | 0
 	     1 file changed, 0 insertions(+), 0 deletions(-)
-	$four_spaces
 	3:  147e64e ! 3:  0559556 s/11/B/
 	     a => b | 0
 	     1 file changed, 0 insertions(+), 0 deletions(-)
-	$four_spaces
-	    @@ -10,7 +10,7 @@
-	      9
-	      10
-	     -11
-	    -+B
-	    ++BB
-	      12
-	      13
-	      14
 	4:  a63e992 ! 4:  d966c5c s/12/B/
 	     a => b | 0
 	     1 file changed, 0 insertions(+), 0 deletions(-)
-	$four_spaces
-	    @@ -8,7 +8,7 @@
-	     @@
-	      9
-	      10
-	    - B
-	    + BB
-	     -12
-	     +B
-	      13
 	EOF
 	test_cmp expected actual
 '

2.19.1.1182.g4ecb1133ce

's/ two//'

Hmph, could the condition in the future change to

	-	if (!opts.output_format)
	+	if (! (opts.output_format & DIFF_FORMAT_MASK))
			opts.output_format |= DIFF_FORMAT_PATCH

if we ever gain a new "output_format" bit that does not affect how we show the diff in a major way, and that is on by default? If so, I think "|=" is more future-proof. Otherwise, "=" is indeed more clear way to spell the intention.

Trivial updates since v4 addressing the feedback on that iteration. Hopefully this is the last one, range-diff with the last version:

1:  5399e57513 = 1:  f225173f43 range-diff doc: add a section about output stability
2:  e56975df6c = 2:  77804ac641 range-diff: fix regression in passing along diff options
3:  edfef733c7 ! 3:  ed67dba073 range-diff: make diff option behavior (e.g. --stat) consistent
    @@ -17,8 +17,8 @@
     
         But we should behave consistently with "diff" in anticipation of such
         output being useful in the future, because it would make for confusing
    -    UI if two "diff" and "range-diff" behaved differently when it came to
    -    how they interpret diff options.
    +    UI if "diff" and "range-diff" behaved differently when it came to how
    +    they interpret diff options.
     
         The new behavior is also consistent with the existing documentation
         added in ba931edd28 ("range-diff: populate the man page",
    @@ -36,7 +36,7 @@
      		memcpy(&opts, diffopt, sizeof(opts));
     -		opts.output_format |= DIFF_FORMAT_PATCH;
     +		if (!opts.output_format)
    -+			opts.output_format |= DIFF_FORMAT_PATCH;
    ++			opts.output_format = DIFF_FORMAT_PATCH;
      		opts.flags.suppress_diff_headers = 1;
      		opts.flags.dual_color_diffed_diffs = dual_color;
      		opts.output_prefix = output_prefix_cb;
    @@ -45,6 +45,12 @@
      --- a/t/t3206-range-diff.sh
      +++ b/t/t3206-range-diff.sh
     @@
    + '
    + 
    + test_expect_success 'changed commit with --stat diff option' '
    +-	four_spaces="    " &&
    + 	git range-diff --no-color --stat topic...changed >actual &&
    + 	cat >expected <<-EOF &&
      	1:  4de457d = 1:  a4b3333 s/5/A/
      	     a => b | 0
      	     1 file changed, 0 insertions(+), 0 deletions(-)

Ævar Arnfjörð Bjarmason (3):
range-diff doc: add a section about output stability
range-diff: fix regression in passing along diff options
range-diff: make diff option behavior (e.g. --stat) consistent

 Documentation/git-range-diff.txt | 17 +++++++++++++++++
 range-diff.c                     |  3 ++-
 t/t3206-range-diff.sh            | 30 ++++++++++++++++++++++++++++++
 3 files changed, 49 insertions(+), 1 deletion(-)

2.19.1.1182.g4ecb1133ce

Hi Ævar,

This range-diff looks good to me.

Thanks,
Dscho

The range-diff command is already advertised as porcelain, but let's make it really clear that the output is completely subject to change, particularly when it comes to diff options such as --stat. Right now that option doesn't work, but fixing that is the subject of a later change.

---
 Documentation/git-range-diff.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

index f693930fdb..8a6ea2c6c5 100644
@@ -78,6 +78,23 @@ between patches", i.e. to compare the author, commit message and diff of
 corresponding old/new commits. There is currently no means to tweak the
 diff options passed to `git log` when generating those patches.
 
+OUTPUT STABILITY
+----------------
+
+The output of the `range-diff` command is subject to change. It is
+intended to be human-readable porcelain output, not something that can
+be used across versions of Git to get a textually stable `range-diff`
+(as opposed to something like the `--stable` option to
+linkgit:git-patch-id[1]). There's also no equivalent of
+linkgit:git-apply[1] for `range-diff`, the output is not intended to
+be machine-readable.
+
+This is particularly true when passing in diff options. Currently some
+options like `--stat` can, as an emergent effect, produce output
+that's quite useless in the context of `range-diff`. Future versions
+of `range-diff` may learn to interpret such options in a manner
+specific to `range-diff` (e.g. for `--stat` producing human-readable
+output which summarizes how the diffstat changed).
 
 CONFIGURATION
 -------------

2.19.1.1182.g4ecb1133ce

In 73a834e9e2 ("range-diff: relieve callers of low-level configuration burden", 2018-07-22) we broke passing down options like --no-patch, --stat etc.

Fix that regression, and add a test asserting the pre-73a834e9e2 behavior for some of these diff options.

As noted in a change leading up to this ("range-diff doc: add a section about output stability", 2018-11-07) the output is not meant to be stable. So this regression test will likely need to be tweaked once we get a "proper" --stat option.

See
https://public-inbox.org/git/[email protected]/
for a further explanation of the regression. The fix here is not the same as in Johannes's on-list patch, for reasons that'll be explained in a follow-up commit.

The quoting of "EOF" here mirrors that of an earlier test. Perhaps that should be fixed, but let's leave that up to a later cleanup change.

---
 range-diff.c          |  2 +-
 t/t3206-range-diff.sh | 53 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 1 deletion(-)

index 3958720f00..ec937008d0 100644
@@ -461,7 +461,7 @@ int show_range_diff(const char *range1, const char *range2,
 		struct strbuf indent = STRBUF_INIT;
 
 		memcpy(&opts, diffopt, sizeof(opts));
-		opts.output_format = DIFF_FORMAT_PATCH;
+		opts.output_format |= DIFF_FORMAT_PATCH;
 		opts.flags.suppress_diff_headers = 1;
 		opts.flags.dual_color_diffed_diffs = dual_color;
 		opts.output_prefix = output_prefix_cb;

2.19.1.1182.g4ecb1133ce

Make the behavior when diff options (e.g. "--stat") are passed consistent with how "diff" behaves.

Before 73a834e9e2 ("range-diff: relieve callers of low-level configuration burden", 2018-07-22) running range-diff with "--stat" would produce stat output and the diff output, as opposed to how "diff" behaves where once "--stat" is specified "--patch" also needs to be provided to emit the patch output.

As noted in a previous change ("range-diff doc: add a section about output stability", 2018-11-07) the "--stat" output with "range-diff" is useless at the moment.

But we should behave consistently with "diff" in anticipation of such output being useful in the future, because it would make for confusing UI if "diff" and "range-diff" behaved differently when it came to how they interpret diff options.

The new behavior is also consistent with the existing documentation added in ba931edd28 ("range-diff: populate the man page", 2018-08-13). See "[...]also accepts the regular diff options[...]" in git-range-diff(1).

---
 range-diff.c          |  3 ++-
 t/t3206-range-diff.sh | 23 -----------------------
 2 files changed, 2 insertions(+), 24 deletions(-)

index ec937008d0..767af8c5bb 100644
@@ -461,7 +461,8 @@ int show_range_diff(const char *range1, const char *range2,
 		struct strbuf indent = STRBUF_INIT;
 
 		memcpy(&opts, diffopt, sizeof(opts));
-		opts.output_format |= DIFF_FORMAT_PATCH;
+		if (!opts.output_format)
+			opts.output_format = DIFF_FORMAT_PATCH;
 		opts.flags.suppress_diff_headers = 1;
 		opts.flags.dual_color_diffed_diffs = dual_color;
 		opts.output_prefix = output_prefix_cb;

index ab44e085d5..e497c1358f 100755
@@ -134,43 +134,20 @@ test_expect_success 'changed commit with --no-patch diff option' '
 '
 
 test_expect_success 'changed commit with --stat diff option' '
-	four_spaces="    " &&
 	git range-diff --no-color --stat topic...changed >actual &&
 	cat >expected <<-EOF &&
 	1:  4de457d = 1:  a4b3333 s/5/A/
 	     a => b | 0
 	     1 file changed, 0 insertions(+), 0 deletions(-)
-	$four_spaces
 	2:  fccce22 = 2:  f51d370 s/4/A/
 	     a => b | 0
 	     1 file changed, 0 insertions(+), 0 deletions(-)
-	$four_spaces
 	3:  147e64e ! 3:  0559556 s/11/B/
 	     a => b | 0
 	     1 file changed, 0 insertions(+), 0 deletions(-)
-	$four_spaces
-	    @@ -10,7 +10,7 @@
-	      9
-	      10
-	     -11
-	    -+B
-	    ++BB
-	      12
-	      13
-	      14
 	4:  a63e992 ! 4:  d966c5c s/12/B/
 	     a => b | 0
 	     1 file changed, 0 insertions(+), 0 deletions(-)
-	$four_spaces
-	    @@ -8,7 +8,7 @@
-	     @@
-	      9
-	      10
-	    - B
-	    + BB
-	     -12
-	     +B
-	      13
 	EOF
 	test_cmp expected actual
 '

2.19.1.1182.g4ecb1133ce

I am OK not to have this option integrated to format-patch from day one, but I do not think it is a good idea to hint that it should not be done later.

Does it defeats the point of range-diff to omit the patch part in the context of the cover letter? How?

I think the output with this option is a good addition to the cover letter as an abbreviated form (as opposed to the full range-diff, whose support was added earlier) that gives an overview.

Calling this --[no-]patch might make it harder to integrate it to format-patch later, though. I suspect that people would expect "format-patch --no-patch ..." to omit both the patch part of the range-diff output *AND* the patch that should be applied to the codebase (it of course would defeat the point of format-patch, so today's format-patch would not pay attention to --no-patch, of course). We need to be careful not to break that when it happens.

I had the same response when reading the commit message but didn't vocalize it. I could see people wanting to suppress the 'patch' part of the embedded range-diff in a cover letter (though probably not as commentary in a single-patch).

Same concern on my side, which is why I was thinking of other, less confusing, names, such as --summarize or such, though even that is too general against the full set of git-format-patch options. It could, perhaps be a separate option, say, "git format-patch --range-changes=<prev>" or something, which would embed the equivalent of "git range-diff --no-patch <prev>...<current>" in the cover letter.

I actually am perfectly fine with --no-patch. My "concern" was merely that we should be careful to make sure that we do not stop producing patches when we plug this patch to format-patch when "format-patch --no-patch" is given; rather, it should only suppress the patch part of range-diff shown in the cover letter.

Maybe this was discussed more when this range-diff format-patch integration was submitted, I wasn't following that closely:

Looking at this more carefully it seems like quite a design limitation that we're conflating the options for format-patch itself and for the range-diff invocation it makes.

Wouldn't it be better to make all these options
e.g. --range-diff-creation-factor=*, --range-diff-no-patch, --range-diff-U1 etc. Now there's no way to say supply a different -U<ctx> for the range-diff & the patches themselves which seems like a semi-common use-case.

Doing that seems to be a matter of teaching setup_revisions() to accumulate unknown options, then parsing those into their own diffopts with the "range-diff-" prefix stripped and handing that data off to the range-diff machinery, not the parsed options for range-diff itself as happens now.

Add a --no-patch option which shows which changes got removed, added or moved etc., without showing the diff associated with them.

This allows for using range-diff as a poor man's "shortlog" for force-pushed branches to see what changed without getting into the details of what specifically. E.g. diffing the latest forced-push to "pu" gives us:

    $ ./git-range-diff --no-patch b58974365b...711aaa392f | head -n 10
     -:  ---------- >  1:  b613de67c4 diff: differentiate error handling in parse_color_moved_ws
    28:  c731affab0 !  2:  23c4bbe28e build: link with curl-defined linker flags
     -:  ---------- >  3:  14f74d5907 git-worktree.txt: correct linkgit command name
     -:  ---------- >  4:  29d51e214c sequencer.c: remove a stray semicolon
     -:  ---------- >  5:  b7845cebc0 tree-walk.c: fix overoptimistic inclusion in :(exclude) matching
     -:  ---------- >  6:  1a550529b1 t/t7510-signed-commit.sh: Add %GP to custom format checks
     -:  ---------- >  7:  1e690847d1 t/t7510-signed-commit.sh: add signing subkey to Eris Discordia key
     9:  d13ecb7d81 !  8:  d8ad847421 Add a base implementation of SHA-256 support
    10:  3f0382eef8 =  9:  cdae1d391c sha256: add an SHA-256 implementation using libgcrypt
    11:  2422fd4227 = 10:  7d81aa0857 hash: add an SHA-256 implementation using OpenSSL

That would print a total of 44 lines of output, but the full range-diff output with --patch is 460 lines.

I thought of implementing --stat too. It would be neat if passing DIFF_FORMAT_DIFFSTAT just worked, but using that shows the underlying implementation details of how range-diff works, instead of a useful diffstat. So I'll leave that to a future change. Such a feature should be something like a textual summary of the --patch output itself, e.g.:

N hunks, X insertions(+), Y deletions(-)

This change doesn't update git-format-patch with a --no-patch option. That can be added later similar to how format-patch first learned --range-diff, and then --creation-factor in 8631bf1cdd ("format-patch: add --creation-factor tweak for --range-diff", 2018-07-22). See [1] and related E-Mails for a discussion about that.

1. https://public-inbox.org/git/[email protected]/

---
Range-diff:
1: 6e735e991c ! 1: fe4e251f26 range-diff: add a --no-patch option to show a summary

    @@ -38,8 +38,10 @@
         option. That can be added later similar to how format-patch first
         learned --range-diff, and then --creation-factor in
         8631bf1cdd ("format-patch: add --creation-factor tweak for
    -    --range-diff", 2018-07-22). I don't see why anyone would want this for
    -    format-patch, it pretty much defeats the point of range-diff.
    +    --range-diff", 2018-07-22). See [1] and related E-Mails for a
    +    discussion about that.
    +
    +    1. https://public-inbox.org/git/[email protected]/
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <[email protected]>
     
    @@ -50,9 +52,10 @@
      	See the ``Algorithm`` section below for an explanation why this is
      	needed.
      
    ++-s::
     +--no-patch::
    -+	Don't show the range-diff itself, only which patches are the
    -+	same or were added or removed etc.
    ++	Suppress diff output. Only shows how the range has changed at
    ++	a commit-level.
     +
      <range1> <range2>::
      	Compare the commits specified by the two ranges, where
    @@ -78,14 +81,14 @@
      	int creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
      	struct diff_options diffopt = { NULL };
      	int simple_color = -1;
    -+	int patch = 1;
    ++	int no_patch = 0;
      	struct option options[] = {
      		OPT_INTEGER(0, "creation-factor", &creation_factor,
      			    N_("Percentage by which creation is weighted")),
      		OPT_BOOL(0, "no-dual-color", &simple_color,
      			    N_("use simple diff colors")),
    -+		OPT_BOOL('p', "patch", &patch,
    -+			 N_("show patch output")),
    ++		OPT_BOOL_F('s', "no-patch", &no_patch,
    ++			 N_("show patch output"), PARSE_OPT_NONEG),
      		OPT_END()
      	};
      	int i, j, res = 0;
    @@ -94,7 +97,7 @@
      
      	res = show_range_diff(range1.buf, range2.buf, creation_factor,
     -			      simple_color < 1, &diffopt);
    -+			      simple_color < 1, patch, &diffopt);
    ++			      simple_color < 1, !no_patch, &diffopt);
      
      	strbuf_release(&range1);
      	strbuf_release(&range2);
    @@ -155,7 +158,14 @@
      	test_cmp expected actual
      '
      
    -+test_expect_success 'changed commit --no-patch' '
    ++test_expect_success 'changed commit -p & --patch' '
    ++	git range-diff --no-color -p topic...changed >actual &&
    ++	test_cmp expected actual &&
    ++	git range-diff --no-color --patch topic...changed >actual &&
    ++	test_cmp expected actual
    ++'
    ++
    ++test_expect_success 'changed commit -s & --no-patch' '
     +	git range-diff --no-color --no-patch topic...changed >actual &&
     +	cat >expected <<-EOF &&
     +	1:  4de457d = 1:  a4b3333 s/5/A/
    @@ -163,6 +173,8 @@
     +	3:  147e64e ! 3:  0559556 s/11/B/
     +	4:  a63e992 ! 4:  d966c5c s/12/B/
     +	EOF
    ++	test_cmp expected actual &&
    ++	git range-diff --no-color -s topic...changed >actual &&
     +	test_cmp expected actual
     +'
     +

 Documentation/git-range-diff.txt |  5 +++
 builtin/log.c                    |  2 +-
 builtin/range-diff.c             |  5 ++-
 log-tree.c                       |  2 +-
 range-diff.c                     |  6 +++-
 range-diff.h                     |  1 +
 t/t3206-range-diff.sh            | 54 ++++++++++++++++++++++++++++++++
 7 files changed, 71 insertions(+), 4 deletions(-)

index f693930fdb..6c1eb647a1 100644
@@ -57,6 +57,11 @@ to revert to color all lines according to the outer diff markers
 	See the ``Algorithm`` section below for an explanation why this is
 	needed.
 
+-s::
+--no-patch::
+	Suppress diff output. Only shows how the range has changed at
+	a commit-level.
+
 <range1> <range2>::
 	Compare the commits specified by the two ranges, where
 	`<range1>` is considered an older version of `<range2>`.

index 061d4fd864..e063bcf2dd 100644
@@ -1096,7 +1096,7 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
 	if (rev->rdiff1) {
 		fprintf_ln(rev->diffopt.file, "%s", rev->rdiff_title);
 		show_range_diff(rev->rdiff1, rev->rdiff2,
-				rev->creation_factor, 1, &rev->diffopt);
+				rev->creation_factor, 1, 1, &rev->diffopt);
 	}
 }
 

index f01a0be851..05d1f6b6b6 100644
@@ -16,11 +16,14 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 	int creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
 	struct diff_options diffopt = { NULL };
 	int simple_color = -1;
+	int no_patch = 0;
 	struct option options[] = {
 		OPT_INTEGER(0, "creation-factor", &creation_factor,
 			    N_("Percentage by which creation is weighted")),
 		OPT_BOOL(0, "no-dual-color", &simple_color,
 			    N_("use simple diff colors")),
+		OPT_BOOL_F('s', "no-patch", &no_patch,
+			 N_("show patch output"), PARSE_OPT_NONEG),
 		OPT_END()
 	};
 	int i, j, res = 0;

@@ -92,7 +95,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 	}
 
 	res = show_range_diff(range1.buf, range2.buf, creation_factor,
-			      simple_color < 1, &diffopt);
+			      simple_color < 1, !no_patch, &diffopt);
 
 	strbuf_release(&range1);
 	strbuf_release(&range2);

index 7a83e99250..843e3ef83b 100644
@@ -762,7 +762,7 @@ void show_log(struct rev_info *opt)
 		next_commentary_block(opt, NULL);
 		fprintf_ln(opt->diffopt.file, "%s", opt->rdiff_title);
 		show_range_diff(opt->rdiff1, opt->rdiff2,
-				opt->creation_factor, 1, &opt->diffopt);
+				opt->creation_factor, 1, 1, &opt->diffopt);
 
 		memcpy(&diff_queued_diff, &dq, sizeof(diff_queued_diff));
 	}

index bd8083f2d1..c1bfa593ce 100644
@@ -436,6 +436,7 @@ static struct strbuf *output_prefix_cb(struct diff_options *opt, void *data)
 
 int show_range_diff(const char *range1, const char *range2,
 		    int creation_factor, int dual_color,
+		    int patch,
 		    struct diff_options *diffopt)
 {
 	int res = 0;

@@ -453,7 +454,10 @@ int show_range_diff(const char *range1, const char *range2,
 		struct strbuf indent = STRBUF_INIT;
 
 		memcpy(&opts, diffopt, sizeof(opts));
-		opts.output_format = DIFF_FORMAT_PATCH;
+		if (patch)
+			opts.output_format = DIFF_FORMAT_PATCH;
+		else
+			opts.output_format = DIFF_FORMAT_NO_OUTPUT;
 		opts.flags.suppress_diff_headers = 1;
 		opts.flags.dual_color_diffed_diffs = dual_color;
 		opts.output_prefix = output_prefix_cb;

index 190593f0c7..99bbc1cd9f 100644
@@ -7,6 +7,7 @@
 
 int show_range_diff(const char *range1, const char *range2,
 		    int creation_factor, int dual_color,
+		    int patch,
 		    struct diff_options *diffopt);
 
 #endif

2.19.1.930.g4563a0d9d0

Hi,

In light of my investigation that revealed that the original behavior (which is still documented in the manual page of range-diff) was broken, and I would much rather see that fixed than adding a workaround.

I would be fine with my patch being combined with the update to the manual page and the regression test, as a v3.

Ciao,
Dscho