[PATCH 0/3] Range diff with ranges lacking dotdot

[PATCH 0/3] Range diff with ranges lacking dotdot

[Johannes Schindelin via GitGitGadget]

In
https://lore.kernel.org/git/20200306091933.mx2jmurmdnsjua4b@pengutronix.de/,
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

[PATCH 1/3] range-diff: refactor check for commit range

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/range-diff.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index 24c4162f744..551d3e689cb 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -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

Re: [PATCH 1/3] range-diff: refactor check for commit range

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.

Makes sense.

Re: [PATCH 1/3] range-diff: refactor check for commit range

[Phillip Wood]

Hi Dscho

On 21/01/2021 22:20, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 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.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   builtin/range-diff.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/range-diff.c b/builtin/range-diff.c
> index 24c4162f744..551d3e689cb 100644
> --- a/builtin/range-diff.c
> +++ b/builtin/range-diff.c
> @@ -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, "..");
> +}

If the user wrongly passes two arguments referring to single commits 
with `:/<text>` or `@{/<text>}` where text contains ".." this will give 
a false positive.

Best Wishes

Phillip

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

Re: [PATCH 1/3] range-diff: refactor check for commit range

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

>>   +static int is_range(const char *range)
>> +{
>> +	return !!strstr(range, "..");
>> +}
>
> If the user wrongly passes two arguments referring to single commits
> with `:/<text>` or `@{/<text>}` where text contains ".." this will
> give a false positive.

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.

Re: [PATCH 1/3] range-diff: refactor check for commit range

[Phillip Wood]

Hi Junio

On 22/01/2021 21:59, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>>>    +static int is_range(const char *range)
>>> +{
>>> +	return !!strstr(range, "..");
>>> +}
>>
>> If the user wrongly passes two arguments referring to single commits
>> with `:/<text>` or `@{/<text>}` where text contains ".." this will
>> give a false positive.
> 
> True.  I do not think this aims to be complete revision parser in
> the first place, though.

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


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

Re: [PATCH 1/3] range-diff: refactor check for commit range

[Johannes Schindelin]

Hi Phillip,

On Sat, 23 Jan 2021, Phillip Wood wrote:

> On 22/01/2021 21:59, Junio C Hamano wrote:
> > Phillip Wood <phillip.wood123@gmail.com> writes:
> >
> > > >    +static int is_range(const char *range)
> > > > +{
> > > > +	return !!strstr(range, "..");
> > > > +}
> > >
> > > If the user wrongly passes two arguments referring to single commits
> > > with `:/<text>` or `@{/<text>}` where text contains ".." this will
> > > give a false positive.
> >
> > True.  I do not think this aims to be complete revision parser in
> > the first place, though.
>
> Yes but it affects the error message given to the user.

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

[PATCH 2/3] range-diff: handle commit ranges other than A..B

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/range-diff.c  | 21 ++++++++++++++++++++-
 t/t3206-range-diff.sh |  8 ++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index 551d3e689cb..6097635c432 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -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)
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 6eb344be031..e217cecac9e 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -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

Re: [PATCH 2/3] range-diff: handle commit ranges other than A..B

[Eric Sunshine]

On Thu, Jan 21, 2021 at 5:22 PM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/builtin/range-diff.c b/builtin/range-diff.c
> @@ -13,7 +13,26 @@ NULL
>  static int is_range(const char *range)
>  {
> +       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] == '^';
>  }

Is this something that the --range-diff option of git-format-patch
will want to do, as well? At present,
builtin/log.c:infer_range_diff_ranges() detects a range only by
checking for "..", much like this function did before this patch. If
so, perhaps this function can be part of the public range-diff API
(or, indeed, part of some other more general API if it's not really
specific to range-diff).

Re: [PATCH 2/3] range-diff: handle commit ranges other than A..B

[Johannes Schindelin]

Hi Eric,

On Thu, 21 Jan 2021, Eric Sunshine wrote:

> On Thu, Jan 21, 2021 at 5:22 PM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > 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.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/builtin/range-diff.c b/builtin/range-diff.c
> > @@ -13,7 +13,26 @@ NULL
> >  static int is_range(const char *range)
> >  {
> > +       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] == '^';
> >  }
>
> Is this something that the --range-diff option of git-format-patch
> will want to do, as well?

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

