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

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

[Ævar Arnfjörð Bjarmason]

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 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(-)

diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
index f693930fdb..d0473cbcb1 100644
--- a/Documentation/git-range-diff.txt
+++ b/Documentation/git-range-diff.txt
@@ -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>`.
diff --git a/builtin/log.c b/builtin/log.c
index 061d4fd864..e063bcf2dd 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -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);
 	}
 }
 
diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index f01a0be851..70c2761751 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -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);
diff --git a/log-tree.c b/log-tree.c
index 7a83e99250..843e3ef83b 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -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));
 	}
diff --git a/range-diff.c b/range-diff.c
index bd8083f2d1..c1bfa593ce 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -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;
diff --git a/range-diff.h b/range-diff.h
index 190593f0c7..99bbc1cd9f 100644
--- a/range-diff.h
+++ b/range-diff.h
@@ -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
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 6aae364171..8f8be0c57f 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -122,6 +122,17 @@ test_expect_success 'changed commit' '
 	test_cmp expected actual
 '
 
+test_expect_success 'changed commit --no-patch' '
+	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 sm config' '
 	git range-diff --no-color --submodule=log topic...changed >actual &&
 	cat >expected <<-EOF &&
@@ -151,6 +162,17 @@ test_expect_success 'changed commit with sm config' '
 	test_cmp expected actual
 '
 
+test_expect_success 'changed commit with sm config --no-patch' '
+	git range-diff --no-color --no-patch --submodule=log 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 'no commits on one side' '
 	git commit --amend -m "new message" &&
 	git range-diff master HEAD@{1} HEAD
@@ -176,6 +198,17 @@ test_expect_success 'changed message' '
 	test_cmp expected actual
 '
 
+test_expect_success 'changed message --no-patch' '
+	git range-diff --no-color --no-patch topic...changed-message >actual &&
+	sed s/Z/\ /g >expected <<-EOF &&
+	1:  4de457d = 1:  f686024 s/5/A/
+	2:  fccce22 ! 2:  4ab067d s/4/A/
+	3:  147e64e = 3:  b9cb956 s/11/B/
+	4:  a63e992 = 4:  8add5f1 s/12/B/
+	EOF
+	test_cmp expected actual
+'
+
 test_expect_success 'dual-coloring' '
 	sed -e "s|^:||" >expect <<-\EOF &&
 	:<YELLOW>1:  a4b3333 = 1:  f686024 s/5/A/<RESET>
@@ -215,6 +248,18 @@ test_expect_success 'dual-coloring' '
 	test_cmp expect actual
 '
 
+test_expect_success 'dual-coloring --no-patch' '
+	sed -e "s|^:||" >expect <<-\EOF &&
+	:<YELLOW>1:  a4b3333 = 1:  f686024 s/5/A/<RESET>
+	:<RED>2:  f51d370 <RESET><YELLOW>!<RESET><GREEN> 2:  4ab067d<RESET><YELLOW> s/4/A/<RESET>
+	:<RED>3:  0559556 <RESET><YELLOW>!<RESET><GREEN> 3:  b9cb956<RESET><YELLOW> s/11/B/<RESET>
+	:<RED>4:  d966c5c <RESET><YELLOW>!<RESET><GREEN> 4:  8add5f1<RESET><YELLOW> s/12/B/<RESET>
+	EOF
+	git range-diff changed...changed-message --color --dual-color --no-patch >actual.raw &&
+	test_decode_color >actual <actual.raw &&
+	test_cmp expect actual
+'
+
 for prev in topic master..topic
 do
 	test_expect_success "format-patch --range-diff=$prev" '
-- 
2.19.1.930.g4563a0d9d0

Re: [PATCH] range-diff: add a --no-patch option to show a summary

[Eric Sunshine]

On Mon, Nov 5, 2018 at 3:07 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> Add a --no-patch option which shows which changes got removed, added
> or moved etc., without showing the diff associated with them.

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/

Re: [PATCH] range-diff: add a --no-patch option to show a summary

[Ævar Arnfjörð Bjarmason]

On Mon, Nov 05 2018, Eric Sunshine wrote:

> On Mon, Nov 5, 2018 at 3:07 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> Add a --no-patch option which shows which changes got removed, added
>> or moved etc., without showing the diff associated with them.
>
> 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.

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

> 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.

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

> The patch itself looks okay.
>
> [1]: https://public-inbox.org/git/8bc517e35d4842f8d9d98f3b99adb9475d6db2d2.1525361419.git.johannes.schindelin@gmx.de/

Re: [PATCH] range-diff: add a --no-patch option to show a summary

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> 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.

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.

Re: [PATCH] range-diff: add a --no-patch option to show a summary

[Eric Sunshine]

On Mon, Nov 5, 2018 at 11:17 PM Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> > 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.
>
> 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.

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).

> 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.

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.

Re: [PATCH] range-diff: add a --no-patch option to show a summary

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> 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.
>
> 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.

Re: [PATCH] range-diff: add a --no-patch option to show a summary

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 06 2018, Eric Sunshine wrote:

> On Mon, Nov 5, 2018 at 11:17 PM Junio C Hamano <gitster@pobox.com> wrote:
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>> > 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.
>>
>> 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.
>
> 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).
>
>> 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.
>
> 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.

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.

Re: [PATCH] range-diff: add a --no-patch option to show a summary

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2351 bytes --]

Hi,

On Mon, 5 Nov 2018, Ævar Arnfjörð Bjarmason wrote:

> On Mon, Nov 05 2018, Eric Sunshine wrote:
>
> > On Mon, Nov 5, 2018 at 3:07 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> >> Add a --no-patch option which shows which changes got removed, added
> >> or moved etc., without showing the diff associated with them.
> >
> > 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.
>
> Thanks for digging. Big thread, not going to re-read it now. I'd just
> like to have this.

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

>
> > 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.
>
> 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
>
> > The patch itself looks okay.
> >
> > [1]: https://public-inbox.org/git/8bc517e35d4842f8d9d98f3b99adb9475d6db2d2.1525361419.git.johannes.schindelin@gmx.de/
>

Re: [PATCH] range-diff: add a --no-patch option to show a summary

[Ævar Arnfjörð Bjarmason]

On Tue, Nov 06 2018, Johannes Schindelin wrote:

> Hi,
>
> On Mon, 5 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
>
>> On Mon, Nov 05 2018, Eric Sunshine wrote:
>>
>> > On Mon, Nov 5, 2018 at 3:07 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> >> Add a --no-patch option which shows which changes got removed, added
>> >> or moved etc., without showing the diff associated with them.
>> >
>> > 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.
>>
>> Thanks for digging. Big thread, not going to re-read it now. I'd just
>> like to have this.
>
> 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.

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?

>>
>> > 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.
>>
>> 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
>>
>> > The patch itself looks okay.
>> >
>> > [1]: https://public-inbox.org/git/8bc517e35d4842f8d9d98f3b99adb9475d6db2d2.1525361419.git.johannes.schindelin@gmx.de/
>>

[PATCH v2] range-diff: add a --no-patch option to show a summary

[Ævar Arnfjörð Bjarmason]

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/87d0ri7gbs.fsf@evledraar.gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
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/87d0ri7gbs.fsf@evledraar.gmail.com/
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ -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(-)

diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
index f693930fdb..6c1eb647a1 100644
--- a/Documentation/git-range-diff.txt
+++ b/Documentation/git-range-diff.txt
@@ -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>`.
diff --git a/builtin/log.c b/builtin/log.c
index 061d4fd864..e063bcf2dd 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -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);
 	}
 }
 
diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index f01a0be851..05d1f6b6b6 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -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);
diff --git a/log-tree.c b/log-tree.c
index 7a83e99250..843e3ef83b 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -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));
 	}
diff --git a/range-diff.c b/range-diff.c
index bd8083f2d1..c1bfa593ce 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -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;
diff --git a/range-diff.h b/range-diff.h
index 190593f0c7..99bbc1cd9f 100644
--- a/range-diff.h
+++ b/range-diff.h
@@ -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
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 6aae364171..27e071650b 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -122,6 +122,26 @@ test_expect_success 'changed commit' '
 	test_cmp expected actual
 '
 
+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/
+	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 &&
+	git range-diff --no-color -s topic...changed >actual &&
+	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 &&
@@ -151,6 +171,17 @@ test_expect_success 'changed commit with sm config' '
 	test_cmp expected actual
 '
 
+test_expect_success 'changed commit with sm config --no-patch' '
+	git range-diff --no-color --no-patch --submodule=log 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 'no commits on one side' '
 	git commit --amend -m "new message" &&
 	git range-diff master HEAD@{1} HEAD
@@ -176,6 +207,17 @@ test_expect_success 'changed message' '
 	test_cmp expected actual
 '
 
+test_expect_success 'changed message --no-patch' '
+	git range-diff --no-color --no-patch topic...changed-message >actual &&
+	sed s/Z/\ /g >expected <<-EOF &&
+	1:  4de457d = 1:  f686024 s/5/A/
+	2:  fccce22 ! 2:  4ab067d s/4/A/
+	3:  147e64e = 3:  b9cb956 s/11/B/
+	4:  a63e992 = 4:  8add5f1 s/12/B/
+	EOF
+	test_cmp expected actual
+'
+
 test_expect_success 'dual-coloring' '
 	sed -e "s|^:||" >expect <<-\EOF &&
 	:<YELLOW>1:  a4b3333 = 1:  f686024 s/5/A/<RESET>
