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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Wed, 27 Jan 2021, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > 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?

I hadn't thought of that.

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

It should probably be libified anyway if it calles `die()` (I don't know
off the top of my head).

Worse than the `die()` is probably the allocated memory; IIRC there is no
way to release the memory allocated by `setup_revisions()` unless one
performs a revwalk.

Will try to find some time to play with the `setup_revisions()` idea.

Ciao,
Dscho