* [PATCH v2 1/3] range-diff/format-patch: refactor check for commit range
2021-01-22 18:16 ` [PATCH v2 " Johannes Schindelin via GitGitGadget
@ 2021-01-22 18:16 ` Johannes Schindelin via GitGitGadget
2021-01-22 20:27 ` Junio C Hamano
2021-01-22 18:16 ` [PATCH v2 2/3] range-diff/format-patch: handle commit ranges other than A..B Johannes Schindelin via GitGitGadget
` (2 subsequent siblings)
3 siblings, 1 reply; 63+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-01-22 18:16 UTC (permalink / raw)
To: git
Cc: Uwe Kleine-König, Eric Sunshine, Johannes Schindelin,
Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* Re: [PATCH v2 1/3] range-diff/format-patch: refactor check for commit range
2021-01-22 18:16 ` [PATCH v2 1/3] range-diff/format-patch: refactor check for commit range Johannes Schindelin via GitGitGadget
@ 2021-01-22 20:27 ` Junio C Hamano
2021-01-25 7:35 ` Uwe Kleine-König
0 siblings, 1 reply; 63+ messages in thread
From: Junio C Hamano @ 2021-01-22 20:27 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Uwe Kleine-König, Eric Sunshine, Johannes Schindelin
"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
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH v2 1/3] range-diff/format-patch: refactor check for commit range
2021-01-22 20:27 ` Junio C Hamano
@ 2021-01-25 7:35 ` Uwe Kleine-König
2021-01-25 19:24 ` Junio C Hamano
0 siblings, 1 reply; 63+ messages in thread
From: Uwe Kleine-König @ 2021-01-25 7:35 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin via GitGitGadget, git, Eric Sunshine,
Johannes Schindelin
[-- 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 --]
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH v2 1/3] range-diff/format-patch: refactor check for commit range
2021-01-25 7:35 ` Uwe Kleine-König
@ 2021-01-25 19:24 ` Junio C Hamano
2021-01-25 21:25 ` Uwe Kleine-König
0 siblings, 1 reply; 63+ messages in thread
From: Junio C Hamano @ 2021-01-25 19:24 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Johannes Schindelin via GitGitGadget, git, Eric Sunshine,
Johannes Schindelin
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 ;-)
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH v2 1/3] range-diff/format-patch: refactor check for commit range
2021-01-25 19:24 ` Junio C Hamano
@ 2021-01-25 21:25 ` Uwe Kleine-König
2021-01-26 19:25 ` Junio C Hamano
0 siblings, 1 reply; 63+ messages in thread
From: Uwe Kleine-König @ 2021-01-25 21:25 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin via GitGitGadget, git, Eric Sunshine,
Johannes Schindelin
[-- 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 --]
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH v2 1/3] range-diff/format-patch: refactor check for commit range
2021-01-25 21:25 ` Uwe Kleine-König
@ 2021-01-26 19:25 ` Junio C Hamano
0 siblings, 0 replies; 63+ messages in thread
From: Junio C Hamano @ 2021-01-26 19:25 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Johannes Schindelin via GitGitGadget, git, Eric Sunshine,
Johannes Schindelin
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
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH v2 2/3] range-diff/format-patch: handle commit ranges other than A..B
2021-01-22 18:16 ` [PATCH v2 " Johannes Schindelin via GitGitGadget
2021-01-22 18:16 ` [PATCH v2 1/3] range-diff/format-patch: refactor check for commit range Johannes Schindelin via GitGitGadget
@ 2021-01-22 18:16 ` Johannes Schindelin via GitGitGadget
2021-01-22 20:32 ` Junio C Hamano
2021-01-22 18:16 ` [PATCH v2 3/3] range-diff(docs): explain how to specify commit ranges Johannes Schindelin via GitGitGadget
2021-01-27 16:37 ` [PATCH v3 0/3] Range diff with ranges lacking dotdot Johannes Schindelin via GitGitGadget
3 siblings, 1 reply; 63+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-01-22 18:16 UTC (permalink / raw)
To: git
Cc: Uwe Kleine-König, Eric Sunshine, Johannes Schindelin,
Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* Re: [PATCH v2 2/3] range-diff/format-patch: handle commit ranges other than A..B
2021-01-22 18:16 ` [PATCH v2 2/3] range-diff/format-patch: handle commit ranges other than A..B Johannes Schindelin via GitGitGadget
@ 2021-01-22 20:32 ` Junio C Hamano
2021-01-27 2:57 ` Johannes Schindelin
0 siblings, 1 reply; 63+ messages in thread
From: Junio C Hamano @ 2021-01-22 20:32 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Uwe Kleine-König, Eric Sunshine, Johannes Schindelin
"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.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH v2 2/3] range-diff/format-patch: handle commit ranges other than A..B
2021-01-22 20:32 ` Junio C Hamano
@ 2021-01-27 2:57 ` Johannes Schindelin
2021-01-28 5:57 ` Junio C Hamano
0 siblings, 1 reply; 63+ messages in thread
From: Johannes Schindelin @ 2021-01-27 2:57 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin via GitGitGadget, git, Uwe Kleine-König,
Eric Sunshine
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
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH v2 2/3] range-diff/format-patch: handle commit ranges other than A..B
2021-01-27 2:57 ` Johannes Schindelin
@ 2021-01-28 5:57 ` Junio C Hamano
2021-01-28 15:38 ` Johannes Schindelin
0 siblings, 1 reply; 63+ messages in thread
From: Junio C Hamano @ 2021-01-28 5:57 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Johannes Schindelin via GitGitGadget, git, Uwe Kleine-König,
Eric Sunshine
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.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH v2 2/3] range-diff/format-patch: handle commit ranges other than A..B
2021-01-28 5:57 ` Junio C Hamano
@ 2021-01-28 15:38 ` Johannes Schindelin
[not found] ` <xmqqim7gx41d.fsf@gitster.c.googlers.com>
0 siblings, 1 reply; 63+ messages in thread
From: Johannes Schindelin @ 2021-01-28 15:38 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin via GitGitGadget, git, Uwe Kleine-König,
Eric Sunshine
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
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH v2 3/3] range-diff(docs): explain how to specify commit ranges
2021-01-22 18:16 ` [PATCH v2 " Johannes Schindelin via GitGitGadget
2021-01-22 18:16 ` [PATCH v2 1/3] range-diff/format-patch: refactor check for commit range Johannes Schindelin via GitGitGadget
2021-01-22 18:16 ` [PATCH v2 2/3] range-diff/format-patch: handle commit ranges other than A..B Johannes Schindelin via GitGitGadget
@ 2021-01-22 18:16 ` Johannes Schindelin via GitGitGadget
2021-01-27 16:37 ` [PATCH v3 0/3] Range diff with ranges lacking dotdot Johannes Schindelin via GitGitGadget
3 siblings, 0 replies; 63+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-01-22 18:16 UTC (permalink / raw)
To: git
Cc: Uwe Kleine-König, Eric Sunshine, Johannes Schindelin,
Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH v3 0/3] Range diff with ranges lacking dotdot
2021-01-22 18:16 ` [PATCH v2 " Johannes Schindelin via GitGitGadget
` (2 preceding siblings ...)
2021-01-22 18:16 ` [PATCH v2 3/3] range-diff(docs): explain how to specify commit ranges Johannes Schindelin via GitGitGadget
@ 2021-01-27 16:37 ` Johannes Schindelin via GitGitGadget
2021-01-27 16:37 ` [PATCH v3 1/3] range-diff/format-patch: refactor check for commit range Johannes Schindelin via GitGitGadget
` (3 more replies)
3 siblings, 4 replies; 63+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-01-27 16:37 UTC (permalink / raw)
To: git; +Cc: Uwe Kleine-König, Eric Sunshine, Johannes Schindelin
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
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH v3 1/3] range-diff/format-patch: refactor check for commit range
2021-01-27 16:37 ` [PATCH v3 0/3] Range diff with ranges lacking dotdot Johannes Schindelin via GitGitGadget
@ 2021-01-27 16:37 ` Johannes Schindelin via GitGitGadget
2021-01-27 16:37 ` [PATCH v3 2/3] range-diff/format-patch: handle commit ranges other than A..B Johannes Schindelin via GitGitGadget
` (2 subsequent siblings)
3 siblings, 0 replies; 63+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-01-27 16:37 UTC (permalink / raw)
To: git
Cc: Uwe Kleine-König, Eric Sunshine, Johannes Schindelin,
Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH v3 2/3] range-diff/format-patch: handle commit ranges other than A..B
2021-01-27 16:37 ` [PATCH v3 0/3] Range diff with ranges lacking dotdot Johannes Schindelin via GitGitGadget
2021-01-27 16:37 ` [PATCH v3 1/3] range-diff/format-patch: refactor check for commit range Johannes Schindelin via GitGitGadget
@ 2021-01-27 16:37 ` Johannes Schindelin via GitGitGadget
2021-01-27 16:37 ` [PATCH v3 3/3] range-diff(docs): explain how to specify commit ranges Johannes Schindelin via GitGitGadget
2021-02-04 9:31 ` [PATCH v4 0/3] Range diff with ranges lacking dotdot Johannes Schindelin via GitGitGadget
3 siblings, 0 replies; 63+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-01-27 16:37 UTC (permalink / raw)
To: git
Cc: Uwe Kleine-König, Eric Sunshine, Johannes Schindelin,
Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH v3 3/3] range-diff(docs): explain how to specify commit ranges
2021-01-27 16:37 ` [PATCH v3 0/3] Range diff with ranges lacking dotdot Johannes Schindelin via GitGitGadget
2021-01-27 16:37 ` [PATCH v3 1/3] range-diff/format-patch: refactor check for commit range Johannes Schindelin via GitGitGadget
2021-01-27 16:37 ` [PATCH v3 2/3] range-diff/format-patch: handle commit ranges other than A..B Johannes Schindelin via GitGitGadget
@ 2021-01-27 16:37 ` Johannes Schindelin via GitGitGadget
2021-02-04 9:31 ` [PATCH v4 0/3] Range diff with ranges lacking dotdot Johannes Schindelin via GitGitGadget
3 siblings, 0 replies; 63+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-01-27 16:37 UTC (permalink / raw)
To: git
Cc: Uwe Kleine-König, Eric Sunshine, Johannes Schindelin,
Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH v4 0/3] Range diff with ranges lacking dotdot
2021-01-27 16:37 ` [PATCH v3 0/3] Range diff with ranges lacking dotdot Johannes Schindelin via GitGitGadget
` (2 preceding siblings ...)
2021-01-27 16:37 ` [PATCH v3 3/3] range-diff(docs): explain how to specify commit ranges Johannes Schindelin via GitGitGadget
@ 2021-02-04 9:31 ` Johannes Schindelin via GitGitGadget
2021-02-04 9:31 ` [PATCH v4 1/3] range-diff/format-patch: refactor check for commit range Johannes Schindelin via GitGitGadget
` (3 more replies)
3 siblings, 4 replies; 63+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-02-04 9:31 UTC (permalink / raw)
To: git; +Cc: Uwe Kleine-König, Eric Sunshine, Johannes Schindelin
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
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH v4 1/3] range-diff/format-patch: refactor check for commit range
2021-02-04 9:31 ` [PATCH v4 0/3] Range diff with ranges lacking dotdot Johannes Schindelin via GitGitGadget
@ 2021-02-04 9:31 ` Johannes Schindelin via GitGitGadget
2021-02-04 18:56 ` Junio C Hamano
2021-02-04 9:31 ` [PATCH v4 2/3] range-diff/format-patch: handle commit ranges other than A..B Johannes Schindelin via GitGitGadget
` (2 subsequent siblings)
3 siblings, 1 reply; 63+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-02-04 9:31 UTC (permalink / raw)
To: git
Cc: Uwe Kleine-König, Eric Sunshine, Johannes Schindelin,
Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* Re: [PATCH v4 1/3] range-diff/format-patch: refactor check for commit range
2021-02-04 9:31 ` [PATCH v4 1/3] range-diff/format-patch: refactor check for commit range Johannes Schindelin via GitGitGadget
@ 2021-02-04 18:56 ` Junio C Hamano
2021-02-04 19:27 ` Johannes Schindelin
0 siblings, 1 reply; 63+ messages in thread
From: Junio C Hamano @ 2021-02-04 18:56 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Uwe Kleine-König, Eric Sunshine, Johannes Schindelin
"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.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH v4 1/3] range-diff/format-patch: refactor check for commit range
2021-02-04 18:56 ` Junio C Hamano
@ 2021-02-04 19:27 ` Johannes Schindelin
0 siblings, 0 replies; 63+ messages in thread
From: Johannes Schindelin @ 2021-02-04 19:27 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin via GitGitGadget, git, Uwe Kleine-König,
Eric Sunshine
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
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH v4 2/3] range-diff/format-patch: handle commit ranges other than A..B
2021-02-04 9:31 ` [PATCH v4 0/3] Range diff with ranges lacking dotdot Johannes Schindelin via GitGitGadget
2021-02-04 9:31 ` [PATCH v4 1/3] range-diff/format-patch: refactor check for commit range Johannes Schindelin via GitGitGadget
@ 2021-02-04 9:31 ` Johannes Schindelin via GitGitGadget
2021-02-04 18:51 ` Junio C Hamano
2021-02-04 18:58 ` Junio C Hamano
2021-02-04 9:31 ` [PATCH v4 3/3] range-diff(docs): explain how to specify commit ranges Johannes Schindelin via GitGitGadget
2021-02-04 23:29 ` [PATCH v5 0/3] Range diff with ranges lacking dotdot Johannes Schindelin via GitGitGadget
3 siblings, 2 replies; 63+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-02-04 9:31 UTC (permalink / raw)
To: git
Cc: Uwe Kleine-König, Eric Sunshine, Johannes Schindelin,
Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* Re: [PATCH v4 2/3] range-diff/format-patch: handle commit ranges other than A..B
2021-02-04 9:31 ` [PATCH v4 2/3] range-diff/format-patch: handle commit ranges other than A..B Johannes Schindelin via GitGitGadget
@ 2021-02-04 18:51 ` Junio C Hamano
2021-02-04 21:42 ` Johannes Schindelin
2021-02-04 18:58 ` Junio C Hamano
1 sibling, 1 reply; 63+ messages in thread
From: Junio C Hamano @ 2021-02-04 18:51 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Uwe Kleine-König, Eric Sunshine, Johannes Schindelin
"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 &&
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH v4 2/3] range-diff/format-patch: handle commit ranges other than A..B
2021-02-04 18:51 ` Junio C Hamano
@ 2021-02-04 21:42 ` Johannes Schindelin
0 siblings, 0 replies; 63+ messages in thread
From: Johannes Schindelin @ 2021-02-04 21:42 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin via GitGitGadget, git, Uwe Kleine-König,
Eric Sunshine
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 &&
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH v4 2/3] range-diff/format-patch: handle commit ranges other than A..B
2021-02-04 9:31 ` [PATCH v4 2/3] range-diff/format-patch: handle commit ranges other than A..B Johannes Schindelin via GitGitGadget
2021-02-04 18:51 ` Junio C Hamano
@ 2021-02-04 18:58 ` Junio C Hamano
2021-02-04 21:57 ` Johannes Schindelin
1 sibling, 1 reply; 63+ messages in thread
From: Junio C Hamano @ 2021-02-04 18:58 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Uwe Kleine-König, Eric Sunshine, Johannes Schindelin
"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".
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH v4 2/3] range-diff/format-patch: handle commit ranges other than A..B
2021-02-04 18:58 ` Junio C Hamano
@ 2021-02-04 21:57 ` Johannes Schindelin
0 siblings, 0 replies; 63+ messages in thread
From: Johannes Schindelin @ 2021-02-04 21:57 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin via GitGitGadget, git, Uwe Kleine-König,
Eric Sunshine
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
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH v4 3/3] range-diff(docs): explain how to specify commit ranges
2021-02-04 9:31 ` [PATCH v4 0/3] Range diff with ranges lacking dotdot Johannes Schindelin via GitGitGadget
2021-02-04 9:31 ` [PATCH v4 1/3] range-diff/format-patch: refactor check for commit range Johannes Schindelin via GitGitGadget
2021-02-04 9:31 ` [PATCH v4 2/3] range-diff/format-patch: handle commit ranges other than A..B Johannes Schindelin via GitGitGadget
@ 2021-02-04 9:31 ` Johannes Schindelin via GitGitGadget
2021-02-04 18:53 ` Junio C Hamano
2021-02-04 23:29 ` [PATCH v5 0/3] Range diff with ranges lacking dotdot Johannes Schindelin via GitGitGadget
3 siblings, 1 reply; 63+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-02-04 9:31 UTC (permalink / raw)
To: git
Cc: Uwe Kleine-König, Eric Sunshine, Johannes Schindelin,
Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* Re: [PATCH v4 3/3] range-diff(docs): explain how to specify commit ranges
2021-02-04 9:31 ` [PATCH v4 3/3] range-diff(docs): explain how to specify commit ranges Johannes Schindelin via GitGitGadget
@ 2021-02-04 18:53 ` Junio C Hamano
2021-02-04 21:58 ` Johannes Schindelin
0 siblings, 1 reply; 63+ messages in thread
From: Junio C Hamano @ 2021-02-04 18:53 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Uwe Kleine-König, Eric Sunshine, Johannes Schindelin
"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
> -------
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH v4 3/3] range-diff(docs): explain how to specify commit ranges
2021-02-04 18:53 ` Junio C Hamano
@ 2021-02-04 21:58 ` Johannes Schindelin
2021-02-04 22:42 ` Junio C Hamano
0 siblings, 1 reply; 63+ messages in thread
From: Johannes Schindelin @ 2021-02-04 21:58 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin via GitGitGadget, git, Uwe Kleine-König,
Eric Sunshine
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
> > -------
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH v4 3/3] range-diff(docs): explain how to specify commit ranges
2021-02-04 21:58 ` Johannes Schindelin
@ 2021-02-04 22:42 ` Junio C Hamano
0 siblings, 0 replies; 63+ messages in thread
From: Junio C Hamano @ 2021-02-04 22:42 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Johannes Schindelin via GitGitGadget, git, Uwe Kleine-König,
Eric Sunshine
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.
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH v5 0/3] Range diff with ranges lacking dotdot
2021-02-04 9:31 ` [PATCH v4 0/3] Range diff with ranges lacking dotdot Johannes Schindelin via GitGitGadget
` (2 preceding siblings ...)
2021-02-04 9:31 ` [PATCH v4 3/3] range-diff(docs): explain how to specify commit ranges Johannes Schindelin via GitGitGadget
@ 2021-02-04 23:29 ` Johannes Schindelin via GitGitGadget
2021-02-04 23:29 ` [PATCH v5 1/3] range-diff/format-patch: refactor check for commit range Johannes Schindelin via GitGitGadget
` (3 more replies)
3 siblings, 4 replies; 63+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-02-04 23:29 UTC (permalink / raw)
To: git; +Cc: Uwe Kleine-König, Eric Sunshine, Johannes Schindelin
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
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH v5 1/3] range-diff/format-patch: refactor check for commit range
2021-02-04 23:29 ` [PATCH v5 0/3] Range diff with ranges lacking dotdot Johannes Schindelin via GitGitGadget
@ 2021-02-04 23:29 ` Johannes Schindelin via GitGitGadget
2021-02-04 23:29 ` [PATCH v5 2/3] range-diff/format-patch: handle commit ranges other than A..B Johannes Schindelin via GitGitGadget
` (2 subsequent siblings)
3 siblings, 0 replies; 63+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-02-04 23:29 UTC (permalink / raw)
To: git
Cc: Uwe Kleine-König, Eric Sunshine, Johannes Schindelin,
Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH v5 2/3] range-diff/format-patch: handle commit ranges other than A..B
2021-02-04 23:29 ` [PATCH v5 0/3] Range diff with ranges lacking dotdot Johannes Schindelin via GitGitGadget
2021-02-04 23:29 ` [PATCH v5 1/3] range-diff/format-patch: refactor check for commit range Johannes Schindelin via GitGitGadget
@ 2021-02-04 23:29 ` Johannes Schindelin via GitGitGadget
2021-02-05 1:07 ` Junio C Hamano
2021-02-04 23:29 ` [PATCH v5 3/3] range-diff(docs): explain how to specify commit ranges Johannes Schindelin via GitGitGadget
2021-02-05 14:44 ` [PATCH v6 0/3] Range diff with ranges lacking dotdot Johannes Schindelin via GitGitGadget
3 siblings, 1 reply; 63+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-02-04 23:29 UTC (permalink / raw)
To: git
Cc: Uwe Kleine-König, Eric Sunshine, Johannes Schindelin,
Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* Re: [PATCH v5 2/3] range-diff/format-patch: handle commit ranges other than A..B
2021-02-04 23:29 ` [PATCH v5 2/3] range-diff/format-patch: handle commit ranges other than A..B Johannes Schindelin via GitGitGadget
@ 2021-02-05 1:07 ` Junio C Hamano
2021-02-05 1:32 ` Junio C Hamano
0 siblings, 1 reply; 63+ messages in thread
From: Junio C Hamano @ 2021-02-05 1:07 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Uwe Kleine-König, Eric Sunshine, Johannes Schindelin
"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.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH v5 2/3] range-diff/format-patch: handle commit ranges other than A..B
2021-02-05 1:07 ` Junio C Hamano
@ 2021-02-05 1:32 ` Junio C Hamano
2021-02-05 14:09 ` Johannes Schindelin
0 siblings, 1 reply; 63+ messages in thread
From: Junio C Hamano @ 2021-02-05 1:32 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Uwe Kleine-König, Eric Sunshine, Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* Re: [PATCH v5 2/3] range-diff/format-patch: handle commit ranges other than A..B
2021-02-05 1:32 ` Junio C Hamano
@ 2021-02-05 14:09 ` Johannes Schindelin
0 siblings, 0 replies; 63+ messages in thread
From: Johannes Schindelin @ 2021-02-05 14:09 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin via GitGitGadget, git, Uwe Kleine-König,
Eric Sunshine
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
>
>
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH v5 3/3] range-diff(docs): explain how to specify commit ranges
2021-02-04 23:29 ` [PATCH v5 0/3] Range diff with ranges lacking dotdot Johannes Schindelin via GitGitGadget
2021-02-04 23:29 ` [PATCH v5 1/3] range-diff/format-patch: refactor check for commit range Johannes Schindelin via GitGitGadget
2021-02-04 23:29 ` [PATCH v5 2/3] range-diff/format-patch: handle commit ranges other than A..B Johannes Schindelin via GitGitGadget
@ 2021-02-04 23:29 ` Johannes Schindelin via GitGitGadget
2021-02-05 14:44 ` [PATCH v6 0/3] Range diff with ranges lacking dotdot Johannes Schindelin via GitGitGadget
3 siblings, 0 replies; 63+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-02-04 23:29 UTC (permalink / raw)
To: git
Cc: Uwe Kleine-König, Eric Sunshine, Johannes Schindelin,
Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH v6 0/3] Range diff with ranges lacking dotdot
2021-02-04 23:29 ` [PATCH v5 0/3] Range diff with ranges lacking dotdot Johannes Schindelin via GitGitGadget
` (2 preceding siblings ...)
2021-02-04 23:29 ` [PATCH v5 3/3] range-diff(docs): explain how to specify commit ranges Johannes Schindelin via GitGitGadget
@ 2021-02-05 14:44 ` Johannes Schindelin via GitGitGadget
2021-02-05 14:44 ` [PATCH v6 1/3] range-diff/format-patch: refactor check for commit range Johannes Schindelin via GitGitGadget
` (2 more replies)
3 siblings, 3 replies; 63+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-02-05 14:44 UTC (permalink / raw)
To: git; +Cc: Uwe Kleine-König, Eric Sunshine, Johannes Schindelin
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
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH v6 1/3] range-diff/format-patch: refactor check for commit range
2021-02-05 14:44 ` [PATCH v6 0/3] Range diff with ranges lacking dotdot Johannes Schindelin via GitGitGadget
@ 2021-02-05 14:44 ` Johannes Schindelin via GitGitGadget
2021-02-05 14:44 ` [PATCH v6 2/3] range-diff/format-patch: handle commit ranges other than A..B Johannes Schindelin via GitGitGadget
2021-02-05 14:44 ` [PATCH v6 3/3] range-diff(docs): explain how to specify commit ranges Johannes Schindelin via GitGitGadget
2 siblings, 0 replies; 63+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-02-05 14:44 UTC (permalink / raw)
To: git
Cc: Uwe Kleine-König, Eric Sunshine, Johannes Schindelin,
Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH v6 2/3] range-diff/format-patch: handle commit ranges other than A..B
2021-02-05 14:44 ` [PATCH v6 0/3] Range diff with ranges lacking dotdot Johannes Schindelin via GitGitGadget
2021-02-05 14:44 ` [PATCH v6 1/3] range-diff/format-patch: refactor check for commit range Johannes Schindelin via GitGitGadget
@ 2021-02-05 14:44 ` Johannes Schindelin via GitGitGadget
2021-02-05 14:44 ` [PATCH v6 3/3] range-diff(docs): explain how to specify commit ranges Johannes Schindelin via GitGitGadget
2 siblings, 0 replies; 63+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-02-05 14:44 UTC (permalink / raw)
To: git
Cc: Uwe Kleine-König, Eric Sunshine, Johannes Schindelin,
Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH v6 3/3] range-diff(docs): explain how to specify commit ranges
2021-02-05 14:44 ` [PATCH v6 0/3] Range diff with ranges lacking dotdot Johannes Schindelin via GitGitGadget
2021-02-05 14:44 ` [PATCH v6 1/3] range-diff/format-patch: refactor check for commit range Johannes Schindelin via GitGitGadget
2021-02-05 14:44 ` [PATCH v6 2/3] range-diff/format-patch: handle commit ranges other than A..B Johannes Schindelin via GitGitGadget
@ 2021-02-05 14:44 ` Johannes Schindelin via GitGitGadget
2 siblings, 0 replies; 63+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2021-02-05 14:44 UTC (permalink / raw)
To: git
Cc: Uwe Kleine-König, Eric Sunshine, Johannes Schindelin,
Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 63+ messages in thread