@@ -215,6 +257,18 @@ test_expect_success 'dual-coloring' '
 	test_cmp expect actual
 '
 
+test_expect_success 'dual-coloring --no-patch' '
+	sed -e "s|^:||" >expect <<-\EOF &&
+	:<YELLOW>1:  a4b3333 = 1:  f686024 s/5/A/<RESET>
+	:<RED>2:  f51d370 <RESET><YELLOW>!<RESET><GREEN> 2:  4ab067d<RESET><YELLOW> s/4/A/<RESET>
+	:<RED>3:  0559556 <RESET><YELLOW>!<RESET><GREEN> 3:  b9cb956<RESET><YELLOW> s/11/B/<RESET>
+	:<RED>4:  d966c5c <RESET><YELLOW>!<RESET><GREEN> 4:  8add5f1<RESET><YELLOW> s/12/B/<RESET>
+	EOF
+	git range-diff changed...changed-message --color --dual-color --no-patch >actual.raw &&
+	test_decode_color >actual <actual.raw &&
+	test_cmp expect actual
+'
+
 for prev in topic master..topic
 do
 	test_expect_success "format-patch --range-diff=$prev" '
-- 
2.19.1.930.g4563a0d9d0

Re: [PATCH v2] range-diff: add a --no-patch option to show a summary

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> diff --git a/builtin/range-diff.c b/builtin/range-diff.c
> index f01a0be851..05d1f6b6b6 100644
> --- a/builtin/range-diff.c
> +++ b/builtin/range-diff.c
> @@ -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),

As OPT_BOOL("patch") natively takes "--no-patch" to flip the bool
off, an int variable "patch" that is initialized to 1 would make it
more readable by avoiding double negation !no_patch like the one we
see below.  I guess the reason behind the contortion you wanted to
give the synonym --silent to it?

> @@ -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);

> @@ -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);

Other than that small "Huh?", the code looks good to me.

> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index 6aae364171..27e071650b 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -122,6 +122,26 @@ test_expect_success 'changed commit' '
>  	test_cmp expected actual
>  '
>  
> +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

This makes sure that -p and --patch produces the same output as the
default case?  I am not sure who in the parseopt API groks the
single letter "-p" in this case offhand.  Care to explain how?

The other side of the test to see -s/--no-patch we see below also
makes sense.

> +test_expect_success 'changed commit -s & --no-patch' '
> +...
> +	cat >expected <<-EOF &&

Quote EOF to allow readers skim the contents without looking for and
worrying about $substitutions in there, unless there are tons of
offending code in the same script already in which case we should
leave the clean-up outside this primary change.

Re: [PATCH] range-diff: add a --no-patch option to show a summary

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 3204 bytes --]

Hi Ævar,

On Tue, 6 Nov 2018, Ævar Arnfjörð Bjarmason wrote:

> On Tue, Nov 06 2018, Johannes Schindelin wrote:
> 
> > On Mon, 5 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> >
> >> On Mon, Nov 05 2018, Eric Sunshine wrote:
> >>
> >> > On Mon, Nov 5, 2018 at 3:07 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> >> >> Add a --no-patch option which shows which changes got removed, added
> >> >> or moved etc., without showing the diff associated with them.
> >> >
> >> > 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.
> >>
> >> Thanks for digging. Big thread, not going to re-read it now. I'd just
> >> like to have this.
> >
> > 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.
> 
> 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?

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

> 
> >>
> >> > 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.
> >>
> >> 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
> >>
> >> > The patch itself looks okay.
> >> >
> >> > [1]: https://public-inbox.org/git/8bc517e35d4842f8d9d98f3b99adb9475d6db2d2.1525361419.git.johannes.schindelin@gmx.de/
> >>
> 

