| Message ID | 88c15617b4ba8ae3211b1a01861eb4165f3eda38.1611267638.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Range diff with ranges lacking dotdot | expand |
On Thu, Jan 21, 2021 at 5:22 PM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote: > In the `SPECIFYING RANGES` section of gitrevisions[7], two ways are > described to specify commit ranges that `range-diff` does not yet > accept: "<commit>^!" and "<commit>^-<n>". > > Let's accept them. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > diff --git a/builtin/range-diff.c b/builtin/range-diff.c > @@ -13,7 +13,26 @@ NULL > static int is_range(const char *range) > { > + if (strstr(range, "..")) > + return 1; > + > + i = strlen(range); > + c = i ? range[--i] : 0; > + if (c == '!') > + i--; /* might be ...^! or ...^@ */ > + else if (isdigit(c)) { > + /* handle ...^-<n> */ > + while (i > 2 && isdigit(range[--i])) > + ; /* keep trimming trailing digits */ > + if (i < 2 || range[i--] != '-') > + return 0; > + } else > + return 0; > + > + return i > 0 && range[i] == '^'; > } Is this something that the --range-diff option of git-format-patch will want to do, as well? At present, builtin/log.c:infer_range_diff_ranges() detects a range only by checking for "..", much like this function did before this patch. If so, perhaps this function can be part of the public range-diff API (or, indeed, part of some other more general API if it's not really specific to range-diff).
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > In the `SPECIFYING RANGES` section of gitrevisions[7], two ways are > described to specify commit ranges that `range-diff` does not yet > accept: "<commit>^!" and "<commit>^-<n>". > > Let's accept them. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > builtin/range-diff.c | 21 ++++++++++++++++++++- > t/t3206-range-diff.sh | 8 ++++++++ > 2 files changed, 28 insertions(+), 1 deletion(-) > > diff --git a/builtin/range-diff.c b/builtin/range-diff.c > index 551d3e689cb..6097635c432 100644 > --- a/builtin/range-diff.c > +++ b/builtin/range-diff.c > @@ -13,7 +13,26 @@ NULL > > static int is_range(const char *range) > { > - return !!strstr(range, ".."); > + size_t i; > + char c; > + > + if (strstr(range, "..")) > + return 1; > + > + i = strlen(range); > + c = i ? range[--i] : 0; > + if (c == '!') > + i--; /* might be ...^! or ...^@ */ I am confused. If it ends with '!', I do not see how it can end with "^@". If the input were "!", i gets strlen("!") which is 1, c gets '!' while predecrementing i down to 0, and we notice c is '!' and decrement i again to make it (size_t)(-1) which is a fairly large number. Then we skip all the else/if cascade, ensure that i is positive, and happily access range[i], which likely is way out of bounds (but it probably is almost one turn around the earth out of bounds, it may access just a single byte before the array). Am I reading the code right? IOW, "git range-diff \! A..B" would do something strange, I would guess. > + else if (isdigit(c)) { > + /* handle ...^-<n> */ > + while (i > 2 && isdigit(range[--i])) > + ; /* keep trimming trailing digits */ > + if (i < 2 || range[i--] != '-') > + return 0; > + } else > + return 0; > + > + return i > 0 && range[i] == '^'; > } > > int cmd_range_diff(int argc, const char **argv, const char *prefix) > diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh > index 6eb344be031..e217cecac9e 100755 > --- a/t/t3206-range-diff.sh > +++ b/t/t3206-range-diff.sh > @@ -150,6 +150,14 @@ test_expect_success 'simple A B C (unmodified)' ' > test_cmp expect actual > ' > > +test_expect_success 'A^! and A^-<n> (unmodified)' ' > + git range-diff --no-color topic^! unmodified^-1 >actual && > + cat >expect <<-EOF && > + 1: $(test_oid t4) = 1: $(test_oid u4) s/12/B/ > + EOF > + test_cmp expect actual > +' > + > test_expect_success 'trivial reordering' ' > git range-diff --no-color master topic reordered >actual && > cat >expect <<-EOF &&
Hi Eric, On Thu, 21 Jan 2021, Eric Sunshine wrote: > On Thu, Jan 21, 2021 at 5:22 PM Johannes Schindelin via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > In the `SPECIFYING RANGES` section of gitrevisions[7], two ways are > > described to specify commit ranges that `range-diff` does not yet > > accept: "<commit>^!" and "<commit>^-<n>". > > > > Let's accept them. > > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > --- > > diff --git a/builtin/range-diff.c b/builtin/range-diff.c > > @@ -13,7 +13,26 @@ NULL > > static int is_range(const char *range) > > { > > + if (strstr(range, "..")) > > + return 1; > > + > > + i = strlen(range); > > + c = i ? range[--i] : 0; > > + if (c == '!') > > + i--; /* might be ...^! or ...^@ */ > > + else if (isdigit(c)) { > > + /* handle ...^-<n> */ > > + while (i > 2 && isdigit(range[--i])) > > + ; /* keep trimming trailing digits */ > > + if (i < 2 || range[i--] != '-') > > + return 0; > > + } else > > + return 0; > > + > > + return i > 0 && range[i] == '^'; > > } > > Is this something that the --range-diff option of git-format-patch > will want to do, as well? Thank you for pointing that out. I should have checked via `git grep 'strstr.*"\.\."'` myself. There are two more instances, one in `rev-parse.c` and the other in `revision.c`, but both are necessary as-are because their return value is actually used to further disect a `..`-style commit range. Thanks, Dscho > At present, builtin/log.c:infer_range_diff_ranges() detects a range only > by checking for "..", much like this function did before this patch. If > so, perhaps this function can be part of the public range-diff API (or, > indeed, part of some other more general API if it's not really specific > to range-diff).
Hi Junio, On Thu, 21 Jan 2021, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > > > In the `SPECIFYING RANGES` section of gitrevisions[7], two ways are > > described to specify commit ranges that `range-diff` does not yet > > accept: "<commit>^!" and "<commit>^-<n>". > > > > Let's accept them. > > > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > --- > > builtin/range-diff.c | 21 ++++++++++++++++++++- > > t/t3206-range-diff.sh | 8 ++++++++ > > 2 files changed, 28 insertions(+), 1 deletion(-) > > > > diff --git a/builtin/range-diff.c b/builtin/range-diff.c > > index 551d3e689cb..6097635c432 100644 > > --- a/builtin/range-diff.c > > +++ b/builtin/range-diff.c > > @@ -13,7 +13,26 @@ NULL > > > > static int is_range(const char *range) > > { > > - return !!strstr(range, ".."); > > + size_t i; > > + char c; > > + > > + if (strstr(range, "..")) > > + return 1; > > + > > + i = strlen(range); > > + c = i ? range[--i] : 0; > > + if (c == '!') > > + i--; /* might be ...^! or ...^@ */ > > I am confused. If it ends with '!', I do not see how it can end > with "^@". Bah. This is a left-over from an earlier version. I tried to hide the fact that I had misunderstood `<rev>^@` to specify a commit range. Oh well. > If the input were "!", i gets strlen("!") which is 1, c gets '!' > while predecrementing i down to 0, and we notice c is '!' and > decrement i again to make it (size_t)(-1) which is a fairly large > number. Right, guarding that `range[--i]` only by `i` is not enough. My idea was to exit early if the string is too short, anyway, i.e. if `i < 3`. > Then we skip all the else/if cascade, ensure that i is positive, and > happily access range[i], which likely is way out of bounds (but it > probably is almost one turn around the earth out of bounds, it may > access just a single byte before the array). > > Am I reading the code right? > > IOW, "git range-diff \! A..B" would do something strange, I would > guess. Right. Will be fixed in the next iteration. Thanks, Dscho
diff --git a/builtin/range-diff.c b/builtin/range-diff.c index 551d3e689cb..6097635c432 100644 --- a/builtin/range-diff.c +++ b/builtin/range-diff.c @@ -13,7 +13,26 @@ NULL static int is_range(const char *range) { - return !!strstr(range, ".."); + size_t i; + char c; + + if (strstr(range, "..")) + return 1; + + i = strlen(range); + c = i ? range[--i] : 0; + if (c == '!') + i--; /* might be ...^! or ...^@ */ + else if (isdigit(c)) { + /* handle ...^-<n> */ + while (i > 2 && isdigit(range[--i])) + ; /* keep trimming trailing digits */ + if (i < 2 || range[i--] != '-') + return 0; + } else + return 0; + + return i > 0 && range[i] == '^'; } int cmd_range_diff(int argc, const char **argv, const char *prefix) diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh index 6eb344be031..e217cecac9e 100755 --- a/t/t3206-range-diff.sh +++ b/t/t3206-range-diff.sh @@ -150,6 +150,14 @@ test_expect_success 'simple A B C (unmodified)' ' test_cmp expect actual ' +test_expect_success 'A^! and A^-<n> (unmodified)' ' + git range-diff --no-color topic^! unmodified^-1 >actual && + cat >expect <<-EOF && + 1: $(test_oid t4) = 1: $(test_oid u4) s/12/B/ + EOF + test_cmp expect actual +' + test_expect_success 'trivial reordering' ' git range-diff --no-color master topic reordered >actual && cat >expect <<-EOF &&