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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 28, 2021, 05:57 UTC
Message-ID: <xmqqa6sty55k.fsf@gitster.c.googlers.com>
In-Reply-To: <nycvar.QRO.7.76.6.2101270342140.57@tvgsbejvaqbjf.bet>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 17 quoted lines

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

Readable, yes, acceptable, I don't know, and I am not even sure if I want to be the one to judge to be honest ;-)

Have you tried the approach to feed the thing to setup_revisions() and inspect what objects are in revs.pending?

When you got a valid range, you should find one or more positive and one or more negative commits , and the approach won't be fooled by strings like "HEAD^{/other than A..B/}".

Or does the revision parsing machinery too eager to "die" when we find a syntax error? If so, scratch that idea, but in the longer haul, fixing these die's would also be something we'd want to do to make more parts of libgit.a callable as a proper library.

Thanks.