> At present, builtin/log.c:infer_range_diff_ranges() detects a range only
> by checking for "..", much like this function did before this patch. If
> so, perhaps this function can be part of the public range-diff API (or,
> indeed, part of some other more general API if it's not really specific
> to range-diff).

Re: [PATCH 2/3] range-diff: handle commit ranges other than A..B

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/range-diff.c  | 21 ++++++++++++++++++++-
>  t/t3206-range-diff.sh |  8 ++++++++
>  2 files changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/range-diff.c b/builtin/range-diff.c
> index 551d3e689cb..6097635c432 100644
> --- a/builtin/range-diff.c
> +++ b/builtin/range-diff.c
> @@ -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 ...^@ */

I am confused.  If it ends with '!', I do not see how it can end
with "^@".

If the input were "!", i gets strlen("!") which is 1, c gets '!'
while predecrementing i down to 0, and we notice c is '!' and
decrement i again to make it (size_t)(-1) which is a fairly large
number.

Then we skip all the else/if cascade, ensure that i is positive, and
happily access range[i], which likely is way out of bounds (but it
probably is almost one turn around the earth out of bounds, it may
access just a single byte before the array).

Am I reading the code right?

IOW, "git range-diff \! A..B" would do something strange, I would
guess.

> +	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)
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index 6eb344be031..e217cecac9e 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -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 &&

Re: [PATCH 2/3] range-diff: handle commit ranges other than A..B

[Johannes Schindelin]

Hi Junio,

On Thu, 21 Jan 2021, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  builtin/range-diff.c  | 21 ++++++++++++++++++++-
> >  t/t3206-range-diff.sh |  8 ++++++++
> >  2 files changed, 28 insertions(+), 1 deletion(-)
> >
> > diff --git a/builtin/range-diff.c b/builtin/range-diff.c
> > index 551d3e689cb..6097635c432 100644
> > --- a/builtin/range-diff.c
> > +++ b/builtin/range-diff.c
> > @@ -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 ...^@ */
>
> I am confused.  If it ends with '!', I do not see how it can end
> with "^@".

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.

> If the input were "!", i gets strlen("!") which is 1, c gets '!'
> while predecrementing i down to 0, and we notice c is '!' and
> decrement i again to make it (size_t)(-1) which is a fairly large
> number.

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

> Then we skip all the else/if cascade, ensure that i is positive, and
> happily access range[i], which likely is way out of bounds (but it
> probably is almost one turn around the earth out of bounds, it may
> access just a single byte before the array).
>
> Am I reading the code right?
>
> IOW, "git range-diff \! A..B" would do something strange, I would
> guess.

Right. Will be fixed in the next iteration.

Thanks,
Dscho

[PATCH 3/3] range-diff(docs): explain how to specify commit ranges

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-range-diff.txt | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
index 9701c1e5fdd..76359baf26d 100644
--- a/Documentation/git-range-diff.txt
+++ b/Documentation/git-range-diff.txt
@@ -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

Re: [PATCH 3/3] range-diff(docs): explain how to specify commit ranges

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  Documentation/git-range-diff.txt | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
> index 9701c1e5fdd..76359baf26d 100644
> --- a/Documentation/git-range-diff.txt
> +++ b/Documentation/git-range-diff.txt
> @@ -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.

Good.

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

Does this merely resemble?  Isn't it exactly what a symmetric range is?

> +- `<base> <rev1> <rev2>`: This is equivalent to `<base>..<rev1>
> +  <base>..<rev2>`.

Nice to see this documented.

Thanks.

Re: [PATCH 3/3] range-diff(docs): explain how to specify commit ranges

[Johannes Schindelin]

Hi Junio,

On Thu, 21 Jan 2021, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  Documentation/git-range-diff.txt | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
> > index 9701c1e5fdd..76359baf26d 100644
> > --- a/Documentation/git-range-diff.txt
> > +++ b/Documentation/git-range-diff.txt
> > @@ -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.
>
> Good.
>
> > +- `<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>`.
>
> Does this merely resemble?  Isn't it exactly what a symmetric range is?

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.

