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

Commit Message

Johannes Schindelin Jan. 21, 2021, 10:20 p.m. UTC

From: Johannes Schindelin <[e-mail]>

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 <[e-mail]>
---
 builtin/range-diff.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Comments

Junio C Hamano Jan. 21, 2021, 11:27 p.m. UTC | #1

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

> From: Johannes Schindelin <[e-mail]>
>
> 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.

Phillip Wood Jan. 22, 2021, 7:12 p.m. UTC | #2

Hi Dscho

On 21/01/2021 22:20, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>   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]);
>

Junio C Hamano Jan. 22, 2021, 9:59 p.m. UTC | #3

Phillip Wood <[e-mail]> 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.

Phillip Wood Jan. 23, 2021, 3:59 p.m. UTC | #4

Hi Junio

On 22/01/2021 21:59, Junio C Hamano wrote:
> Phillip Wood <[e-mail]> 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.
>

Johannes Schindelin Jan. 26, 2021, 3:19 p.m. UTC | #5

Hi Phillip,

On Sat, 23 Jan 2021, Phillip Wood wrote:

> On 22/01/2021 21:59, Junio C Hamano wrote:
> > Phillip Wood <[e-mail]> 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

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