Re: [PATCH 5/5] path.c: don't call the match function without value in trie_find()
- From
SZEDER Gábor <szeder.dev@gmail.com>
- Date
- Oct 28, 2019, 12:00 UTC
- Message-ID
- <20191028120054.GS4348@szeder.dev>
- In-Reply-To
- <nycvar.QRO.7.76.6.1910281155220.46@tvgsbejvaqbjf.bet>
On Mon, Oct 28, 2019 at 11:57:10AM +0100, Johannes Schindelin wrote:
Show 6 quoted lines
> > - 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.
Show 28 quoted lines
> 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.