> > +- `<base> <rev1> <rev2>`: This is equivalent to `<base>..<rev1>
> > +  <base>..<rev2>`.
>
> Nice to see this documented.

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

Ciao,
Dscho

Re: [PATCH 3/3] range-diff(docs): explain how to specify commit ranges

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

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.

>> Does this merely resemble?  Isn't it exactly what a symmetric range is?
>
> 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.

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?

Re: [PATCH 3/3] range-diff(docs): explain how to specify commit ranges

[Johannes Schindelin]

Hi Junio,

On Fri, 22 Jan 2021, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> > +- `<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>`.
>
> 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.

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);
	[...]

> >> Does this merely resemble?  Isn't it exactly what a symmetric range is?
> >
> > 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.
>
> This however is an end-user documentation, isn't it?

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

Re: [PATCH 3/3] range-diff(docs): explain how to specify commit ranges

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> 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.
>
> You know what? I lied.

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

>> >> Does this merely resemble?  Isn't it exactly what a symmetric range is?
>> >
>> > 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.
>>
>> This however is an end-user documentation, isn't it?
>
> 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`.

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

Re: [PATCH 3/3] range-diff(docs): explain how to specify commit ranges

[Uwe Kleine-König]

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

On Thu, Jan 21, 2021 at 10:20:38PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 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.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  Documentation/git-range-diff.txt | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
> index 9701c1e5fdd..76359baf26d 100644
> --- a/Documentation/git-range-diff.txt
> +++ b/Documentation/git-range-diff.txt
> @@ -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>`.

git-log takes a range, too. There you can specify a single rev (with the
semantic to list all commits from this rev up (or down?) to the root).
So <rev> means implicitly <rev>^∞..<rev> for git-log.

Does it make sense to implement this here, too? Maybe this even allows
sharing some more code?

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] range-diff(docs): explain how to specify commit ranges

[Johannes Schindelin]

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

Hi Uwe,

On Fri, 22 Jan 2021, Uwe Kleine-König wrote:

