Re: [PATCH 5/5] path.c: don't call the match function without value in trie_find()

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

[-- Attachment #1: Type: text/plain, Size: 1532 bytes --]

Hi Gábor,

On Mon, 28 Oct 2019, SZEDER Gábor wrote:

> On Mon, Oct 28, 2019 at 11:57:10AM +0100, Johannes Schindelin wrote:
> > >   - According to the comment describing trie_find(), it should only
> > >     call the given match function 'fn' for a "/-or-\0-terminated
> > >     prefix of the key for which the trie contains a value".  This is
> > >     not true: there are three places where trie_find() calls the match
> > >     function, but one of them is missing the check for value's
> > >     existence.
>
> > Thank you for this entire patch series. Just one nit:
> >
> >
> > > diff --git a/path.c b/path.c
> > > index cf57bd52dd..e21b00c4d4 100644
> > > --- a/path.c
> > > +++ b/path.c
> > > @@ -299,9 +299,13 @@ static int trie_find(struct trie *root, const char *key, match_fn fn,
> > >
> > >  	/* Matched the entire compressed section */
> > >  	key += i;
> > > -	if (!*key)
> > > +	if (!*key) {
> > >  		/* End of key */
> > > -		return fn(key, root->value, baton);
> > > +		if (root->value)
> > > +			return fn(key, root->value, baton);
> > > +		else
> > > +			return -1;
> >
> > I would have preferred this:
> >
> > +		if (!root->value)
> > +			return -1;
> > +		return fn(key, root->value, baton);
> >
> > ... as it would more accurately reflect my mental model of an "early
> > out".
>
> The checks at the other two of those three callsites look like this,
> and I just followed suit for the sake of consistency.

Oh, okay. Sorry for the noise, then.

Thanks,
Dscho