Re: [PATCH v3] line-range: Fix infinite loop bug with degenerate '$' regex

[Lars Kellogg-Stedman <lars@oddbit.com>]

On Sun, Dec 11, 2022 at 12:32:38PM +0900, Junio C Hamano wrote:
> Lars Kellogg-Stedman <lars@oddbit.com> writes:
> 
> > When the -L argument to "git log" is passed the degenerate regular
> > expression "$" (as in "-L :$:line-range.c"), this results in an
> > infinite loop in find_funcname_matching_regexp().
> 
> Is "matching an empty line" the only way a regular expression can be
> a "degenerate" one?  If not, perhaps being a bit more explicit would
> help the readers, e.g.

"Matching an empty line" isn't the issue (and if I implied that it
was, that's my bad). The issue only crops up when matching the
zero-width regular expression at the *end* of a line.

I'll update the subject and description to be more clear...

> This clear explanation probably deserves to be in the commit log
> message proper.

...including adding the extended description to the commit message. Some projects
object if commit messages get too wordy.

> Please write it on two lines, and highlight an empty body of the
> loop, like so

Will do.

> > @@ -161,6 +160,7 @@ static const char *find_funcname_matching_regexp(xdemitconf_t *xecfg, const char
> >  			return bol;
> >  		start = eol;
> >  	}
> > +	return NULL;
> >  }
> 
> What is this change about?  Isn't the above an endless loop without
> break, from which the only way for the control to leave it is by
> returning?

It's not an endless loop without break; this change modified the loop
condition:

> -       while (1) {
> +       while (*start) {

That would have prevented the infinite loop in the first place. I'm
happy to drop this change if you prefer, but if this condition had
been in place originally we wouldn't have had the infinite loop bug
(we would still have had incorrect behavior, but it would have been
perhaps easier for an end user to diagnose).

> Style: lose the SP after "cat >".

Will do.

-- 
Lars Kellogg-Stedman <lars@oddbit.com> | larsks @ {irc,twitter,github}
http://blog.oddbit.com/                | N1LKS