> On Thu, Jan 21, 2021 at 10:20:38PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  Documentation/git-range-diff.txt | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
> > index 9701c1e5fdd..76359baf26d 100644
> > --- a/Documentation/git-range-diff.txt
> > +++ b/Documentation/git-range-diff.txt
> > @@ -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>`.
>
> git-log takes a range, too. There you can specify a single rev (with the
> semantic to list all commits from this rev up (or down?) to the root).
> So <rev> means implicitly <rev>^∞..<rev> for git-log.
>
> Does it make sense to implement this here, too? Maybe this even allows
> sharing some more code?

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

Re: [PATCH 0/3] Range diff with ranges lacking dotdot

[Uwe Kleine-König]

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

Hello Johannes,

On Thu, Jan 21, 2021 at 10:20:35PM +0000, Johannes Schindelin via GitGitGadget wrote:
> In
> https://lore.kernel.org/git/20200306091933.mx2jmurmdnsjua4b@pengutronix.de/,
> it was reported that git range-diff does not handle commit ranges like
> rev^!. This patch series fixes that.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 0/3] Range diff with ranges lacking dotdot

[Johannes Schindelin via GitGitGadget]

In
https://lore.kernel.org/git/20200306091933.mx2jmurmdnsjua4b@pengutronix.de/,
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 <Johannes.Schindelin@gmx.de>
      
       ## 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.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
     + ## 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 <Johannes.Schindelin@gmx.de>
      
       ## 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 <johannes.schindelin@gmx.de>
      
     - ## 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

[PATCH v2 1/3] range-diff/format-patch: refactor check for commit range

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/log.c        | 2 +-
 builtin/range-diff.c | 9 +++++----
 revision.c           | 5 +++++
 revision.h           | 7 +++++++
 4 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index bd6ff4f9f95..099abdfb7e6 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -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);
diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index 24c4162f744..89d54158011 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -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]);
diff --git a/revision.c b/revision.c
index 9dff845bed6..00675f598a3 100644
--- a/revision.c
+++ b/revision.c
@@ -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, "..");
+}
diff --git a/revision.h b/revision.h
index 086ff10280d..66777c8e60f 100644
--- a/revision.h
+++ b/revision.h
@@ -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

Re: [PATCH v2 1/3] range-diff/format-patch: refactor check for commit range

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.

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.

> diff --git a/revision.h b/revision.h

Move this to range-diff.h, not revision.h which is about true commit
ranges.

> index 086ff10280d..66777c8e60f 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -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.
> + */

And s/defines a commit range/is usable as a range to range-diff/

Thanks.

> +int specifies_commit_range(const char *range);
> +
>  #endif

Re: [PATCH v2 1/3] range-diff/format-patch: refactor check for commit range

[Uwe Kleine-König]

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

On Fri, Jan 22, 2021 at 12:27:10PM -0800, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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.
> 
> 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).

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/3] range-diff/format-patch: refactor check for commit range

[Junio C Hamano]

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:

>> > In preparation for allowing more sophisticated ways to specify commit
>> > ranges, let's refactor the check into its own function.
>> 
>> 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).
>
> 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.

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

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

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

Re: [PATCH v2 1/3] range-diff/format-patch: refactor check for commit range

[Uwe Kleine-König]

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

Hello Junio,

On Mon, Jan 25, 2021 at 11:24:39AM -0800, Junio C Hamano wrote:
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:
> 
> >> > In preparation for allowing more sophisticated ways to specify commit
> >> > ranges, let's refactor the check into its own function.
> >> 
> >> 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).
> >
> > 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.
> 
> 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 ...
> 
> > 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.
> 
> ... I am OK with that point of view.  It certainly is simpler to
> explain to end users.

It seems you understood my argument :-)

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

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

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/3] range-diff/format-patch: refactor check for commit range

[Junio C Hamano]

Uwe Kleine-König <u.kleine-koenig@pengutronix.de> writes:

>> > 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.
>> 
>> ... I am OK with that point of view.  It certainly is simpler to
>> explain to end users.
>
> It seems you understood my argument :-)

So it seems ;-).

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

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.

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

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

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

[PATCH v2 2/3] range-diff/format-patch: handle commit ranges other than A..B

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 revision.c            | 22 +++++++++++++++++++++-
 t/t3206-range-diff.sh |  8 ++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index 00675f598a3..9ee063a2c03 100644
--- a/revision.c
+++ b/revision.c
@@ -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] == '^';
 }
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 6eb344be031..e217cecac9e 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -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

Re: [PATCH v2 2/3] range-diff/format-patch: handle commit ranges other than A..B

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> +	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] == '^';

This is still way too complex for my liking, but at least I cannot
immediately spot a glaring off-by-one like the previous round ;-)

This is a tangent ([*1*]), but we often equate an omission to
implicitly specified HEAD; e.g. "git log @{u}.." is what we did
since we started building on top of our upstream.  I wonder if it
makes sense to allow similar short-hand so that ^! alone would mean
HEAD^!, ^@ alone would mean HEAD^@, and so on.

Thanks.


[Footnote]

*1* read: this has nothing to do with how ready I think this patch
    is, but the topic reminds me of it strongly enough that I raise
    it here, because I know the opinions on this unrelated thing on
    recipients of this response are worth listening to.

Re: [PATCH v2 2/3] range-diff/format-patch: handle commit ranges other than A..B

[Johannes Schindelin]

Hi Junio,

On Fri, 22 Jan 2021, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > +	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] == '^';
>
> This is still way too complex for my liking, but at least I cannot
> immediately spot a glaring off-by-one like the previous round ;-)

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?

> This is a tangent ([*1*]), but we often equate an omission to
> implicitly specified HEAD; e.g. "git log @{u}.." is what we did
> since we started building on top of our upstream.  I wonder if it
> makes sense to allow similar short-hand so that ^! alone would mean
> HEAD^!, ^@ alone would mean HEAD^@, and so on.

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

Re: [PATCH v2 2/3] range-diff/format-patch: handle commit ranges other than A..B

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

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.

Re: [PATCH v2 2/3] range-diff/format-patch: handle commit ranges other than A..B

[Johannes Schindelin]

Hi Junio,

On Wed, 27 Jan 2021, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > 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?
>
> 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?

I hadn't thought of that.

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

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

Re: [PATCH v2 2/3] range-diff/format-patch: handle commit ranges other than A..B

[Johannes Schindelin]

Hi Junio,

On Thu, 28 Jan 2021, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > 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.
>
> Hmph, do you mean the singleton instances of "struct object" and
> those specific types that contain it?

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

Ciao,
Dscho

[PATCH v2 3/3] range-diff(docs): explain how to specify commit ranges

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-range-diff.txt | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
index 9701c1e5fdd..76359baf26d 100644
--- a/Documentation/git-range-diff.txt
+++ b/Documentation/git-range-diff.txt
@@ -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

[PATCH v3 0/3] Range diff with ranges lacking dotdot

[Johannes Schindelin via GitGitGadget]

In
https://lore.kernel.org/git/20200306091933.mx2jmurmdnsjua4b@pengutronix.de/,
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 <johannes.schindelin@gmx.de>
      
     - ## 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

[PATCH v3 1/3] range-diff/format-patch: refactor check for commit range

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/log.c        | 2 +-
 builtin/range-diff.c | 9 +++++----
 range-diff.c         | 5 +++++
 range-diff.h         | 8 ++++++++
 4 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index bd6ff4f9f95..aeece57e86a 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -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);
diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index 24c4162f744..5b1f6326322 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -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]);
diff --git a/range-diff.c b/range-diff.c
index b9950f10c8c..9b93e08e840 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -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, "..");
+}
diff --git a/range-diff.h b/range-diff.h
index 583ced2e8e7..c17dbc2e75a 100644
--- a/range-diff.h
+++ b/range-diff.h
@@ -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

[PATCH v3 2/3] range-diff/format-patch: handle commit ranges other than A..B

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 range-diff.c          | 14 +++++++++++++-
 t/t3206-range-diff.sh |  8 ++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/range-diff.c b/range-diff.c
index 9b93e08e840..0c6ac4f954d 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -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);
 }
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 6eb344be031..e217cecac9e 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -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

[PATCH v3 3/3] range-diff(docs): explain how to specify commit ranges

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-range-diff.txt | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
index 9701c1e5fdd..14bffb272a0 100644
--- a/Documentation/git-range-diff.txt
+++ b/Documentation/git-range-diff.txt
@@ -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

[PATCH v4 0/3] Range diff with ranges lacking dotdot

[Johannes Schindelin via GitGitGadget]

In
https://lore.kernel.org/git/20200306091933.mx2jmurmdnsjua4b@pengutronix.de/,
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 <johannes.schindelin@gmx.de>
      
       ## 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

[PATCH v4 1/3] range-diff/format-patch: refactor check for commit range

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/log.c        | 2 +-
 builtin/range-diff.c | 9 +++++----
 range-diff.c         | 5 +++++
 range-diff.h         | 8 ++++++++
 4 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index bd6ff4f9f956..aeece57e86a2 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -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);
diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index 24c4162f7446..5b1f6326322f 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -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]);
diff --git a/range-diff.c b/range-diff.c
index b9950f10c8c4..9b93e08e8407 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -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, "..");
+}
diff --git a/range-diff.h b/range-diff.h
index 583ced2e8e74..c17dbc2e75a8 100644
--- a/range-diff.h
+++ b/range-diff.h
@@ -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

Re: [PATCH v4 1/3] range-diff/format-patch: refactor check for commit range

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> diff --git a/range-diff.h b/range-diff.h
> index 583ced2e8e74..c17dbc2e75a8 100644
> --- a/range-diff.h
> +++ b/range-diff.h
> @@ -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);

If we were to use [v4 2/3], then we do parse it, even though we do
use the parse result to reject some valid ranges (like "a history
all the way down to root" in the implementation).  I think just
dropping everything after "Note that" is a sufficient fix.

Re: [PATCH v4 1/3] range-diff/format-patch: refactor check for commit range

[Johannes Schindelin]

Hi Junio,

On Thu, 4 Feb 2021, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > diff --git a/range-diff.h b/range-diff.h
> > index 583ced2e8e74..c17dbc2e75a8 100644
> > --- a/range-diff.h
> > +++ b/range-diff.h
> > @@ -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);
>
> If we were to use [v4 2/3], then we do parse it, even though we do
> use the parse result to reject some valid ranges (like "a history
> all the way down to root" in the implementation).  I think just
> dropping everything after "Note that" is a sufficient fix.

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

[PATCH v4 2/3] range-diff/format-patch: handle commit ranges other than A..B

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 range-diff.c          | 19 ++++++++++++++++++-
 t/t3206-range-diff.sh |  8 ++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/range-diff.c b/range-diff.c
index 9b93e08e8407..07e212d5bb8c 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -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;
 }
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 6eb344be0312..e217cecac9ed 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -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

Re: [PATCH v4 2/3] range-diff/format-patch: handle commit ranges other than A..B

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

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

One thing that worries me with this code is that I do not see
anybody that clears UNINTERESTING bit in the flags.  In-core objects
are singletons, so if a user fed the command two ranges,

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

and this code first handled "A..B", smudging the in-core instance of
the commit object A with UNINTERESTING bit, that in-core instance
will be reused when the second range argument "C..A" is given to
this function again.

At that point, has anybody cleared the UNINTERESTING bit in the
flags word for the in-core commit A?  I do not see it done in this
function, but perhaps I am missing it done in the init/setup
functions (I somehow doubt it, though)?

Shoudn't we be calling clear_commit_marks(ALL_REF_FLAGS) on the
commits in revs.pending[] array before we clear it?  Depending on
the shape of "arg" that is end-user supplied, we may have walked the
history in handle_dotdot_1() to parse it (e.g. "A...B").

Also we'd want to see what needs to be cleared in revs.cmdline
that would have been populated by calls to add_rev_cmdline().

Other than that, I quite like the way the actual code turned out to
be.

> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index 6eb344be0312..e217cecac9ed 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -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 &&

Re: [PATCH v4 2/3] range-diff/format-patch: handle commit ranges other than A..B

[Johannes Schindelin]

Hi Junio,

On Thu, 4 Feb 2021, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> >  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;
> >  }
>
> One thing that worries me with this code is that I do not see
> anybody that clears UNINTERESTING bit in the flags.  In-core objects
> are singletons, so if a user fed the command two ranges,
>
> 	git range-diff A..B C..A
>
> and this code first handled "A..B", smudging the in-core instance of
> the commit object A with UNINTERESTING bit, that in-core instance
> will be reused when the second range argument "C..A" is given to
> this function again.
>
> At that point, has anybody cleared the UNINTERESTING bit in the
> flags word for the in-core commit A?  I do not see it done in this
> function, but perhaps I am missing it done in the init/setup
> functions (I somehow doubt it, though)?
>
> Shoudn't we be calling clear_commit_marks(ALL_REF_FLAGS) on the
> commits in revs.pending[] array before we clear it?

Yes, we should ;-)

> Depending on the shape of "arg" that is end-user supplied, we may have
> walked the history in handle_dotdot_1() to parse it (e.g. "A...B").

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

> Also we'd want to see what needs to be cleared in revs.cmdline
> that would have been populated by calls to add_rev_cmdline().

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

> Other than that, I quite like the way the actual code turned out to
> be.
>
> > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> > index 6eb344be0312..e217cecac9ed 100755
> > --- a/t/t3206-range-diff.sh
> > +++ b/t/t3206-range-diff.sh
> > @@ -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 &&
>

Re: [PATCH v4 2/3] range-diff/format-patch: handle commit ranges other than A..B

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

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

Now we actually parse the single-token range, instead of relying on
"does it have dot-dot" heuristics, we can make sure that we reject

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

as "not a range with negative and positive ends".

Re: [PATCH v4 2/3] range-diff/format-patch: handle commit ranges other than A..B

[Johannes Schindelin]

Hi Junio,

On Thu, 4 Feb 2021, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > +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
> > +'
>
> Now we actually parse the single-token range, instead of relying on
> "does it have dot-dot" heuristics, we can make sure that we reject
>
>     "HEAD^{/^string with .. in it}"
>
> as "not a range with negative and positive ends".

Sure, why not.

Ciao,
Dscho

[PATCH v4 3/3] range-diff(docs): explain how to specify commit ranges

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-range-diff.txt | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
index 9701c1e5fdd5..14bffb272a06 100644
--- a/Documentation/git-range-diff.txt
+++ b/Documentation/git-range-diff.txt
@@ -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

Re: [PATCH v4 3/3] range-diff(docs): explain how to specify commit ranges

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  Documentation/git-range-diff.txt | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
> index 9701c1e5fdd5..14bffb272a06 100644
> --- a/Documentation/git-range-diff.txt
> +++ b/Documentation/git-range-diff.txt
> @@ -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>`.

