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 28, 2021, 15:38 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2101281636400.54@tvgsbejvaqbjf.bet>
- In-Reply-To
- <xmqqa6sty55k.fsf@gitster.c.googlers.com>
Hi Junio,
On Wed, 27 Jan 2021, Junio C Hamano wrote:
Show 25 quoted lines
> 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.
Show 8 quoted lines
> 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