git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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.

Previous: Johannes SchindelinNext: Johannes Schindelin
Message 13 of 14 in “path.c: a couple of common dir/trie fixes”
  1. 0/5 path.c: a couple of common dir/trie fixesSZEDER Gábor, Oct 21, 2019
  2. 1/5 Documentation: mention more worktree-specific exceptionsSZEDER Gábor, Oct 21, 2019
  3. 3/5 path.c: mark 'logs/HEAD' in 'common_list' as fileSZEDER Gábor, Oct 21, 2019
  4. 2/5 path.c: clarify trie_find()'s in-code commentSZEDER Gábor, Oct 21, 2019
  5. 4/5 path.c: clarify two field names in 'struct common_dir'SZEDER Gábor, Oct 21, 2019
  6. 5/5 path.c: don't call the match function without value in trie_find()SZEDER Gábor, Oct 21, 2019
  7. David TurnerOct 21, 2019
  8. SZEDER GáborOct 21, 2019
  9. Junio C HamanoOct 23, 2019
  10. SZEDER GáborOct 23, 2019
  11. Junio C HamanoOct 24, 2019
  12. Johannes SchindelinOct 28, 2019
  13. SZEDER GáborOct 28, 2019
  14. Johannes SchindelinOct 28, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.