As I said before, this _is_ a symmetric range that has commits
reachable from rev1 but not from rev2 on the left hand side, and
commits reachable from rev2 but not from rev1 on the right hand
side, not just something else that resembles a symmetric range.

> +- `<base> <rev1> <rev2>`: This is equivalent to `<base>..<rev1>
> +  <base>..<rev2>`.
>  
>  OPTIONS
>  -------

Re: [PATCH v4 3/3] range-diff(docs): explain how to specify commit ranges

[Johannes Schindelin]

Hi Junio,

On Thu, 4 Feb 2021, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  Documentation/git-range-diff.txt | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
> > index 9701c1e5fdd5..14bffb272a06 100644
> > --- a/Documentation/git-range-diff.txt
> > +++ b/Documentation/git-range-diff.txt
> > @@ -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>`.
>
> As I said before, this _is_ a symmetric range that has commits
> reachable from rev1 but not from rev2 on the left hand side, and
> commits reachable from rev2 but not from rev1 on the right hand
> side, not just something else that resembles a symmetric range.

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

> > +- `<base> <rev1> <rev2>`: This is equivalent to `<base>..<rev1>
> > +  <base>..<rev2>`.
> >
> >  OPTIONS
> >  -------
>

Re: [PATCH v4 3/3] range-diff(docs): explain how to specify commit ranges

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> As I said before, this _is_ a symmetric range that has commits
>> reachable from rev1 but not from rev2 on the left hand side, and
>> commits reachable from rev2 but not from rev1 on the right hand
>> side, not just something else that resembles a symmetric range.
>
> If we were talking about one commit range instead of two, I would agree
> with you.

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.

[PATCH v5 0/3] Range diff with ranges lacking dotdot

[Johannes Schindelin via GitGitGadget]

In
https://lore.kernel.org/git/20200306091933.mx2jmurmdnsjua4b@pengutronix.de/,
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 <johannes.schindelin@gmx.de>
      
       ## 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

[PATCH v5 1/3] range-diff/format-patch: refactor check for commit range

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/log.c        | 2 +-
 builtin/range-diff.c | 9 +++++----
 range-diff.c         | 5 +++++
 range-diff.h         | 8 ++++++++
 4 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index bd6ff4f9f956..aeece57e86a2 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -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);
diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index 24c4162f7446..5b1f6326322f 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -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]);
diff --git a/range-diff.c b/range-diff.c
index b9950f10c8c4..9b93e08e8407 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -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, "..");
+}
diff --git a/range-diff.h b/range-diff.h
index 583ced2e8e74..c17dbc2e75a8 100644
--- a/range-diff.h
+++ b/range-diff.h
@@ -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

[PATCH v5 2/3] range-diff/format-patch: handle commit ranges other than A..B

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 range-diff.c          | 24 +++++++++++++++++++++++-
 range-diff.h          |  4 +---
 t/t3206-range-diff.sh | 13 +++++++++++++
 3 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/range-diff.c b/range-diff.c
index 9b93e08e8407..c307bca9de23 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -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;
 }
diff --git a/range-diff.h b/range-diff.h
index c17dbc2e75a8..4abd70c40fed 100644
--- a/range-diff.h
+++ b/range-diff.h
@@ -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);
 
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 6eb344be0312..45f21ee215d7 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -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

Re: [PATCH v5 2/3] range-diff/format-patch: handle commit ranges other than A..B

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

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

Ah, I didn't think of adding this inside the same loop.  As long as
the pending objects are independent, this should work, but it is
unsafe, I think.  Imagine what happens if revs.pending[0].item can
reach the object revs.pending[1].item?  By the time the loop wants
to inspect the latter, its flags may have been cleared already
because we clear flags in objects that are reachable from the
former.

Let's do this as a post-processing for safety (or correctness).

	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:
> +	free(copy);
> +	object_array_clear(&revs.pending);
> +	return negative > 0 && positive > 0;
>  }

or something like that.

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

s/rang/range/, I would think.

Thanks.

Re: [PATCH v5 2/3] range-diff/format-patch: handle commit ranges other than A..B

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Ah, I didn't think of adding this inside the same loop.  As long as
> the pending objects are independent, this should work, but it is
> unsafe, I think.

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

diff --git a/range-diff.c b/range-diff.c
index c307bca9de..7a38dc8715 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -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;
 }
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 45f21ee215..2b518378d4 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -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

Re: [PATCH v5 2/3] range-diff/format-patch: handle commit ranges other than A..B

[Johannes Schindelin]

Hi Junio,

On Thu, 4 Feb 2021, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
> > Ah, I didn't think of adding this inside the same loop.  As long as
> > the pending objects are independent, this should work, but it is
> > unsafe, I think.
>
> 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.

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

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

I implemented that.

>  * Dedent by judicious use of 'goto'.

Not necessary: the code is short and narrow enough.

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

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

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

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

>
> ----- >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(-)
>
> diff --git a/range-diff.c b/range-diff.c
> index c307bca9de..7a38dc8715 100644
> --- a/range-diff.c
> +++ b/range-diff.c
> @@ -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;
>  }
> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
> index 45f21ee215..2b518378d4 100755
> --- a/t/t3206-range-diff.sh
> +++ b/t/t3206-range-diff.sh
> @@ -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
>
>

[PATCH v5 3/3] range-diff(docs): explain how to specify commit ranges

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-range-diff.txt | 11 +++++++++++
 1 file changed, 11 insertions(+)

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

[PATCH v6 0/3] Range diff with ranges lacking dotdot

[Johannes Schindelin via GitGitGadget]

In
https://lore.kernel.org/git/20200306091933.mx2jmurmdnsjua4b@pengutronix.de/,
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

[PATCH v6 1/3] range-diff/format-patch: refactor check for commit range

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/log.c        | 2 +-
 builtin/range-diff.c | 9 +++++----
 range-diff.c         | 5 +++++
 range-diff.h         | 8 ++++++++
 4 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index bd6ff4f9f956..aeece57e86a2 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -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);
diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index 24c4162f7446..5b1f6326322f 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -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]);
diff --git a/range-diff.c b/range-diff.c
index b9950f10c8c4..9b93e08e8407 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -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, "..");
+}
diff --git a/range-diff.h b/range-diff.h
index 583ced2e8e74..c17dbc2e75a8 100644
--- a/range-diff.h
+++ b/range-diff.h
@@ -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

[PATCH v6 2/3] range-diff/format-patch: handle commit ranges other than A..B

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 range-diff.c          | 26 +++++++++++++++++++++++++-
 range-diff.h          |  4 +---
 t/t3206-range-diff.sh | 13 +++++++++++++
 3 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/range-diff.c b/range-diff.c
index 9b93e08e8407..a88612cb8923 100644
--- a/range-diff.c
+++ b/range-diff.c
@@ -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;
 }
diff --git a/range-diff.h b/range-diff.h
index c17dbc2e75a8..4abd70c40fed 100644
--- a/range-diff.h
+++ b/range-diff.h
@@ -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);
 
diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index 6eb344be0312..2b518378d4a0 100755
--- a/t/t3206-range-diff.sh
+++ b/t/t3206-range-diff.sh
@@ -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

[PATCH v6 3/3] range-diff(docs): explain how to specify commit ranges

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Documentation/git-range-diff.txt | 11 +++++++++++
 1 file changed, 11 insertions(+)

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