Range diff with ranges lacking dotdot

From: Johannes Schindelin <[email protected]>

Currently, when called with exactly two arguments, we test for a literal .. in each of the two.

However, <commit>^! is a perfectly valid commit range, equivalent to <commit>^..<commit> according to the SPECIFYING RANGES section of gitrevisions[7].

In preparation for allowing more sophisticated ways to specify commit ranges, let's refactor the conditional into its own function.

---
 builtin/range-diff.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

index 24c4162f744..551d3e689cb 100644
@@ -11,6 +11,11 @@ N_("git range-diff [<options>] <base> <old-tip> <new-tip>"),
 NULL
 };
 
+static int is_range(const char *range)
+{
+	return !!strstr(range, "..");
+}
+
 int cmd_range_diff(int argc, const char **argv, const char *prefix)
 {
 	int creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;

@@ -46,12 +51,12 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 		diffopt.use_color = 1;
 
 	if (argc == 2) {
-		if (!strstr(argv[0], ".."))
-			die(_("no .. in range: '%s'"), argv[0]);
+		if (!is_range(argv[0]))
+			die(_("not a commit range: '%s'"), argv[0]);
 		strbuf_addstr(&range1, argv[0]);
 
-		if (!strstr(argv[1], ".."))
-			die(_("no .. in range: '%s'"), argv[1]);
+		if (!is_range(argv[1]))
+			die(_("not a commit range: '%s'"), argv[1]);
 		strbuf_addstr(&range2, argv[1]);
 	} else if (argc == 3) {
 		strbuf_addf(&range1, "%s..%s", argv[0], argv[1]);

gitgitgadget

From: Johannes Schindelin <[email protected]>

In the SPECIFYING RANGES section of gitrevisions[7], two ways are described to specify commit ranges that range-diff does not yet
accept: "<commit>^!" and "<commit>^-<n>".

Let's accept them.

---
 builtin/range-diff.c  | 21 ++++++++++++++++++++-
 t/t3206-range-diff.sh |  8 ++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

index 551d3e689cb..6097635c432 100644
@@ -13,7 +13,26 @@ NULL
 
 static int is_range(const char *range)
 {
-	return !!strstr(range, "..");
+	size_t i;
+	char c;
+
+	if (strstr(range, ".."))
+		return 1;
+
+	i = strlen(range);
+	c = i ? range[--i] : 0;
+	if (c == '!')
+		i--; /* might be ...^! or ...^@ */
+	else if (isdigit(c)) {
+		/* handle ...^-<n> */
+		while (i > 2 && isdigit(range[--i]))
+			; /* keep trimming trailing digits */
+		if (i < 2 || range[i--] != '-')
+			return 0;
+	} else
+		return 0;
+
+	return i > 0 && range[i] == '^';
 }
 
 int cmd_range_diff(int argc, const char **argv, const char *prefix)

index 6eb344be031..e217cecac9e 100755
@@ -150,6 +150,14 @@ test_expect_success 'simple A B C (unmodified)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'A^! and A^-<n> (unmodified)' '
+	git range-diff --no-color topic^! unmodified^-1 >actual &&
+	cat >expect <<-EOF &&
+	1:  $(test_oid t4) = 1:  $(test_oid u4) s/12/B/
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'trivial reordering' '
 	git range-diff --no-color master topic reordered >actual &&
 	cat >expect <<-EOF &&

gitgitgadget

From: Johannes Schindelin <[email protected]>

There are three forms, depending whether the user specifies one, two or three non-option arguments. We've never actually explained how this works in the manual, so let's explain it.

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

index 9701c1e5fdd..76359baf26d 100644
@@ -28,6 +28,19 @@ Finally, the list of matching commits is shown in the order of the
 second commit range, with unmatched commits being inserted just after
 all of their ancestors have been shown.
 
+There are three ways to specify the commit ranges:
+
+- `<range1> <range2>`: Either commit range can be of the form
+  `<base>..<rev>`, `<rev>^!` or `<rev>^-<n>`. See `SPECIFYING RANGES`
+  in linkgit:gitrevisions[7] for more details.
+
+- `<rev1>...<rev2>`. This resembles the symmetric ranges mentioned in
+  the `SPECIFYING RANGES` section of linkgit:gitrevisions[7], and is
+  equivalent to `<base>..<rev1> <base>..<rev2>` where `<base>` is the
+  merge base as obtained via `git merge-base <rev1> <rev2>`.
+
+- `<base> <rev1> <rev2>`: This is equivalent to `<base>..<rev1>
+  <base>..<rev2>`.
 
 OPTIONS
 -------

gitgitgadget

In
https://lore.kernel.org/git/[email protected]/,
it was reported that git range-diff does not handle commit ranges like rev^!. This patch series fixes that.

Johannes Schindelin (3):
range-diff: refactor check for commit range
range-diff: handle commit ranges other than A..B range-diff(docs): explain how to specify commit ranges

 Documentation/git-range-diff.txt | 13 +++++++++++++
 builtin/range-diff.c             | 32 ++++++++++++++++++++++++++++----
 t/t3206-range-diff.sh            |  8 ++++++++
 3 files changed, 49 insertions(+), 4 deletions(-)

base-commit: 71ca53e8125e36efbda17293c50027d31681a41f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-841%2Fdscho%2Frange-diff-with-ranges-lacking-dotdot-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-841/dscho/range-diff-with-ranges-lacking-dotdot-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/841

gitgitgadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

Makes sense.

"Johannes Schindelin via GitGitGadget" <[email protected]>

"Johannes Schindelin via GitGitGadget" <[email protected]>

Hello Johannes,

Oh wow, great you picked that up. It still occasionally annoys me that the range parser of range-diff cannot do $hash^!.

I assume you will fix the stuff Junio pointed out (or argument your patch 1 is right, I didn't try to check). When this is discussed to an end or fixed I will happily test your series.

And by the way you also made one of my coworkers happy with your series.

Best regards and thanks
Uwe

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Hi Junio,

Bah. This is a left-over from an earlier version. I tried to hide the fact that I had misunderstood <rev>^@ to specify a commit range. Oh well.

Right, guarding that range[--i] only by i is not enough. My idea was to exit early if the string is too short, anyway, i.e. if i < 3.

Right. Will be fixed in the next iteration.

Thanks,
Dscho

Hi Junio,

No, it is not exactly what a symmetric range is because range-diff treats both arms of the symmetric range independently, as two distinct non-symmetric ranges.

Yes. I was rather surprised that I had overlooked that in my original submission of the range-diff command.

Ciao,
Dscho

Hi Eric,

Thank you for pointing that out. I should have checked via git grep 'strstr.*"\.\."' myself. There are two more instances, one in rev-parse.c and the other in revision.c, but both are necessary as-are because their return value is actually used to further disect a ..-style commit range.

Thanks,
Dscho

From: Johannes Schindelin <[email protected]>

In the SPECIFYING RANGES section of gitrevisions[7], two ways are described to specify commit ranges that range-diff does not yet
accept: "<commit>^!" and "<commit>^-<n>".

Let's accept them.

---
 revision.c            | 22 +++++++++++++++++++++-
 t/t3206-range-diff.sh |  8 ++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)

index 00675f598a3..9ee063a2c03 100644
@@ -4209,5 +4209,25 @@ void put_revision_mark(const struct rev_info *revs, const struct commit *commit)
 
 int specifies_commit_range(const char *range)
 {
-	return !!strstr(range, "..");
+	size_t i;
+	char c;
+
+	if (strstr(range, ".."))
+		return 1;
+
+	i = strlen(range);
+	c = i > 2 ? range[--i] : 0;
+	if (c == '!')
+		i--; /* might be ...^! */
+	else if (isdigit(c)) {
+		/* handle ...^-<n> */
+		while (i > 2 && isdigit(range[--i]))
+			; /* keep trimming trailing digits */
+		if (i < 2 || range[i--] != '-')
+			return 0;
+	} else
+		return 0;
+
+	/* Before the `!` or the `-<n>`, we expect `<rev>^` */
+	return i > 0 && range[i] == '^';
 }

index 6eb344be031..e217cecac9e 100755
@@ -150,6 +150,14 @@ test_expect_success 'simple A B C (unmodified)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'A^! and A^-<n> (unmodified)' '
+	git range-diff --no-color topic^! unmodified^-1 >actual &&
+	cat >expect <<-EOF &&
+	1:  $(test_oid t4) = 1:  $(test_oid u4) s/12/B/
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'trivial reordering' '
 	git range-diff --no-color master topic reordered >actual &&
 	cat >expect <<-EOF &&

gitgitgadget

From: Johannes Schindelin <[email protected]>

There are three forms, depending whether the user specifies one, two or three non-option arguments. We've never actually explained how this works in the manual, so let's explain it.

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

index 9701c1e5fdd..76359baf26d 100644
@@ -28,6 +28,19 @@ Finally, the list of matching commits is shown in the order of the
 second commit range, with unmatched commits being inserted just after
 all of their ancestors have been shown.
 
+There are three ways to specify the commit ranges:
+
+- `<range1> <range2>`: Either commit range can be of the form
+  `<base>..<rev>`, `<rev>^!` or `<rev>^-<n>`. See `SPECIFYING RANGES`
+  in linkgit:gitrevisions[7] for more details.
+
+- `<rev1>...<rev2>`. This resembles the symmetric ranges mentioned in
+  the `SPECIFYING RANGES` section of linkgit:gitrevisions[7], and is
+  equivalent to `<base>..<rev1> <base>..<rev2>` where `<base>` is the
+  merge base as obtained via `git merge-base <rev1> <rev2>`.
+
+- `<base> <rev1> <rev2>`: This is equivalent to `<base>..<rev1>
+  <base>..<rev2>`.
 
 OPTIONS
 -------

gitgitgadget

The above paragraph says A...B is turned into $(git merge-base A B)..A and $(git merge-base A B)..B, but I wonder if we should be rewriting it into A..B and B..A instead; that would make it unnecessary to explain what should happen when there are more than one merge bases.

This however is an end-user documentation, isn't it?

The fact that range-diff internally translates A...B into something else is an implementation detail of achieving something, and from the end-users' point of view, isn't it "take A...B from the end-user, and show the difference between the left and right branches of the symmetric range" that range-diff achieves to do?

So it processes exactly the same two sets of commits as what is given by "--left-right A...B", no?

In
https://lore.kernel.org/git/[email protected]/,
it was reported that git range-diff does not handle commit ranges like rev^!. This patch series fixes that.

Changes since v1:

* In addition to git range-diff, git format-patch --range-diff gets the same improvement.
* The comment talking about ^@ was removed.
* The parsing was made a bit safer (e.g. catching ! by its own as an invalid range).

Johannes Schindelin (3):
range-diff/format-patch: refactor check for commit range range-diff/format-patch: handle commit ranges other than A..B range-diff(docs): explain how to specify commit ranges

 Documentation/git-range-diff.txt | 13 +++++++++++++
 builtin/log.c                    |  2 +-
 builtin/range-diff.c             |  9 +++++----
 revision.c                       | 25 +++++++++++++++++++++++++
 revision.h                       |  7 +++++++
 t/t3206-range-diff.sh            |  8 ++++++++
 6 files changed, 59 insertions(+), 5 deletions(-)

base-commit: 71ca53e8125e36efbda17293c50027d31681a41f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-841%2Fdscho%2Frange-diff-with-ranges-lacking-dotdot-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-841/dscho/range-diff-with-ranges-lacking-dotdot-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/841

Range-diff vs v1:

1: 5839ba4f761 ! 1: 3f21e10f919 range-diff: refactor check for commit range
@@ Metadata
Author: Johannes Schindelin <[email protected]>

## Commit message ##
- range-diff: refactor check for commit range
+ range-diff/format-patch: refactor check for commit range

- Currently, when called with exactly two arguments, we test for a literal
- .. in each of the two.
+ Currently, when called with exactly two arguments, git range-diff
+ tests for a literal .. in each of the two. Likewise, the argument
+ provided via --range-diff to git format-patch is checked in the same
+ manner.

However, <commit>^! is a perfectly valid commit range, equivalent to <commit>^..<commit> according to the SPECIFYING RANGES section of gitrevisions[7].

In preparation for allowing more sophisticated ways to specify commit
- ranges, let's refactor the conditional into its own function.
+ ranges, let's refactor the check into its own function.

+ ## builtin/log.c ##
+@@ builtin/log.c: static void infer_range_diff_ranges(struct strbuf *r1,
+ struct commit *head)
+ {
+ const char *head_oid = oid_to_hex(&head->object.oid);
+- int prev_is_range = !!strstr(prev, "..");
++ int prev_is_range = specifies_commit_range(prev);
+
+ if (prev_is_range)
+ strbuf_addstr(r1, prev);
+
## builtin/range-diff.c ##
-@@ builtin/range-diff.c: N_("git range-diff [<options>] <base> <old-tip> <new-tip>"),
- NULL
- };
+@@
+ #include "parse-options.h"
+ #include "range-diff.h"
+ #include "config.h"
++#include "revision.h"

-+static int is_range(const char *range)
-+{
-+ return !!strstr(range, "..");
-+}
-+
- int cmd_range_diff(int argc, const char **argv, const char *prefix)
- {
- int creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
+ static const char * const builtin_range_diff_usage[] = {
+ N_("git range-diff [<options>] <old-base>..<old-tip> <new-base>..<new-tip>"), @@ builtin/range-diff.c: int cmd_range_diff(int argc, const char **argv, const char *prefix) diffopt.use_color = 1;

if (argc == 2) {
- if (!strstr(argv[0], ".."))
- die(_("no .. in range: '%s'"), argv[0]);
-+ if (!is_range(argv[0]))
++ if (!specifies_commit_range(argv[0]))
+ die(_("not a commit range: '%s'"), argv[0]);
strbuf_addstr(&range1, argv[0]);

- if (!strstr(argv[1], ".."))
- die(_("no .. in range: '%s'"), argv[1]);
-+ if (!is_range(argv[1]))
++ if (!specifies_commit_range(argv[1]))
+ die(_("not a commit range: '%s'"), argv[1]);
strbuf_addstr(&range2, argv[1]);
} else if (argc == 3) {
strbuf_addf(&range1, "%s..%s", argv[0], argv[1]);
+
+ ## revision.c ##
+@@ revision.c: void put_revision_mark(const struct rev_info *revs, const struct commit *commit)
+ fputs(mark, stdout);
+ putchar(' ');
+ }
++
++int specifies_commit_range(const char *range)
++{
++ return !!strstr(range, "..");
++}
+
+ ## revision.h ##
+@@ revision.h: int rewrite_parents(struct rev_info *revs,
+ */
+ struct commit_list *get_saved_parents(struct rev_info *revs, const struct commit *commit); +
++/*
++ * Determine whether the given argument defines a commit range, e.g. A..B.
++ * Note that this only validates the format but does _not_ parse it, i.e.
++ * it does _not_ look up the specified commits in the local repository.
++ */
++int specifies_commit_range(const char *range);
++
+ #endif
2: 88c15617b4b ! 2: 2c2744333ec range-diff: handle commit ranges other than A..B

     @@ Metadata
      Author: Johannes Schindelin <[email protected]>
      
       ## Commit message ##
     -    range-diff: handle commit ranges other than A..B
     +    range-diff/format-patch: handle commit ranges other than A..B
      
          In the `SPECIFYING RANGES` section of gitrevisions[7], two ways are
          described to specify commit ranges that `range-diff` does not yet
     @@ Commit message
      
          Signed-off-by: Johannes Schindelin <[email protected]>
      
     - ## builtin/range-diff.c ##
     -@@ builtin/range-diff.c: NULL
     + ## revision.c ##
     +@@ revision.c: void put_revision_mark(const struct rev_info *revs, const struct commit *commit)
       
     - static int is_range(const char *range)
     + int specifies_commit_range(const char *range)
       {
      -	return !!strstr(range, "..");
      +	size_t i;
     @@ builtin/range-diff.c: NULL
      +		return 1;
      +
      +	i = strlen(range);
     -+	c = i ? range[--i] : 0;
     ++	c = i > 2 ? range[--i] : 0;
      +	if (c == '!')
     -+		i--; /* might be ...^! or ...^@ */
     ++		i--; /* might be ...^! */
      +	else if (isdigit(c)) {
      +		/* handle ...^-<n> */
      +		while (i > 2 && isdigit(range[--i]))
     @@ builtin/range-diff.c: NULL
      +	} else
      +		return 0;
      +
     ++	/* Before the `!` or the `-<n>`, we expect `<rev>^` */
      +	return i > 0 && range[i] == '^';
       }
     - 
     - int cmd_range_diff(int argc, const char **argv, const char *prefix)
      
       ## t/t3206-range-diff.sh ##
      @@ t/t3206-range-diff.sh: test_expect_success 'simple A B C (unmodified)' '

3: 041456b6e73 = 3: 4f5e5acd954 range-diff(docs): explain how to specify commit ranges

gitgitgadget

From: Johannes Schindelin <[email protected]>

Currently, when called with exactly two arguments, git range-diff tests for a literal .. in each of the two. Likewise, the argument provided via --range-diff to git format-patch is checked in the same manner.

However, <commit>^! is a perfectly valid commit range, equivalent to <commit>^..<commit> according to the SPECIFYING RANGES section of gitrevisions[7].

In preparation for allowing more sophisticated ways to specify commit ranges, let's refactor the check into its own function.

---
 builtin/log.c        | 2 +-
 builtin/range-diff.c | 9 +++++----
 revision.c           | 5 +++++
 revision.h           | 7 +++++++
 4 files changed, 18 insertions(+), 5 deletions(-)

index bd6ff4f9f95..099abdfb7e6 100644
@@ -1680,7 +1680,7 @@ static void infer_range_diff_ranges(struct strbuf *r1,
 				    struct commit *head)
 {
 	const char *head_oid = oid_to_hex(&head->object.oid);
-	int prev_is_range = !!strstr(prev, "..");
+	int prev_is_range = specifies_commit_range(prev);
 
 	if (prev_is_range)
 		strbuf_addstr(r1, prev);

index 24c4162f744..89d54158011 100644
@@ -3,6 +3,7 @@
 #include "parse-options.h"
 #include "range-diff.h"
 #include "config.h"
+#include "revision.h"
 
 static const char * const builtin_range_diff_usage[] = {
 N_("git range-diff [<options>] <old-base>..<old-tip> <new-base>..<new-tip>"),

@@ -46,12 +47,12 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 		diffopt.use_color = 1;
 
 	if (argc == 2) {
-		if (!strstr(argv[0], ".."))
-			die(_("no .. in range: '%s'"), argv[0]);
+		if (!specifies_commit_range(argv[0]))
+			die(_("not a commit range: '%s'"), argv[0]);
 		strbuf_addstr(&range1, argv[0]);
 
-		if (!strstr(argv[1], ".."))
-			die(_("no .. in range: '%s'"), argv[1]);
+		if (!specifies_commit_range(argv[1]))
+			die(_("not a commit range: '%s'"), argv[1]);
 		strbuf_addstr(&range2, argv[1]);
 	} else if (argc == 3) {
 		strbuf_addf(&range1, "%s..%s", argv[0], argv[1]);

index 9dff845bed6..00675f598a3 100644
@@ -4206,3 +4206,8 @@ void put_revision_mark(const struct rev_info *revs, const struct commit *commit)
 	fputs(mark, stdout);
 	putchar(' ');
 }
+
+int specifies_commit_range(const char *range)
+{
+	return !!strstr(range, "..");
+}

index 086ff10280d..66777c8e60f 100644
@@ -457,4 +457,11 @@ int rewrite_parents(struct rev_info *revs,
  */
 struct commit_list *get_saved_parents(struct rev_info *revs, const struct commit *commit);
 
+/*
+ * Determine whether the given argument defines a commit range, e.g. A..B.
+ * Note that this only validates the format but does _not_ parse it, i.e.
+ * it does _not_ look up the specified commits in the local repository.
+ */
+int specifies_commit_range(const char *range);
+
 #endif

gitgitgadget

Hi Dscho

"Johannes Schindelin via GitGitGadget" <[email protected]>

"Johannes Schindelin via GitGitGadget" <[email protected]>

I think the sharing between the two makes sense, but the helper function should make it clear in its name that this is "the kind of commit range range-diff wants to take". Among the commit range "git log" and friends can take, range-diff can take only a subset of it, and only a subset of it is meaningful to range-diff (e.g. HEAD^@ is still a commit range you can give to "git log", but it would not make much sense to give it to range-diff).

Perhaps

s/specifies_commit_range/is_range_diff_range/

or something.

True. I do not think this aims to be complete revision parser in the first place, though.

It is tempting to at least idly speculate if an approach to run setup_revisions() on argument is_range() takes and checking the result would yield a more practical solution. I would imagine that we would want to see in the resulting revs.pending has at least one positive and one negative, and none of them have SYMMETRIC_LEFT set in their .flags word.

Side note: Strictly speaking, people could wish "rev" to mean
"everything reachable from the rev, down to root", so requiring one negative may technically be a wrong thing, but in the context of "range-diff", I am not sure how useful such a behaviour would be.

Hi Junio

Yes but it affects the error message given to the user. If I run

git range-diff $(git rev-parse HEAD^{/..q}) $(git rev-parse HEAD^{/..x})

It fails immediately with

fatal: no .. in range: 'b60863619cf47b2e1e891c2376bd4f6da8111ab1'

This patch improves the error message to say it is not a range

but

git range-diff HEAD^{/..q} HEAD^{/..x}

runs for several minutes without producing any output and eventually fails with

fatal: Out of memory, malloc failed (tried to allocate 33846432676 bytes)

So I think it would be helpful to be more careful here.

Best Wishes

Phillip

Does it make so little sense to forbid passing HEAD^@ as a range to range-diff? I can imagine situations where is would make sense, e.g. I often create customer patch stacks from a set of topic branches using octopus merge. To compare two of these ^@ might be handy.

My POV is that if it's easy to use the same function (and so the same set of range descriptors) for git log and git range-diff then do so. This yields a consistent behaviour which is IMHO better than preventing people to do things that are considered strange today.

Best regards
Uwe

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Hello Junio,

It seems you understood my argument :-)

I agree that for a long history

git range-diff A B

is an expensive request and I wouldn't invest too many cycles optimizing it. (And if I'd optimize it, it wouldn't be done using textual combination of the two strings but by checking if the two ranges intersect. This way something like

	git range-diff v4.0..v4.6-rc1 v4.0..v4.5.6

and maybe even

	git range-diff v4.0..v4.6-rc1 v4.0-rc1..v4.5.6

would benefit, too. But note I'm not (anymore) familiar with the git source code, so I don't know if this is easy/sensible to do and I'm just looking at the problem from an architectural and theoretical POV.)

I don't want to make it more complicated for Dscho, I'm happy if I can in the near future use range-diff with $rev1^! $ref2^! . So feel free to ignore me.

Best regards
Uwe

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Hi Phillip,

True. But my patch series does not try to fix that (it is not an issue _introduced_ by this patch series, it was there all along).

Ciao,
Dscho

Hi Uwe,

I don't think that it makes sense to support open-ended ranges. git range-diff is expensive, its runtime is proportional to the number of patches in the first range times the number of patches in the second range. Allowing open-ended ranges will simply allow users to be stuck with a long runtime by mistake, and I do not see any valid use case in return for that risk.

Ciao,
Johannes

You can discuss for each individual syntax of a single-token range and decide which ones are and are not suitable for range-diff, but I suspect the reason behind this business started with dot-dot is to perform a superficial "sanity check" at the command line parser level before passing them to the revision machinery, and having to deal with potential errors and/or having to compare unreasonably large segments of history that the user did not intend.

Also I first thought that the command changes the behaviour, given two tokens, depending on the shape of these two tokens (i.e. when they satisfy the "is-range?" we are discussing, they are taken as two ranges to be compared, and otherwise does something else), but after revisiting the code and "git help range-diff", it always does one thing when given

(1) one arg: gives a symmetric range and what is to be compared is its left and right half,

(2) two args: each is meant to name a set of commits and these two are to be compared) or

(3) three args: each is meant to name a commit, and the arg1..arg2 and arg1..arg3 are the ranges to be compared.

so ...

... I am OK with that point of view. It certainly is simpler to explain to end users.

Having said that, it would make it much harder to implement efficiently, though. For example, when your user says

git range-diff A B

to compare "git log A" (all the way down to the root) and "git log B" (likewise), you'd certainly optimize the older common part of the history out, essentially turning it into

git range-diff A..B B..A

or its moral equivalent

git range-diff A...B

But you cannot apply such an optimization blindly. When the user gives A..B and B..A as two args, you somehow need to notice that you shouldn't rewrite it to "A..B...B..A", and for that, you'd still need some "parsing" of these args.

So, I dunno. Limiting the second form to only forms that the implementation does not have to do such optimization would certainly make it simpler for Dscho to implement ;-)

So it seems ;-).

Well, your devil's advocate can argue that accepting an input that can easily make the tool unusable would be irresponsible, though.

And there are two possible ways out:

(1) declare that optimizing "A B" into "A...B" is way too difficult to do in general, and find a good enough way to see if A and B individually gives a "range" that should be manageable; or

(2) invest cycles to optimize, so your users do not have to care.

I think the series takes the former approach, and I find it understandable.

It is a different matter if the way found and implemented in the patch is "good enough" to tell if a given argument names a manageable range. You said something about "rev^{/^here are two..dots}" that can be mistaken as a "good enough" range, but it actually names a revision and all the way down to the root.

Yup, that is in line with what I mumbled earlier about setup_revisions() and inspecting the rev_info.pending, I think.

Hi Junio,

You know what? I lied. The code already does what you suggested. Look [here](https://github.com/git/git/blob/v2.30.0/builtin/range-diff.c#L59-L77):

	[...]
        } else if (argc == 1) {
                const char *b = strstr(argv[0], "..."), *a = argv[0];
                int a_len;

                if (!b) {
                        error(_("single arg format must be symmetric range"));
                        usage_with_options(builtin_range_diff_usage, options);
                }

                a_len = (int)(b - a);
                if (!a_len) {
                        a = "HEAD";
                        a_len = strlen(a);
                }
                b += 3;
                if (!*b)
                        b = "HEAD";
                strbuf_addf(&range1, "%s..%.*s", b, a_len, a);
                strbuf_addf(&range2, "%.*s..%s", a_len, a, b);
	[...]

Yes, and the end user is talking about _two_ commit ranges in the context of git range-diff, and about _one_ commit range in the context of git log.

So I still think that it really just only resembles the symmetric range. It is fundamentally as different from it as the number 2 is different from the number 1.

Ciao,
Dscho

Hi Junio,

Maybe I am too stuck with the idea of avoiding regular expressions after that StackOverflow incident... Maybe

	static regex_t *regex;

	if (strstr(range, ".."))
		return 1;

	if (!regex) {
		regex = xmalloc(sizeof(*regex));
		if (regcomp(regex, "\\^(!|-[0-9]*)$", REG_EXTENDED))
			BUG("could not compile regex");
	}

	return !regexec(regex, range, 0, NULL, 0);

is more readable, and acceptable in this context?

I don't know. On one hand, it would make things more consistent. On the other hand, it would be relatively hard to explain, I think. And we already have the shortcut @!^, which is hard enough to explain as it is.

Ciao,
Dscho

In
https://lore.kernel.org/git/[email protected]/,
it was reported that git range-diff does not handle commit ranges like rev^!. This patch series fixes that.

Changes since v2:

* Move the helper function from revision.c to range-diff.c and rename it.
* Use a regex to make it easier to understand what we're trying to match.
* Fix the documentation that claimed that we used git merge-base internally when git range-diff parses ...-style arguments, which is not the case.

Changes since v1:

* In addition to git range-diff, git format-patch --range-diff gets the same improvement.
* The comment talking about ^@ was removed.
* The parsing was made a bit safer (e.g. catching ! by its own as an invalid range).

Johannes Schindelin (3):
range-diff/format-patch: refactor check for commit range range-diff/format-patch: handle commit ranges other than A..B range-diff(docs): explain how to specify commit ranges

 Documentation/git-range-diff.txt | 12 ++++++++++++
 builtin/log.c                    |  2 +-
 builtin/range-diff.c             |  9 +++++----
 range-diff.c                     | 17 +++++++++++++++++
 range-diff.h                     |  8 ++++++++
 t/t3206-range-diff.sh            |  8 ++++++++
 6 files changed, 51 insertions(+), 5 deletions(-)

base-commit: 71ca53e8125e36efbda17293c50027d31681a41f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-841%2Fdscho%2Frange-diff-with-ranges-lacking-dotdot-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-841/dscho/range-diff-with-ranges-lacking-dotdot-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/841

Range-diff vs v2:

1: 3f21e10f919 ! 1: b98fa94b870 range-diff/format-patch: refactor check for commit range

     @@ builtin/log.c: static void infer_range_diff_ranges(struct strbuf *r1,
       {
       	const char *head_oid = oid_to_hex(&head->object.oid);
      -	int prev_is_range = !!strstr(prev, "..");
     -+	int prev_is_range = specifies_commit_range(prev);
     ++	int prev_is_range = is_range_diff_range(prev);
       
       	if (prev_is_range)
       		strbuf_addstr(r1, prev);
     @@ builtin/range-diff.c: int cmd_range_diff(int argc, const char **argv, const char
       	if (argc == 2) {
      -		if (!strstr(argv[0], ".."))
      -			die(_("no .. in range: '%s'"), argv[0]);
     -+		if (!specifies_commit_range(argv[0]))
     ++		if (!is_range_diff_range(argv[0]))
      +			die(_("not a commit range: '%s'"), argv[0]);
       		strbuf_addstr(&range1, argv[0]);
       
      -		if (!strstr(argv[1], ".."))
      -			die(_("no .. in range: '%s'"), argv[1]);
     -+		if (!specifies_commit_range(argv[1]))
     ++		if (!is_range_diff_range(argv[1]))
      +			die(_("not a commit range: '%s'"), argv[1]);
       		strbuf_addstr(&range2, argv[1]);
       	} else if (argc == 3) {
       		strbuf_addf(&range1, "%s..%s", argv[0], argv[1]);
      
     - ## revision.c ##
     -@@ revision.c: void put_revision_mark(const struct rev_info *revs, const struct commit *commit)
     - 	fputs(mark, stdout);
     - 	putchar(' ');
     + ## range-diff.c ##
     +@@ range-diff.c: int show_range_diff(const char *range1, const char *range2,
     + 
     + 	return res;
       }
      +
     -+int specifies_commit_range(const char *range)
     ++int is_range_diff_range(const char *arg)
      +{
     -+	return !!strstr(range, "..");
     ++	return !!strstr(arg, "..");
      +}
      
     - ## revision.h ##
     -@@ revision.h: int rewrite_parents(struct rev_info *revs,
     -  */
     - struct commit_list *get_saved_parents(struct rev_info *revs, const struct commit *commit);
     + ## range-diff.h ##
     +@@ range-diff.h: int show_range_diff(const char *range1, const char *range2,
     + 		    const struct diff_options *diffopt,
     + 		    const struct strvec *other_arg);
       
      +/*
     -+ * Determine whether the given argument defines a commit range, e.g. A..B.
     -+ * Note that this only validates the format but does _not_ parse it, i.e.
     -+ * it does _not_ look up the specified commits in the local repository.
     ++ * Determine whether the given argument is usable as a range argument of `git
     ++ * range-diff`, e.g. A..B. Note that this only validates the format but does
     ++ * _not_ parse it, i.e. it does _not_ look up the specified commits in the
     ++ * local repository.
      + */
     -+int specifies_commit_range(const char *range);
     ++int is_range_diff_range(const char *arg);
      +
       #endif
 2:  2c2744333ec ! 2:  0880ca587e6 range-diff/format-patch: handle commit ranges other than A..B
     @@ Commit message
      
          Signed-off-by: Johannes Schindelin <[email protected]>
      
     - ## revision.c ##
     -@@ revision.c: void put_revision_mark(const struct rev_info *revs, const struct commit *commit)
     + ## range-diff.c ##
     +@@ range-diff.c: int show_range_diff(const char *range1, const char *range2,
       
     - int specifies_commit_range(const char *range)
     + int is_range_diff_range(const char *arg)
       {
     --	return !!strstr(range, "..");
     -+	size_t i;
     -+	char c;
     +-	return !!strstr(arg, "..");
     ++	static regex_t *regex;
      +
     -+	if (strstr(range, ".."))
     ++	if (strstr(arg, ".."))
      +		return 1;
      +
     -+	i = strlen(range);
     -+	c = i > 2 ? range[--i] : 0;
     -+	if (c == '!')
     -+		i--; /* might be ...^! */
     -+	else if (isdigit(c)) {
     -+		/* handle ...^-<n> */
     -+		while (i > 2 && isdigit(range[--i]))
     -+			; /* keep trimming trailing digits */
     -+		if (i < 2 || range[i--] != '-')
     -+			return 0;
     -+	} else
     -+		return 0;
     ++	/* match `<rev>^!` and `<rev>^-<n>` */
     ++	if (!regex) {
     ++		regex = xmalloc(sizeof(*regex));
     ++		if (regcomp(regex, "\\^(!|-[0-9]*)$", REG_EXTENDED) < 0)
     ++			BUG("could not compile range-diff regex");
     ++	}
      +
     -+	/* Before the `!` or the `-<n>`, we expect `<rev>^` */
     -+	return i > 0 && range[i] == '^';
     ++	return !regexec(regex, arg, 0, NULL, 0);
       }
      
       ## t/t3206-range-diff.sh ##

3: 4f5e5acd954 ! 3: 5ab9321a34c range-diff(docs): explain how to specify commit ranges
@@ Documentation/git-range-diff.txt: Finally, the list of matching commits is shown
+
+- <rev1>...<rev2>. This resembles the symmetric ranges mentioned in
+ the SPECIFYING RANGES section of linkgit:gitrevisions[7], and is
-+ equivalent to <base>..<rev1> <base>..<rev2> where <base> is the -+ merge base as obtained via git merge-base <rev1> <rev2>.
++ equivalent to <rev2>..<rev1> <rev1>..<rev2>.
+
+- <base> <rev1> <rev2>: This is equivalent to `<base>..<rev1>
+ <base>..<rev2>`.

gitgitgadget

From: Johannes Schindelin <[email protected]>

There are three forms, depending whether the user specifies one, two or three non-option arguments. We've never actually explained how this works in the manual, so let's explain it.

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

index 9701c1e5fdd..14bffb272a0 100644
@@ -28,6 +28,18 @@ Finally, the list of matching commits is shown in the order of the
 second commit range, with unmatched commits being inserted just after
 all of their ancestors have been shown.
 
+There are three ways to specify the commit ranges:
+
+- `<range1> <range2>`: Either commit range can be of the form
+  `<base>..<rev>`, `<rev>^!` or `<rev>^-<n>`. See `SPECIFYING RANGES`
+  in linkgit:gitrevisions[7] for more details.
+
+- `<rev1>...<rev2>`. This resembles the symmetric ranges mentioned in
+  the `SPECIFYING RANGES` section of linkgit:gitrevisions[7], and is
+  equivalent to `<rev2>..<rev1> <rev1>..<rev2>`.
+
+- `<base> <rev1> <rev2>`: This is equivalent to `<base>..<rev1>
+  <base>..<rev2>`.
 
 OPTIONS
 -------

gitgitgadget

From: Johannes Schindelin <[email protected]>

In the SPECIFYING RANGES section of gitrevisions[7], two ways are described to specify commit ranges that range-diff does not yet
accept: "<commit>^!" and "<commit>^-<n>".

Let's accept them.

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

index 9b93e08e840..0c6ac4f954d 100644
@@ -567,5 +567,17 @@ int show_range_diff(const char *range1, const char *range2,
 
 int is_range_diff_range(const char *arg)
 {
-	return !!strstr(arg, "..");
+	static regex_t *regex;
+
+	if (strstr(arg, ".."))
+		return 1;
+
+	/* match `<rev>^!` and `<rev>^-<n>` */
+	if (!regex) {
+		regex = xmalloc(sizeof(*regex));
+		if (regcomp(regex, "\\^(!|-[0-9]*)$", REG_EXTENDED) < 0)
+			BUG("could not compile range-diff regex");
+	}
+
+	return !regexec(regex, arg, 0, NULL, 0);
 }

index 6eb344be031..e217cecac9e 100755
@@ -150,6 +150,14 @@ test_expect_success 'simple A B C (unmodified)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'A^! and A^-<n> (unmodified)' '
+	git range-diff --no-color topic^! unmodified^-1 >actual &&
+	cat >expect <<-EOF &&
+	1:  $(test_oid t4) = 1:  $(test_oid u4) s/12/B/
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'trivial reordering' '
 	git range-diff --no-color master topic reordered >actual &&
 	cat >expect <<-EOF &&

gitgitgadget

From: Johannes Schindelin <[email protected]>

Currently, when called with exactly two arguments, git range-diff tests for a literal .. in each of the two. Likewise, the argument provided via --range-diff to git format-patch is checked in the same manner.

However, <commit>^! is a perfectly valid commit range, equivalent to <commit>^..<commit> according to the SPECIFYING RANGES section of gitrevisions[7].

In preparation for allowing more sophisticated ways to specify commit ranges, let's refactor the check into its own function.

---
 builtin/log.c        | 2 +-
 builtin/range-diff.c | 9 +++++----
 range-diff.c         | 5 +++++
 range-diff.h         | 8 ++++++++
 4 files changed, 19 insertions(+), 5 deletions(-)

index bd6ff4f9f95..aeece57e86a 100644
@@ -1680,7 +1680,7 @@ static void infer_range_diff_ranges(struct strbuf *r1,
 				    struct commit *head)
 {
 	const char *head_oid = oid_to_hex(&head->object.oid);
-	int prev_is_range = !!strstr(prev, "..");
+	int prev_is_range = is_range_diff_range(prev);
 
 	if (prev_is_range)
 		strbuf_addstr(r1, prev);

index 24c4162f744..5b1f6326322 100644
@@ -3,6 +3,7 @@
 #include "parse-options.h"
 #include "range-diff.h"
 #include "config.h"
+#include "revision.h"
 
 static const char * const builtin_range_diff_usage[] = {
 N_("git range-diff [<options>] <old-base>..<old-tip> <new-base>..<new-tip>"),

@@ -46,12 +47,12 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 		diffopt.use_color = 1;
 
 	if (argc == 2) {
-		if (!strstr(argv[0], ".."))
-			die(_("no .. in range: '%s'"), argv[0]);
+		if (!is_range_diff_range(argv[0]))
+			die(_("not a commit range: '%s'"), argv[0]);
 		strbuf_addstr(&range1, argv[0]);
 
-		if (!strstr(argv[1], ".."))
-			die(_("no .. in range: '%s'"), argv[1]);
+		if (!is_range_diff_range(argv[1]))
+			die(_("not a commit range: '%s'"), argv[1]);
 		strbuf_addstr(&range2, argv[1]);
 	} else if (argc == 3) {
 		strbuf_addf(&range1, "%s..%s", argv[0], argv[1]);

index b9950f10c8c..9b93e08e840 100644
@@ -564,3 +564,8 @@ int show_range_diff(const char *range1, const char *range2,
 
 	return res;
 }
+
+int is_range_diff_range(const char *arg)
+{
+	return !!strstr(arg, "..");
+}

index 583ced2e8e7..c17dbc2e75a 100644
@@ -16,4 +16,12 @@ int show_range_diff(const char *range1, const char *range2,
 		    const struct diff_options *diffopt,
 		    const struct strvec *other_arg);
 
+/*
+ * Determine whether the given argument is usable as a range argument of `git
+ * range-diff`, e.g. A..B. Note that this only validates the format but does
+ * _not_ parse it, i.e. it does _not_ look up the specified commits in the
+ * local repository.
+ */
+int is_range_diff_range(const char *arg);
+
 #endif

gitgitgadget

I knew. So we need an update to the patch.

You are forgetting that "log A...B" shows only one half of what a symmetric range means, and hides which side each commit belongs to.

"git log --left-right A...B" shows what a symmetric range really is; there are two sides, left and right, and "git range-diff A...B" is a natural way to compare these two ranges.

I've been quite happy with the way how "git range-diff @{-1}..." shows the comparison of two sides of the symmetric range, given by "git log --oneline --left-right @{-1}..."

Readable, yes, acceptable, I don't know, and I am not even sure if I want to be the one to judge to be honest ;-)

Have you tried the approach to feed the thing to setup_revisions() and inspect what objects are in revs.pending?

When you got a valid range, you should find one or more positive and one or more negative commits , and the approach won't be fooled by strings like "HEAD^{/other than A..B/}".

Or does the revision parsing machinery too eager to "die" when we find a syntax error? If so, scratch that idea, but in the longer haul, fixing these die's would also be something we'd want to do to make more parts of libgit.a callable as a proper library.

Thanks.

Hi Junio,

I hadn't thought of that.

It should probably be libified anyway if it calles die() (I don't know off the top of my head).

Worse than the die() is probably the allocated memory; IIRC there is no way to release the memory allocated by setup_revisions() unless one performs a revwalk.

Will try to find some time to play with the setup_revisions() idea.

Ciao,
Dscho

In
https://lore.kernel.org/git/[email protected]/,
it was reported that git range-diff does not handle commit ranges like rev^!. This patch series fixes that.

Changes since v3:

* The revision machinery is now used directly to validate the commit ranges.

Changes since v2:

* Move the helper function from revision.c to range-diff.c and rename it.
* Use a regex to make it easier to understand what we're trying to match.
* Fix the documentation that claimed that we used git merge-base internally when git range-diff parses ...-style arguments, which is not the case.

Changes since v1:

* In addition to git range-diff, git format-patch --range-diff gets the same improvement.
* The comment talking about ^@ was removed.
* The parsing was made a bit safer (e.g. catching ! by its own as an invalid range).

Johannes Schindelin (3):
range-diff/format-patch: refactor check for commit range range-diff/format-patch: handle commit ranges other than A..B range-diff(docs): explain how to specify commit ranges

 Documentation/git-range-diff.txt | 12 ++++++++++++
 builtin/log.c                    |  2 +-
 builtin/range-diff.c             |  9 +++++----
 range-diff.c                     | 22 ++++++++++++++++++++++
 range-diff.h                     |  8 ++++++++
 t/t3206-range-diff.sh            |  8 ++++++++
 6 files changed, 56 insertions(+), 5 deletions(-)

base-commit: 71ca53e8125e36efbda17293c50027d31681a41f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-841%2Fdscho%2Frange-diff-with-ranges-lacking-dotdot-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-841/dscho/range-diff-with-ranges-lacking-dotdot-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/841

Range-diff vs v3:

 1:  b98fa94b8703 = 1:  b98fa94b8703 range-diff/format-patch: refactor check for commit range
 2:  0880ca587e63 ! 2:  448e6a64fa15 range-diff/format-patch: handle commit ranges other than A..B
     @@ Commit message
          described to specify commit ranges that `range-diff` does not yet
          accept: "<commit>^!" and "<commit>^-<n>".
      
     -    Let's accept them.
     +    Let's accept them, by parsing them via the revision machinery and
     +    looking for at least one interesting and one uninteresting revision in
     +    the resulting `pending` array.
      
          Signed-off-by: Johannes Schindelin <[email protected]>
      
       ## range-diff.c ##
     +@@
     + #include "pretty.h"
     + #include "userdiff.h"
     + #include "apply.h"
     ++#include "revision.h"
     + 
     + struct patch_util {
     + 	/* For the search for an exact match */
      @@ range-diff.c: int show_range_diff(const char *range1, const char *range2,
       
       int is_range_diff_range(const char *arg)
       {
      -	return !!strstr(arg, "..");
     -+	static regex_t *regex;
     -+
     -+	if (strstr(arg, ".."))
     -+		return 1;
     ++	char *copy = xstrdup(arg); /* setup_revisions() modifies it */
     ++	const char *argv[] = { "", copy, "--", NULL };
     ++	int i, positive = 0, negative = 0;
     ++	struct rev_info revs;
      +
     -+	/* match `<rev>^!` and `<rev>^-<n>` */
     -+	if (!regex) {
     -+		regex = xmalloc(sizeof(*regex));
     -+		if (regcomp(regex, "\\^(!|-[0-9]*)$", REG_EXTENDED) < 0)
     -+			BUG("could not compile range-diff regex");
     ++	init_revisions(&revs, NULL);
     ++	if (setup_revisions(3, argv, &revs, 0) == 1) {
     ++		for (i = 0; i < revs.pending.nr; i++)
     ++			if (revs.pending.objects[i].item->flags & UNINTERESTING)
     ++				negative++;
     ++			else
     ++				positive++;
      +	}
      +
     -+	return !regexec(regex, arg, 0, NULL, 0);
     ++	free(copy);
     ++	object_array_clear(&revs.pending);
     ++	return negative > 0 && positive > 0;
       }
      
       ## t/t3206-range-diff.sh ##

3: 5ab9321a34ca = 3: 295fdc1cd32c range-diff(docs): explain how to specify commit ranges

gitgitgadget

From: Johannes Schindelin <[email protected]>

Currently, when called with exactly two arguments, git range-diff tests for a literal .. in each of the two. Likewise, the argument provided via --range-diff to git format-patch is checked in the same manner.

However, <commit>^! is a perfectly valid commit range, equivalent to <commit>^..<commit> according to the SPECIFYING RANGES section of gitrevisions[7].

In preparation for allowing more sophisticated ways to specify commit ranges, let's refactor the check into its own function.

---
 builtin/log.c        | 2 +-
 builtin/range-diff.c | 9 +++++----
 range-diff.c         | 5 +++++
 range-diff.h         | 8 ++++++++
 4 files changed, 19 insertions(+), 5 deletions(-)

index bd6ff4f9f956..aeece57e86a2 100644
@@ -1680,7 +1680,7 @@ static void infer_range_diff_ranges(struct strbuf *r1,
 				    struct commit *head)
 {
 	const char *head_oid = oid_to_hex(&head->object.oid);
-	int prev_is_range = !!strstr(prev, "..");
+	int prev_is_range = is_range_diff_range(prev);
 
 	if (prev_is_range)
 		strbuf_addstr(r1, prev);

index 24c4162f7446..5b1f6326322f 100644
@@ -3,6 +3,7 @@
 #include "parse-options.h"
 #include "range-diff.h"
 #include "config.h"
+#include "revision.h"
 
 static const char * const builtin_range_diff_usage[] = {
 N_("git range-diff [<options>] <old-base>..<old-tip> <new-base>..<new-tip>"),

@@ -46,12 +47,12 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 		diffopt.use_color = 1;
 
 	if (argc == 2) {
-		if (!strstr(argv[0], ".."))
-			die(_("no .. in range: '%s'"), argv[0]);
+		if (!is_range_diff_range(argv[0]))
+			die(_("not a commit range: '%s'"), argv[0]);
 		strbuf_addstr(&range1, argv[0]);
 
-		if (!strstr(argv[1], ".."))
-			die(_("no .. in range: '%s'"), argv[1]);
+		if (!is_range_diff_range(argv[1]))
+			die(_("not a commit range: '%s'"), argv[1]);
 		strbuf_addstr(&range2, argv[1]);
 	} else if (argc == 3) {
 		strbuf_addf(&range1, "%s..%s", argv[0], argv[1]);

index b9950f10c8c4..9b93e08e8407 100644
@@ -564,3 +564,8 @@ int show_range_diff(const char *range1, const char *range2,
 
 	return res;
 }
+
+int is_range_diff_range(const char *arg)
+{
+	return !!strstr(arg, "..");
+}

index 583ced2e8e74..c17dbc2e75a8 100644
@@ -16,4 +16,12 @@ int show_range_diff(const char *range1, const char *range2,
 		    const struct diff_options *diffopt,
 		    const struct strvec *other_arg);
 
+/*
+ * Determine whether the given argument is usable as a range argument of `git
+ * range-diff`, e.g. A..B. Note that this only validates the format but does
+ * _not_ parse it, i.e. it does _not_ look up the specified commits in the
+ * local repository.
+ */
+int is_range_diff_range(const char *arg);
+
 #endif

gitgitgadget

From: Johannes Schindelin <[email protected]>

In the SPECIFYING RANGES section of gitrevisions[7], two ways are described to specify commit ranges that range-diff does not yet
accept: "<commit>^!" and "<commit>^-<n>".

Let's accept them, by parsing them via the revision machinery and looking for at least one interesting and one uninteresting revision in the resulting pending array.

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

index 9b93e08e8407..07e212d5bb8c 100644
@@ -11,6 +11,7 @@
 #include "pretty.h"
 #include "userdiff.h"
 #include "apply.h"
+#include "revision.h"
 
 struct patch_util {
 	/* For the search for an exact match */

@@ -567,5 +568,21 @@ int show_range_diff(const char *range1, const char *range2,
 
 int is_range_diff_range(const char *arg)
 {
-	return !!strstr(arg, "..");
+	char *copy = xstrdup(arg); /* setup_revisions() modifies it */
+	const char *argv[] = { "", copy, "--", NULL };
+	int i, positive = 0, negative = 0;
+	struct rev_info revs;
+
+	init_revisions(&revs, NULL);
+	if (setup_revisions(3, argv, &revs, 0) == 1) {
+		for (i = 0; i < revs.pending.nr; i++)
+			if (revs.pending.objects[i].item->flags & UNINTERESTING)
+				negative++;
+			else
+				positive++;
+	}
+
+	free(copy);
+	object_array_clear(&revs.pending);
+	return negative > 0 && positive > 0;
 }

index 6eb344be0312..e217cecac9ed 100755
@@ -150,6 +150,14 @@ test_expect_success 'simple A B C (unmodified)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'A^! and A^-<n> (unmodified)' '
+	git range-diff --no-color topic^! unmodified^-1 >actual &&
+	cat >expect <<-EOF &&
+	1:  $(test_oid t4) = 1:  $(test_oid u4) s/12/B/
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'trivial reordering' '
 	git range-diff --no-color master topic reordered >actual &&
 	cat >expect <<-EOF &&

gitgitgadget

From: Johannes Schindelin <[email protected]>

There are three forms, depending whether the user specifies one, two or three non-option arguments. We've never actually explained how this works in the manual, so let's explain it.

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

index 9701c1e5fdd5..14bffb272a06 100644
@@ -28,6 +28,18 @@ Finally, the list of matching commits is shown in the order of the
 second commit range, with unmatched commits being inserted just after
 all of their ancestors have been shown.
 
+There are three ways to specify the commit ranges:
+
+- `<range1> <range2>`: Either commit range can be of the form
+  `<base>..<rev>`, `<rev>^!` or `<rev>^-<n>`. See `SPECIFYING RANGES`
+  in linkgit:gitrevisions[7] for more details.
+
+- `<rev1>...<rev2>`. This resembles the symmetric ranges mentioned in
+  the `SPECIFYING RANGES` section of linkgit:gitrevisions[7], and is
+  equivalent to `<rev2>..<rev1> <rev1>..<rev2>`.
+
+- `<base> <rev1> <rev2>`: This is equivalent to `<base>..<rev1>
+  <base>..<rev2>`.
 
 OPTIONS
 -------

gitgitgadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

"Johannes Schindelin via GitGitGadget" <[email protected]>

"Johannes Schindelin via GitGitGadget" <[email protected]>

"Johannes Schindelin via GitGitGadget" <[email protected]>

    "HEAD^{/^string with .. in it}"

Hi Junio,

Fair. I will add the sentence in 1/3 (because it is still correct there) and remove it as part of 2/3.

Ciao,
Dscho

Hi Junio,

Yes, we should ;-)

The code in handle_dotdot_1() that handles ... calls get_merge_bases(), which eventually calls get_merge_bases_many_0() with the cleanup parameter set to 1, i.e. it _does_ clean up the flags.

Otherwise, git log A...B couldn't work, could it? ;-)

Is this cleared, like, ever? I don't see any code that handles cmdline that way. Seems as we're willfully leaking this.

Ciao,
Dscho

Hi Junio,

Sure, why not.

Ciao,
Dscho

Hi Junio,

If we were talking about one commit range instead of two, I would agree with you.

To put an end to this pointless discussion, I struck the sentence from the documentation. It might even make it easier to read for users. So that's a win.

Ciao,
Dscho

It is one symmetric commit range A...B that has two sides, i.e. B..A on the left side and A..B on the right side.

In
https://lore.kernel.org/git/[email protected]/,
it was reported that git range-diff does not handle commit ranges like rev^!. This patch series fixes that.

Changes since v4:

* The commit marks are now cleared in is_range_diff_range().
* A regression test now verifies that HEAD^{/something..or other} isn't mistaken for a commit range.
* The manual page no longer mentions "symmetric range", to avoid contentious language.

Changes since v3:

* The revision machinery is now used directly to validate the commit ranges.

Changes since v2:

* Move the helper function from revision.c to range-diff.c and rename it.
* Use a regex to make it easier to understand what we're trying to match.
* Fix the documentation that claimed that we used git merge-base internally when git range-diff parses ...-style arguments, which is not the case.

Changes since v1:

* In addition to git range-diff, git format-patch --range-diff gets the same improvement.
* The comment talking about ^@ was removed.
* The parsing was made a bit safer (e.g. catching ! by its own as an invalid range).

Johannes Schindelin (3):
range-diff/format-patch: refactor check for commit range range-diff/format-patch: handle commit ranges other than A..B range-diff(docs): explain how to specify commit ranges

 Documentation/git-range-diff.txt | 11 +++++++++++
 builtin/log.c                    |  2 +-
 builtin/range-diff.c             |  9 +++++----
 range-diff.c                     | 27 +++++++++++++++++++++++++++
 range-diff.h                     |  6 ++++++
 t/t3206-range-diff.sh            | 13 +++++++++++++
 6 files changed, 63 insertions(+), 5 deletions(-)

base-commit: 71ca53e8125e36efbda17293c50027d31681a41f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-841%2Fdscho%2Frange-diff-with-ranges-lacking-dotdot-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-841/dscho/range-diff-with-ranges-lacking-dotdot-v5
Pull-Request: https://github.com/gitgitgadget/git/pull/841

Range-diff vs v4:

 1:  b98fa94b8703 = 1:  b98fa94b8703 range-diff/format-patch: refactor check for commit range
 2:  448e6a64fa15 ! 2:  04b5d75adbc3 range-diff/format-patch: handle commit ranges other than A..B
     @@ Commit message
          looking for at least one interesting and one uninteresting revision in
          the resulting `pending` array.
      
     +    This also finally lets us reject arguments that _do_ contain `..` but
     +    are not actually ranges, e.g. `HEAD^{/do.. match this}`.
     +
          Signed-off-by: Johannes Schindelin <[email protected]>
      
       ## range-diff.c ##
     @@ range-diff.c: int show_range_diff(const char *range1, const char *range2,
      +	struct rev_info revs;
      +
      +	init_revisions(&revs, NULL);
     -+	if (setup_revisions(3, argv, &revs, 0) == 1) {
     -+		for (i = 0; i < revs.pending.nr; i++)
     -+			if (revs.pending.objects[i].item->flags & UNINTERESTING)
     ++	if (setup_revisions(3, argv, &revs, 0) == 1)
     ++		for (i = 0; i < revs.pending.nr; i++) {
     ++			struct object *obj = revs.pending.objects[i].item;
     ++
     ++			if (obj->flags & UNINTERESTING)
      +				negative++;
      +			else
      +				positive++;
     -+	}
     ++			if (obj->type == OBJ_COMMIT)
     ++				clear_commit_marks((struct commit *)obj,
     ++						   ALL_REV_FLAGS);
     ++		}
      +
      +	free(copy);
      +	object_array_clear(&revs.pending);
      +	return negative > 0 && positive > 0;
       }
      
     + ## range-diff.h ##
     +@@ range-diff.h: int show_range_diff(const char *range1, const char *range2,
     + 
     + /*
     +  * Determine whether the given argument is usable as a range argument of `git
     +- * range-diff`, e.g. A..B. Note that this only validates the format but does
     +- * _not_ parse it, i.e. it does _not_ look up the specified commits in the
     +- * local repository.
     ++ * range-diff`, e.g. A..B.
     +  */
     + int is_range_diff_range(const char *arg);
     + 
     +
       ## t/t3206-range-diff.sh ##
      @@ t/t3206-range-diff.sh: test_expect_success 'simple A B C (unmodified)' '
       	test_cmp expect actual
     @@ t/t3206-range-diff.sh: test_expect_success 'simple A B C (unmodified)' '
      +	EOF
      +	test_cmp expect actual
      +'
     ++
     ++test_expect_success 'A^{/..} is not mistaken for a range' '
     ++	test_must_fail git range-diff topic^.. topic^{/..} 2>error &&
     ++	test_i18ngrep "not a commit rang" error
     ++'
      +
       test_expect_success 'trivial reordering' '
       	git range-diff --no-color master topic reordered >actual &&

3: 295fdc1cd32c ! 3: bc5de807735d range-diff(docs): explain how to specify commit ranges
@@ Documentation/git-range-diff.txt: Finally, the list of matching commits is shown
+ <base>..<rev>, <rev>^! or <rev>^-<n>. See SPECIFYING RANGES
+ in linkgit:gitrevisions[7] for more details.
+
-+- <rev1>...<rev2>. This resembles the symmetric ranges mentioned in -+ the SPECIFYING RANGES section of linkgit:gitrevisions[7], and is -+ equivalent to <rev2>..<rev1> <rev1>..<rev2>.
++- <rev1>...<rev2>. This is equivalent to
++ <rev2>..<rev1> <rev1>..<rev2>.
+
+- <base> <rev1> <rev2>: This is equivalent to `<base>..<rev1>
+ <base>..<rev2>`.

gitgitgadget

From: Johannes Schindelin <[email protected]>

There are three forms, depending whether the user specifies one, two or three non-option arguments. We've never actually explained how this works in the manual, so let's explain it.

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

index 9701c1e5fdd5..a968d5237dae 100644
@@ -28,6 +28,17 @@ Finally, the list of matching commits is shown in the order of the
 second commit range, with unmatched commits being inserted just after
 all of their ancestors have been shown.
 
+There are three ways to specify the commit ranges:
+
+- `<range1> <range2>`: Either commit range can be of the form
+  `<base>..<rev>`, `<rev>^!` or `<rev>^-<n>`. See `SPECIFYING RANGES`
+  in linkgit:gitrevisions[7] for more details.
+
+- `<rev1>...<rev2>`. This is equivalent to
+  `<rev2>..<rev1> <rev1>..<rev2>`.
+
+- `<base> <rev1> <rev2>`: This is equivalent to `<base>..<rev1>
+  <base>..<rev2>`.
 
 OPTIONS
 -------

gitgitgadget

From: Johannes Schindelin <[email protected]>

Currently, when called with exactly two arguments, git range-diff tests for a literal .. in each of the two. Likewise, the argument provided via --range-diff to git format-patch is checked in the same manner.

However, <commit>^! is a perfectly valid commit range, equivalent to <commit>^..<commit> according to the SPECIFYING RANGES section of gitrevisions[7].

In preparation for allowing more sophisticated ways to specify commit ranges, let's refactor the check into its own function.

---
 builtin/log.c        | 2 +-
 builtin/range-diff.c | 9 +++++----
 range-diff.c         | 5 +++++
 range-diff.h         | 8 ++++++++
 4 files changed, 19 insertions(+), 5 deletions(-)

index bd6ff4f9f956..aeece57e86a2 100644
@@ -1680,7 +1680,7 @@ static void infer_range_diff_ranges(struct strbuf *r1,
 				    struct commit *head)
 {
 	const char *head_oid = oid_to_hex(&head->object.oid);
-	int prev_is_range = !!strstr(prev, "..");
+	int prev_is_range = is_range_diff_range(prev);
 
 	if (prev_is_range)
 		strbuf_addstr(r1, prev);

index 24c4162f7446..5b1f6326322f 100644
@@ -3,6 +3,7 @@
 #include "parse-options.h"
 #include "range-diff.h"
 #include "config.h"
+#include "revision.h"
 
 static const char * const builtin_range_diff_usage[] = {
 N_("git range-diff [<options>] <old-base>..<old-tip> <new-base>..<new-tip>"),

@@ -46,12 +47,12 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 		diffopt.use_color = 1;
 
 	if (argc == 2) {
-		if (!strstr(argv[0], ".."))
-			die(_("no .. in range: '%s'"), argv[0]);
+		if (!is_range_diff_range(argv[0]))
+			die(_("not a commit range: '%s'"), argv[0]);
 		strbuf_addstr(&range1, argv[0]);
 
-		if (!strstr(argv[1], ".."))
-			die(_("no .. in range: '%s'"), argv[1]);
+		if (!is_range_diff_range(argv[1]))
+			die(_("not a commit range: '%s'"), argv[1]);
 		strbuf_addstr(&range2, argv[1]);
 	} else if (argc == 3) {
 		strbuf_addf(&range1, "%s..%s", argv[0], argv[1]);

index b9950f10c8c4..9b93e08e8407 100644
@@ -564,3 +564,8 @@ int show_range_diff(const char *range1, const char *range2,
 
 	return res;
 }
+
+int is_range_diff_range(const char *arg)
+{
+	return !!strstr(arg, "..");
+}

index 583ced2e8e74..c17dbc2e75a8 100644
@@ -16,4 +16,12 @@ int show_range_diff(const char *range1, const char *range2,
 		    const struct diff_options *diffopt,
 		    const struct strvec *other_arg);
 
+/*
+ * Determine whether the given argument is usable as a range argument of `git
+ * range-diff`, e.g. A..B. Note that this only validates the format but does
+ * _not_ parse it, i.e. it does _not_ look up the specified commits in the
+ * local repository.
+ */
+int is_range_diff_range(const char *arg);
+
 #endif

gitgitgadget

From: Johannes Schindelin <[email protected]>

In the SPECIFYING RANGES section of gitrevisions[7], two ways are described to specify commit ranges that range-diff does not yet
accept: "<commit>^!" and "<commit>^-<n>".

Let's accept them, by parsing them via the revision machinery and looking for at least one interesting and one uninteresting revision in the resulting pending array.

This also finally lets us reject arguments that _do_ contain .. but are not actually ranges, e.g. HEAD^{/do.. match this}.

---
 range-diff.c          | 24 +++++++++++++++++++++++-
 range-diff.h          |  4 +---
 t/t3206-range-diff.sh | 13 +++++++++++++
 3 files changed, 37 insertions(+), 4 deletions(-)

index 9b93e08e8407..c307bca9de23 100644
@@ -11,6 +11,7 @@
 #include "pretty.h"
 #include "userdiff.h"
 #include "apply.h"
+#include "revision.h"
 
 struct patch_util {
 	/* For the search for an exact match */

@@ -567,5 +568,26 @@ int show_range_diff(const char *range1, const char *range2,
 
 int is_range_diff_range(const char *arg)
 {
-	return !!strstr(arg, "..");
+	char *copy = xstrdup(arg); /* setup_revisions() modifies it */
+	const char *argv[] = { "", copy, "--", NULL };
+	int i, positive = 0, negative = 0;
+	struct rev_info revs;
+
+	init_revisions(&revs, NULL);
+	if (setup_revisions(3, argv, &revs, 0) == 1)
+		for (i = 0; i < revs.pending.nr; i++) {
+			struct object *obj = revs.pending.objects[i].item;
+
+			if (obj->flags & UNINTERESTING)
+				negative++;
+			else
+				positive++;
+			if (obj->type == OBJ_COMMIT)
+				clear_commit_marks((struct commit *)obj,
+						   ALL_REV_FLAGS);
+		}
+
+	free(copy);
+	object_array_clear(&revs.pending);
+	return negative > 0 && positive > 0;
 }

index c17dbc2e75a8..4abd70c40fed 100644
@@ -18,9 +18,7 @@ int show_range_diff(const char *range1, const char *range2,
 
 /*
  * Determine whether the given argument is usable as a range argument of `git
- * range-diff`, e.g. A..B. Note that this only validates the format but does
- * _not_ parse it, i.e. it does _not_ look up the specified commits in the
- * local repository.
+ * range-diff`, e.g. A..B.
  */
 int is_range_diff_range(const char *arg);
 

index 6eb344be0312..45f21ee215d7 100755
@@ -150,6 +150,19 @@ test_expect_success 'simple A B C (unmodified)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'A^! and A^-<n> (unmodified)' '
+	git range-diff --no-color topic^! unmodified^-1 >actual &&
+	cat >expect <<-EOF &&
+	1:  $(test_oid t4) = 1:  $(test_oid u4) s/12/B/
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'A^{/..} is not mistaken for a range' '
+	test_must_fail git range-diff topic^.. topic^{/..} 2>error &&
+	test_i18ngrep "not a commit rang" error
+'
+
 test_expect_success 'trivial reordering' '
 	git range-diff --no-color master topic reordered >actual &&
 	cat >expect <<-EOF &&

gitgitgadget

"Johannes Schindelin via GitGitGadget" <[email protected]>

	int negpos = 0;

	if (setup_revisions(3, argv, &revs, 0) != 1)
		goto cleanup;
	for (i = 0; i < revs.pending.nr && negpos != 3; i++) {
		struct object *obj = revs.pending.objects[i].item;
		negpos |= (obj->flags & UNINTERESTING) ? 01 : 02;
	}

	for (i = 0; i < revs.pending.nr; i++) {
		struct object *obj = revs.pending.objects[i].item;
                if (obj->type == OBJ_COMMIT)
			clear_commit_marks((struct commit *)obj, ALL_REV_FLAGS);
	}

    cleanup:

Here is what I added on top of the 3-patch series for tonight's pushout.

* No need to count positive/negative; just seeing both exists is sufficient and there is no need to examine any later elements, if any.

* Clearing commit marks needs to be done after we have seen enough, or it can clear the stuff we would want to inspect before we have a chance to. Do it in a separate post-cleaning loop.

* Dedent by judicious use of 'goto'.

* The last parameter to setup_revisions() is a pointer, so spell it NULL not 0.

* "rang" -> "range" typofix (it might be even better to look for "range:")

----- >8 ---------- >8 ---------- >8 ---------- >8 ---------- >8 ---------- >8 -----
Subject: [PATCH] fixup! range-diff/format-patch: handle commit ranges other than A..B

 range-diff.c          | 30 +++++++++++++++++-------------
 t/t3206-range-diff.sh |  2 +-
 2 files changed, 18 insertions(+), 14 deletions(-)

index c307bca9de..7a38dc8715 100644
@@ -570,24 +570,28 @@ int is_range_diff_range(const char *arg)
 {
 	char *copy = xstrdup(arg); /* setup_revisions() modifies it */
 	const char *argv[] = { "", copy, "--", NULL };
-	int i, positive = 0, negative = 0;
+	int i;
 	struct rev_info revs;
+	unsigned npmask = 0;
 
 	init_revisions(&revs, NULL);
-	if (setup_revisions(3, argv, &revs, 0) == 1)
-		for (i = 0; i < revs.pending.nr; i++) {
-			struct object *obj = revs.pending.objects[i].item;
+	if (setup_revisions(3, argv, &revs, NULL) != 1)
+		goto cleanup;
 
-			if (obj->flags & UNINTERESTING)
-				negative++;
-			else
-				positive++;
-			if (obj->type == OBJ_COMMIT)
-				clear_commit_marks((struct commit *)obj,
-						   ALL_REV_FLAGS);
-		}
+	for (i = 0; i < revs.pending.nr && npmask != 3; i++) {
+		struct object *obj = revs.pending.objects[i].item;
+
+		npmask |= (obj->flags & UNINTERESTING) ? 01 : 02;
+	}
+
+	for (i = 0; i < revs.pending.nr; i++) {
+		struct object *obj = revs.pending.objects[i].item;
+		if (obj->type == OBJ_COMMIT)
+			clear_commit_marks((struct commit *)obj, ALL_REV_FLAGS);
+	}
 
+cleanup:
 	free(copy);
 	object_array_clear(&revs.pending);
-	return negative > 0 && positive > 0;
+	return npmask == 3;
 }

index 45f21ee215..2b518378d4 100755
@@ -160,7 +160,7 @@ test_expect_success 'A^! and A^-<n> (unmodified)' '
 
 test_expect_success 'A^{/..} is not mistaken for a range' '
 	test_must_fail git range-diff topic^.. topic^{/..} 2>error &&
-	test_i18ngrep "not a commit rang" error
+	test_i18ngrep "not a commit range" error
 '
 
 test_expect_success 'trivial reordering' '

2.30.0-601-g9b6178ed87

From: Johannes Schindelin <[email protected]>

In the SPECIFYING RANGES section of gitrevisions[7], two ways are described to specify commit ranges that range-diff does not yet
accept: "<commit>^!" and "<commit>^-<n>".

Let's accept them, by parsing them via the revision machinery and looking for at least one interesting and one uninteresting revision in the resulting pending array.

This also finally lets us reject arguments that _do_ contain .. but are not actually ranges, e.g. HEAD^{/do.. match this}.

---
 range-diff.c          | 26 +++++++++++++++++++++++++-
 range-diff.h          |  4 +---
 t/t3206-range-diff.sh | 13 +++++++++++++
 3 files changed, 39 insertions(+), 4 deletions(-)

index 9b93e08e8407..a88612cb8923 100644
@@ -11,6 +11,7 @@
 #include "pretty.h"
 #include "userdiff.h"
 #include "apply.h"
+#include "revision.h"
 
 struct patch_util {
 	/* For the search for an exact match */

@@ -567,5 +568,28 @@ int show_range_diff(const char *range1, const char *range2,
 
 int is_range_diff_range(const char *arg)
 {
-	return !!strstr(arg, "..");
+	char *copy = xstrdup(arg); /* setup_revisions() modifies it */
+	const char *argv[] = { "", copy, "--", NULL };
+	int i, positive = 0, negative = 0;
+	struct rev_info revs;
+
+	init_revisions(&revs, NULL);
+	if (setup_revisions(3, argv, &revs, NULL) == 1) {
+		for (i = 0; i < revs.pending.nr; i++)
+			if (revs.pending.objects[i].item->flags & UNINTERESTING)
+				negative++;
+			else
+				positive++;
+		for (i = 0; i < revs.pending.nr; i++) {
+			struct object *obj = revs.pending.objects[i].item;
+
+			if (obj->type == OBJ_COMMIT)
+				clear_commit_marks((struct commit *)obj,
+						   ALL_REV_FLAGS);
+		}
+	}
+
+	free(copy);
+	object_array_clear(&revs.pending);
+	return negative > 0 && positive > 0;
 }

index c17dbc2e75a8..4abd70c40fed 100644
@@ -18,9 +18,7 @@ int show_range_diff(const char *range1, const char *range2,
 
 /*
  * Determine whether the given argument is usable as a range argument of `git
- * range-diff`, e.g. A..B. Note that this only validates the format but does
- * _not_ parse it, i.e. it does _not_ look up the specified commits in the
- * local repository.
+ * range-diff`, e.g. A..B.
  */
 int is_range_diff_range(const char *arg);
 

index 6eb344be0312..2b518378d4a0 100755
@@ -150,6 +150,19 @@ test_expect_success 'simple A B C (unmodified)' '
 	test_cmp expect actual
 '
 
+test_expect_success 'A^! and A^-<n> (unmodified)' '
+	git range-diff --no-color topic^! unmodified^-1 >actual &&
+	cat >expect <<-EOF &&
+	1:  $(test_oid t4) = 1:  $(test_oid u4) s/12/B/
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'A^{/..} is not mistaken for a range' '
+	test_must_fail git range-diff topic^.. topic^{/..} 2>error &&
+	test_i18ngrep "not a commit range" error
+'
+
 test_expect_success 'trivial reordering' '
 	git range-diff --no-color master topic reordered >actual &&
 	cat >expect <<-EOF &&

gitgitgadget

From: Johannes Schindelin <[email protected]>

There are three forms, depending whether the user specifies one, two or three non-option arguments. We've never actually explained how this works in the manual, so let's explain it.

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

index 9701c1e5fdd5..a968d5237dae 100644
@@ -28,6 +28,17 @@ Finally, the list of matching commits is shown in the order of the
 second commit range, with unmatched commits being inserted just after
 all of their ancestors have been shown.
 
+There are three ways to specify the commit ranges:
+
+- `<range1> <range2>`: Either commit range can be of the form
+  `<base>..<rev>`, `<rev>^!` or `<rev>^-<n>`. See `SPECIFYING RANGES`
+  in linkgit:gitrevisions[7] for more details.
+
+- `<rev1>...<rev2>`. This is equivalent to
+  `<rev2>..<rev1> <rev1>..<rev2>`.
+
+- `<base> <rev1> <rev2>`: This is equivalent to `<base>..<rev1>
+  <base>..<rev2>`.
 
 OPTIONS
 -------

gitgitgadget

In
https://lore.kernel.org/git/[email protected]/,
it was reported that git range-diff does not handle commit ranges like rev^!. This patch series fixes that.

Changes since v5:

* The commit marks are now cleared in a separate loop.
* The regression test no longer looks only for "rang" but for "range" in the error message.
* We now pass NULL as opt parameter to setup_revisions(), not 0.

Changes since v4:

* The commit marks are now cleared in is_range_diff_range().
* A regression test now verifies that HEAD^{/something..or other} isn't mistaken for a commit range.
* The manual page no longer mentions "symmetric range", to avoid contentious language.

Changes since v3:

* The revision machinery is now used directly to validate the commit ranges.

Changes since v2:

* Move the helper function from revision.c to range-diff.c and rename it.
* Use a regex to make it easier to understand what we're trying to match.
* Fix the documentation that claimed that we used git merge-base internally when git range-diff parses ...-style arguments, which is not the case.

Changes since v1:

* In addition to git range-diff, git format-patch --range-diff gets the same improvement.
* The comment talking about ^@ was removed.
* The parsing was made a bit safer (e.g. catching ! by its own as an invalid range).

Johannes Schindelin (3):
range-diff/format-patch: refactor check for commit range range-diff/format-patch: handle commit ranges other than A..B range-diff(docs): explain how to specify commit ranges

 Documentation/git-range-diff.txt | 11 +++++++++++
 builtin/log.c                    |  2 +-
 builtin/range-diff.c             |  9 +++++----
 range-diff.c                     | 29 +++++++++++++++++++++++++++++
 range-diff.h                     |  6 ++++++
 t/t3206-range-diff.sh            | 13 +++++++++++++
 6 files changed, 65 insertions(+), 5 deletions(-)

base-commit: 71ca53e8125e36efbda17293c50027d31681a41f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-841%2Fdscho%2Frange-diff-with-ranges-lacking-dotdot-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-841/dscho/range-diff-with-ranges-lacking-dotdot-v6
Pull-Request: https://github.com/gitgitgadget/git/pull/841

Range-diff vs v5:

 1:  b98fa94b8703 = 1:  b98fa94b8703 range-diff/format-patch: refactor check for commit range
 2:  04b5d75adbc3 ! 2:  f8e6a1ad9d3d range-diff/format-patch: handle commit ranges other than A..B
     @@ range-diff.c: int show_range_diff(const char *range1, const char *range2,
      +	struct rev_info revs;
      +
      +	init_revisions(&revs, NULL);
     -+	if (setup_revisions(3, argv, &revs, 0) == 1)
     -+		for (i = 0; i < revs.pending.nr; i++) {
     -+			struct object *obj = revs.pending.objects[i].item;
     -+
     -+			if (obj->flags & UNINTERESTING)
     ++	if (setup_revisions(3, argv, &revs, NULL) == 1) {
     ++		for (i = 0; i < revs.pending.nr; i++)
     ++			if (revs.pending.objects[i].item->flags & UNINTERESTING)
      +				negative++;
      +			else
      +				positive++;
     ++		for (i = 0; i < revs.pending.nr; i++) {
     ++			struct object *obj = revs.pending.objects[i].item;
     ++
      +			if (obj->type == OBJ_COMMIT)
      +				clear_commit_marks((struct commit *)obj,
      +						   ALL_REV_FLAGS);
      +		}
     ++	}
      +
      +	free(copy);
      +	object_array_clear(&revs.pending);
     @@ t/t3206-range-diff.sh: test_expect_success 'simple A B C (unmodified)' '
      +
      +test_expect_success 'A^{/..} is not mistaken for a range' '
      +	test_must_fail git range-diff topic^.. topic^{/..} 2>error &&
     -+	test_i18ngrep "not a commit rang" error
     ++	test_i18ngrep "not a commit range" error
      +'
      +
       test_expect_success 'trivial reordering' '

3: bc5de807735d = 3: 08c5f8732747 range-diff(docs): explain how to specify commit ranges

gitgitgadget

Hi Junio,

Your code is substantially harder to read. I really dislike that npmask idea.

I implemented that.

Not necessary: the code is short and narrow enough.

I had missed that in your review. Fixed, too.

Technically, it is not necessary, as we're only verifying that the intended error message is shown, and that "e" does not make any difference.

But it _was_ a typo, so I fixed it, too.

Next iteration is coming soon,
Dscho

From: Johannes Schindelin <[email protected]>

Currently, when called with exactly two arguments, git range-diff tests for a literal .. in each of the two. Likewise, the argument provided via --range-diff to git format-patch is checked in the same manner.

However, <commit>^! is a perfectly valid commit range, equivalent to <commit>^..<commit> according to the SPECIFYING RANGES section of gitrevisions[7].

In preparation for allowing more sophisticated ways to specify commit ranges, let's refactor the check into its own function.

---
 builtin/log.c        | 2 +-
 builtin/range-diff.c | 9 +++++----
 range-diff.c         | 5 +++++
 range-diff.h         | 8 ++++++++
 4 files changed, 19 insertions(+), 5 deletions(-)

index bd6ff4f9f956..aeece57e86a2 100644
@@ -1680,7 +1680,7 @@ static void infer_range_diff_ranges(struct strbuf *r1,
 				    struct commit *head)
 {
 	const char *head_oid = oid_to_hex(&head->object.oid);
-	int prev_is_range = !!strstr(prev, "..");
+	int prev_is_range = is_range_diff_range(prev);
 
 	if (prev_is_range)
 		strbuf_addstr(r1, prev);

index 24c4162f7446..5b1f6326322f 100644
@@ -3,6 +3,7 @@
 #include "parse-options.h"
 #include "range-diff.h"
 #include "config.h"
+#include "revision.h"
 
 static const char * const builtin_range_diff_usage[] = {
 N_("git range-diff [<options>] <old-base>..<old-tip> <new-base>..<new-tip>"),

@@ -46,12 +47,12 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 		diffopt.use_color = 1;
 
 	if (argc == 2) {
-		if (!strstr(argv[0], ".."))
-			die(_("no .. in range: '%s'"), argv[0]);
+		if (!is_range_diff_range(argv[0]))
+			die(_("not a commit range: '%s'"), argv[0]);
 		strbuf_addstr(&range1, argv[0]);
 
-		if (!strstr(argv[1], ".."))
-			die(_("no .. in range: '%s'"), argv[1]);
+		if (!is_range_diff_range(argv[1]))
+			die(_("not a commit range: '%s'"), argv[1]);
 		strbuf_addstr(&range2, argv[1]);
 	} else if (argc == 3) {
 		strbuf_addf(&range1, "%s..%s", argv[0], argv[1]);

index b9950f10c8c4..9b93e08e8407 100644
@@ -564,3 +564,8 @@ int show_range_diff(const char *range1, const char *range2,
 
 	return res;
 }
+
+int is_range_diff_range(const char *arg)
+{
+	return !!strstr(arg, "..");
+}

index 583ced2e8e74..c17dbc2e75a8 100644
@@ -16,4 +16,12 @@ int show_range_diff(const char *range1, const char *range2,
 		    const struct diff_options *diffopt,
 		    const struct strvec *other_arg);
 
+/*
+ * Determine whether the given argument is usable as a range argument of `git
+ * range-diff`, e.g. A..B. Note that this only validates the format but does
+ * _not_ parse it, i.e. it does _not_ look up the specified commits in the
+ * local repository.
+ */
+int is_range_diff_range(const char *arg);
+
 #endif

gitgitgadget

Hi Junio,

I am concerned when I see what add_rev_cmdline() does, for example.

Ciao,
Dscho