Re: [PATCH] range-diff: add a --no-patch option to show a summary

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2257 bytes --]

Hi Ævar,

On Wed, 7 Nov 2018, Johannes Schindelin wrote:

> On Tue, 6 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> 
> > On Tue, Nov 06 2018, Johannes Schindelin wrote:
> > 
> > > On Mon, 5 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> > >
> > >> On Mon, Nov 05 2018, Eric Sunshine wrote:
> > >>
> > >> > On Mon, Nov 5, 2018 at 3:07 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> > >> >> Add a --no-patch option which shows which changes got removed, added
> > >> >> or moved etc., without showing the diff associated with them.
> > >> >
> > >> > 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.
> > >>
> > >> Thanks for digging. Big thread, not going to re-read it now. I'd just
> > >> like to have this.
> > >
> > > 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.
> > 
> > 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?
> 
> 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.

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

Ciao,
Dscho

Re: [PATCH] range-diff: add a --no-patch option to show a summary

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2574 bytes --]

Hi Ævar,

On Wed, 7 Nov 2018, Johannes Schindelin wrote:

> On Wed, 7 Nov 2018, Johannes Schindelin wrote:
> 
> > On Tue, 6 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> > 
> > > On Tue, Nov 06 2018, Johannes Schindelin wrote:
> > > 
> > > > On Mon, 5 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> > > >
> > > >> On Mon, Nov 05 2018, Eric Sunshine wrote:
> > > >>
> > > >> > On Mon, Nov 5, 2018 at 3:07 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> > > >> >> Add a --no-patch option which shows which changes got removed, added
> > > >> >> or moved etc., without showing the diff associated with them.
> > > >> >
> > > >> > 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.
> > > >>
> > > >> Thanks for digging. Big thread, not going to re-read it now. I'd just
> > > >> like to have this.
> > > >
> > > > 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.
> > > 
> > > 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?
> > 
> > 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.
> 
> Nevermind, this was solved by passing `--exec-path=$PWD`. And *now* I can
> reproduce your finding.

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

Re: [PATCH] range-diff: add a --no-patch option to show a summary

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 4662 bytes --]

Me again,

On Wed, 7 Nov 2018, Johannes Schindelin wrote:

> On Wed, 7 Nov 2018, Johannes Schindelin wrote:
> 
> > On Wed, 7 Nov 2018, Johannes Schindelin wrote:
> > 
> > > On Tue, 6 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> > > 
> > > > On Tue, Nov 06 2018, Johannes Schindelin wrote:
> > > > 
> > > > > On Mon, 5 Nov 2018, Ævar Arnfjörð Bjarmason wrote:
> > > > >
> > > > >> On Mon, Nov 05 2018, Eric Sunshine wrote:
> > > > >>
> > > > >> > On Mon, Nov 5, 2018 at 3:07 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> > > > >> >> Add a --no-patch option which shows which changes got removed, added
> > > > >> >> or moved etc., without showing the diff associated with them.
> > > > >> >
> > > > >> > 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.
> > > > >>
> > > > >> Thanks for digging. Big thread, not going to re-read it now. I'd just
> > > > >> like to have this.
> > > > >
> > > > > 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.
> > > > 
> > > > 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?
> > > 
> > > 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.
> > 
> > Nevermind, this was solved by passing `--exec-path=$PWD`. And *now* I can
> > reproduce your finding.
> 
> 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.

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:

diff --git a/range-diff.c b/range-diff.c
index 3dd2edda0176..014112ee401f 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -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

Re: [PATCH v2] range-diff: add a --no-patch option to show a summary

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 3315 bytes --]

Hi,

On Wed, 7 Nov 2018, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> 
> > diff --git a/builtin/range-diff.c b/builtin/range-diff.c
> > index f01a0be851..05d1f6b6b6 100644
> > --- a/builtin/range-diff.c
> > +++ b/builtin/range-diff.c
> > @@ -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),
> 
> As OPT_BOOL("patch") natively takes "--no-patch" to flip the bool
> off, an int variable "patch" that is initialized to 1 would make it
> more readable by avoiding double negation !no_patch like the one we
> see below.  I guess the reason behind the contortion you wanted to
> give the synonym --silent to it?

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

