diff mbox series

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

Message ID b98fa94b87037b811ea973c1aeb7cfe08d7c1bd6.1612431093.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Range diff with ranges lacking dotdot | expand

Commit Message

Johannes Schindelin Feb. 4, 2021, 9:31 a.m. UTC 
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(-)

Comments

Junio C Hamano Feb. 4, 2021, 6:56 p.m. UTC | #1 
"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.
Johannes Schindelin Feb. 4, 2021, 7:27 p.m. UTC | #2 
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
diff mbox series

Patch

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