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

Hi Junio,

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

"Johannes Schindelin via GitGitGadget" [off-list ref]
writes:

quoted

From: Johannes Schindelin <redacted>

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 <redacted>
---
 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