Hi Junio,
On Thu, 21 Jan 2021, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@xxxxxx>
> >
> > 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@xxxxxx>
> > ---
> > 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
