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

2 messages, 2 authors, 2021-01-27 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2021-01-22 20:35:50

"Johannes Schindelin via GitGitGadget" [off-list ref]
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.

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

From: Johannes Schindelin <hidden>
Date: 2021-01-27 15:52:52

Hi Junio,

On Fri, 22 Jan 2021, Junio C Hamano wrote:
"Johannes Schindelin via GitGitGadget" [off-list ref]
writes:
quoted
+	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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help