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 21, 2019, 20:57 UTC
Message-ID: <20191021205703.GB4348@szeder.dev>
In-Reply-To: <20191021160043.701-6-szeder.dev@gmail.com>

On Mon, Oct 21, 2019 at 06:00:43PM +0200, SZEDER Gábor wrote:

Show 26 quoted lines

> 'logs/refs' is not a working tree-specific path, but since commit
> b9317d55a3 (Make sure refs/rewritten/ is per-worktree, 2019-03-07)
> 'git rev-parse --git-path' has been returning a bogus path if a
> trailing '/' is present:
> 
>   $ git -C WT/ rev-parse --git-path logs/refs --git-path logs/refs/
>   /home/szeder/src/git/.git/logs/refs
>   /home/szeder/src/git/.git/worktrees/WT/logs/refs/
> 
> We use a trie data structure to efficiently decide whether a path
> belongs to the common dir or is working tree-specific.  As it happens
> b9317d55a3 triggered a bug that is as old as the trie implementation
> itself, added in 4e09cf2acf (path: optimize common dir checking,
> 2015-08-31).
> 
>   - 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.
> 
>   - b9317d55a3 added two new keys to the trie: 'logs/refs/rewritten'
>     and 'logs/refs/worktree', next to the already existing
>     'logs/refs/bisect'.  This resulted in a trie node with the path
>     'logs/refs', which didn't exist before, and which doesn't have a

Oops, I missed the trailing slash, that must be 'logs/refs/'!

>     value attached.  A query for 'logs/refs/' finds this node and then
>     hits that one callsite of the match function which doesn't check
>     for the value's existence, and thus invokes the match function
>     with NULL as value.