> 
> > @@ -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);
> 
> > @@ -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);
> 
> Other than that small "Huh?", the code looks good to me.
> 
> > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> > index 6aae364171..27e071650b 100755
> > --- a/t/t3206-range-diff.sh
> > +++ b/t/t3206-range-diff.sh
> > @@ -122,6 +122,26 @@ test_expect_success 'changed commit' '
> >  	test_cmp expected actual
> >  '
> >  
> > +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
> 
> This makes sure that -p and --patch produces the same output as the
> default case?  I am not sure who in the parseopt API groks the
> single letter "-p" in this case offhand.  Care to explain how?
> 
> The other side of the test to see -s/--no-patch we see below also
> makes sense.
> 
> > +test_expect_success 'changed commit -s & --no-patch' '
> > +...
> > +	cat >expected <<-EOF &&
> 
> Quote EOF to allow readers skim the contents without looking for and
> worrying about $substitutions in there, unless there are tons of
> offending code in the same script already in which case we should
> leave the clean-up outside this primary change.
> 
> 

[PATCH v3 0/2] range-diff: doc + regression fix

[Ævar Arnfjörð Bjarmason]

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

[PATCH v3 1/2] range-diff doc: add a section about output stability

[Ævar Arnfjörð Bjarmason]

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-range-diff.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
index f693930fdb..bbd07a9be8 100644
--- a/Documentation/git-range-diff.txt
+++ b/Documentation/git-range-diff.txt
@@ -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

[PATCH v3 2/2] range-diff: fix regression in passing along diff options

[Ævar Arnfjörð Bjarmason]

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/nycvar.QRO.7.76.6.1811071202480.39@tvgsbejvaqbjf.bet/
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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 range-diff.c          |  3 ++-
 t/t3206-range-diff.sh | 30 ++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/range-diff.c b/range-diff.c
index bd8083f2d1..488844c0af 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -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;
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 6aae364171..e497c1358f 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -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

Re: [PATCH v3 1/2] range-diff doc: add a section about output stability

[Stephen & Linda Smith]

On Wednesday, November 7, 2018 5:22:01 AM MST Ævar Arnfjörð Bjarmason wrote:
> +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

"`--stat` can as an emergent": I read that for times to decided it was correct 
grammar.   Should it be reworded to read better?   Just a nit.

> +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).

Re: [PATCH v3 1/2] range-diff doc: add a section about output stability

[Martin Ågren]

On Wed, 7 Nov 2018 at 13:22, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> +
> +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

s/specifc/specific/

> +to `range-diff` (e.g. for `--stat` summarizing how the diffstat
> +changed).

Re: [PATCH v3 1/2] range-diff doc: add a section about output stability

[Junio C Hamano]

Stephen & Linda Smith <ischis2@cox.net> writes:

>> +This is particularly true when passing in diff options. Currently some
>> +options like `--stat` can as an emergent effect produce output that's
>
> "`--stat` can as an emergent": I read that for times to decided it was correct 
> grammar.   Should it be reworded to read better?   Just a nit.

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".

>
>> +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).

Re: [PATCH v3 2/2] range-diff: fix regression in passing along diff options

[Eric Sunshine]

