Re: [PATCH 5/5] path.c: don't call the match function without value in trie_find()
On Wed, Oct 23, 2019 at 01:01:00PM +0900, Junio C Hamano wrote: > SZEDER Gábor writes: > > >> - 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. > > Given that the trie is maintained by hand in common_list[], I wonder > if we can mechanically catch errors like the one b9317d55a3 added, > by perhaps having a self-test function that a t/helper/ program > calls to perform consistency check after the "git" gets built. I'm not sure what you mean by "consistency check". The resulting trie looked as expected both before and after b9317d55a3, i.e. each trie node had the right contents, value, and children, as far as I could tell. The issue was in the lookup function.
Re: [PATCH 5/5] path.c: don't call the match function without value in trie_find()
SZEDER Gábor writes: >> - 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. Given that the trie is maintained by hand in common_list[], I wonder if we can mechanically catch errors like the one b9317d55a3 added, by perhaps having a self-test function that a t/helper/ program calls to perform consistency check after the "git" gets built. Thanks.
Re: [PATCH 5/5] path.c: don't call the match function without value in trie_find()
On Mon, Oct 21, 2019 at 06:00:43PM +0200, SZEDER Gábor wrote: > '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.
Re: [PATCH 5/5] path.c: don't call the match function without value in trie_find()
On Mon, 2019-10-21 at 18:00 +0200, SZEDER Gábor wrote:
> Add the missing condition to trie_find() so it will never invoke the
> match function with a non-existing value. check_common() will then
> no
> longer have to check that it got a non-NULL value, so remove that
> condition.
...
>
> /* Partial path normalization: skip consecutive slashes */
> while (key[0] == '/' && key[1] == '/')
> @@ -345,9 +349,6 @@ static int check_common(const char *unmatched,
> void *value, void *baton)
> {
> struct common_dir *dir = value;
>
> - if (!dir)
> - return 0;
Do we want to assert(dir) here?
Overall, LGTM. Thanks for the clean-up.
