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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 27, 2021, 02:57 UTC
Message-ID: <nycvar.QRO.7.76.6.2101270342140.57@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqqv9bo7lwd.fsf@gitster.c.googlers.com>

Hi Junio,

On Fri, 22 Jan 2021, Junio C Hamano wrote:

Show 21 quoted lines

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> 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 ;-)

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?

Show 5 quoted lines

> 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