On Wed, Nov 7, 2018 at 7:22 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> 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.

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...

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/range-diff.c b/range-diff.c
> @@ -453,7 +453,8 @@ int show_range_diff(const char *range1, const char *range2,
>                 memcpy(&opts, diffopt, sizeof(opts));
> -               opts.output_format = DIFF_FORMAT_PATCH;
> +               if (!opts.output_format)
> +                       opts.output_format = DIFF_FORMAT_PATCH;

Looking at diff.c:parse_diff_opt() and enable_patch_output(), rather
than introducing this new conditional, I'm thinking that a more
correct fix would be:

    opts.output_format |= DIFF_FORMAT_PATCH;

(note the '|=' operator). This would result in 'opts.output_format'
containing (DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT), just as it did
prior to 73a834e9e2 when --no-patch was specified.

Re: [PATCH v3 2/2] range-diff: fix regression in passing along diff options

[Ævar Arnfjörð Bjarmason]

On Thu, Nov 08 2018, Eric Sunshine wrote:

> On Wed, Nov 7, 2018 at 7:22 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> 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.
>
> 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...
>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>> diff --git a/range-diff.c b/range-diff.c
>> @@ -453,7 +453,8 @@ int show_range_diff(const char *range1, const char *range2,
>>                 memcpy(&opts, diffopt, sizeof(opts));
>> -               opts.output_format = DIFF_FORMAT_PATCH;
>> +               if (!opts.output_format)
>> +                       opts.output_format = DIFF_FORMAT_PATCH;
>
> Looking at diff.c:parse_diff_opt() and enable_patch_output(), rather
> than introducing this new conditional, I'm thinking that a more
> correct fix would be:
>
>     opts.output_format |= DIFF_FORMAT_PATCH;
>
> (note the '|=' operator). This would result in 'opts.output_format'
> containing (DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT), just as it did
> prior to 73a834e9e2 when --no-patch was specified.

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".

Re: [PATCH v3 2/2] range-diff: fix regression in passing along diff options

[Eric Sunshine]

On Thu, Nov 8, 2018 at 5:34 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> On Thu, Nov 08 2018, Eric Sunshine wrote:
> > Looking at diff.c:parse_diff_opt() and enable_patch_output(), rather
> > than introducing this new conditional, I'm thinking that a more
> > correct fix would be:
> >
> >     opts.output_format |= DIFF_FORMAT_PATCH;
> >
> > (note the '|=' operator). This would result in 'opts.output_format'
> > containing (DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT), just as it did
> > prior to 73a834e9e2 when --no-patch was specified.
>
> Maybe I'm misunderstanding, but if you mean this on top:
>
>     -               if (!opts.output_format)
>     -                       opts.output_format = DIFF_FORMAT_PATCH;
>     +               opts.output_format |= DIFF_FORMAT_PATCH;

That is indeed what I mean.

> 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").

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).

> 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".

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.

Re: [PATCH v3 2/2] range-diff: fix regression in passing along diff options

[Ævar Arnfjörð Bjarmason]

On Fri, Nov 09 2018, Eric Sunshine wrote:

> On Thu, Nov 8, 2018 at 5:34 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> On Thu, Nov 08 2018, Eric Sunshine wrote:
>> > Looking at diff.c:parse_diff_opt() and enable_patch_output(), rather
>> > than introducing this new conditional, I'm thinking that a more
>> > correct fix would be:
>> >
>> >     opts.output_format |= DIFF_FORMAT_PATCH;
>> >
>> > (note the '|=' operator). This would result in 'opts.output_format'
>> > containing (DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT), just as it did
>> > prior to 73a834e9e2 when --no-patch was specified.
>>
>> Maybe I'm misunderstanding, but if you mean this on top:
>>
>>     -               if (!opts.output_format)
>>     -                       opts.output_format = DIFF_FORMAT_PATCH;
>>     +               opts.output_format |= DIFF_FORMAT_PATCH;
>
> That is indeed what I mean.

*Nod*

>> 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").
>
> 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).

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

>> 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".
>
> 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.

*Nod*

[PATCH v4 0/3] range-diff fixes

[Ævar Arnfjörð Bjarmason]

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/nycvar.QRO.7.76.6.1811071202480.39@tvgsbejvaqbjf.bet/
    -    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

[PATCH v4 1/3] range-diff doc: add a section about output stability

[Ævar Arnfjörð Bjarmason]

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-range-diff.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
index f693930fdb..8a6ea2c6c5 100644
--- a/Documentation/git-range-diff.txt
+++ b/Documentation/git-range-diff.txt
@@ -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

[PATCH v4 2/3] range-diff: fix regression in passing along diff options

[Ævar Arnfjörð Bjarmason]

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/nycvar.QRO.7.76.6.1811071202480.39@tvgsbejvaqbjf.bet/
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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 range-diff.c          |  2 +-
 t/t3206-range-diff.sh | 53 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/range-diff.c b/range-diff.c
index bd8083f2d1..ea317f92f9 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -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;
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 6aae364171..ab44e085d5 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -122,6 +122,59 @@ 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' '
+	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
+'
+
 test_expect_success 'changed commit with sm config' '
 	git range-diff --no-color --submodule=log topic...changed >actual &&
 	cat >expected <<-EOF &&
-- 
2.19.1.1182.g4ecb1133ce

[PATCH v4 3/3] range-diff: make diff option behavior (e.g. --stat) consistent

[Ævar Arnfjörð Bjarmason]

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).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 range-diff.c          |  3 ++-
 t/t3206-range-diff.sh | 22 ----------------------
 2 files changed, 2 insertions(+), 23 deletions(-)

