Re: [PATCH 2/3] range-diff: handle commit ranges other than A..B

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 22, 2021, 16:20 UTC
Message-ID: <nycvar.QRO.7.76.6.2101221713230.52@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqq1redc0x9.fsf@gitster.c.googlers.com>

Hi Junio,

On Thu, 21 Jan 2021, Junio C Hamano wrote:

Show 39 quoted lines

> "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`.

Show 9 quoted lines

> 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