diff --git a/range-diff.c b/range-diff.c
index ea317f92f9..72bde281f3 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -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;
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index ab44e085d5..9352f65280 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -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

Re: [PATCH v4 3/3] range-diff: make diff option behavior (e.g. --stat) consistent

[Stephen & Linda Smith]

On Friday, November 9, 2018 3:18:03 AM MST Ævar Arnfjörð Bjarmason wrote:
> 
> 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
 's/ two//'

> how they interpret diff options.
> 

Re: [PATCH v4 0/3] range-diff fixes

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 262 bytes --]

Hi Ævar,

On Fri, 9 Nov 2018, Ævar Arnfjörð Bjarmason wrote:

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

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

Ciao,
Dscho

Re: [PATCH v4 3/3] range-diff: make diff option behavior (e.g. --stat) consistent

[Eric Sunshine]

On Fri, Nov 9, 2018 at 5:18 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> Make the behavior when diff options (e.g. "--stat") are passed
> consistent with how "diff" behaves.
> [...]
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/range-diff.c b/range-diff.c
> @@ -453,7 +453,8 @@ int show_range_diff(const char *range1, const char *range2,
> -               opts.output_format |= DIFF_FORMAT_PATCH;
> +               if (!opts.output_format)
> +                       opts.output_format |= DIFF_FORMAT_PATCH;

I think this can just be '=' now instead of '|=' (to avoid confusing
the reader, even if it's functionally equivalent).

Re: [PATCH v4 3/3] range-diff: make diff option behavior (e.g. --stat) consistent

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Fri, Nov 9, 2018 at 5:18 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> Make the behavior when diff options (e.g. "--stat") are passed
>> consistent with how "diff" behaves.
>> [...]
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>> diff --git a/range-diff.c b/range-diff.c
>> @@ -453,7 +453,8 @@ int show_range_diff(const char *range1, const char *range2,
>> -               opts.output_format |= DIFF_FORMAT_PATCH;
>> +               if (!opts.output_format)
>> +                       opts.output_format |= DIFF_FORMAT_PATCH;
>
> I think this can just be '=' now instead of '|=' (to avoid confusing
> the reader, even if it's functionally equivalent).

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.

Re: [PATCH v4 3/3] range-diff: make diff option behavior (e.g. --stat) consistent

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index ab44e085d5..9352f65280 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -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

This removes all references to four_spaces=" " assigned at the very
beginning of this test piece, so that assignment should also go, no?

>  	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
>  '

[PATCH v5 0/3] range-diff fixes

[Ævar Arnfjörð Bjarmason]

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

[PATCH v5 1/3] range-diff doc: add a section about output stability

[Ævar Arnfjörð Bjarmason]

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-range-diff.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
index f693930fdb..8a6ea2c6c5 100644
--- a/Documentation/git-range-diff.txt
+++ b/Documentation/git-range-diff.txt
@@ -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

[PATCH v5 2/3] range-diff: fix regression in passing along diff options

[Ævar Arnfjörð Bjarmason]

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/nycvar.QRO.7.76.6.1811071202480.39@tvgsbejvaqbjf.bet/
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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 range-diff.c          |  2 +-
 t/t3206-range-diff.sh | 53 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/range-diff.c b/range-diff.c
index 3958720f00..ec937008d0 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -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;
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 6aae364171..ab44e085d5 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -122,6 +122,59 @@ 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' '
+	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
+'
+
 test_expect_success 'changed commit with sm config' '
 	git range-diff --no-color --submodule=log topic...changed >actual &&
 	cat >expected <<-EOF &&
-- 
2.19.1.1182.g4ecb1133ce

[PATCH v5 3/3] range-diff: make diff option behavior (e.g. --stat) consistent

[Ævar Arnfjörð Bjarmason]

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).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 range-diff.c          |  3 ++-
 t/t3206-range-diff.sh | 23 -----------------------
 2 files changed, 2 insertions(+), 24 deletions(-)

diff --git a/range-diff.c b/range-diff.c
index ec937008d0..767af8c5bb 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -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;
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index ab44e085d5..e497c1358f 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -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

Re: [PATCH v5 0/3] range-diff fixes

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2502 bytes --]

Hi Ævar,

On Tue, 13 Nov 2018, Ævar Arnfjörð Bjarmason wrote:

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

This range-diff looks good to me.

Thanks,
Dscho

> 
> 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
> 
>