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

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: "SZEDER Gábor" <szeder.dev@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	David Turner <novalis@novalis.org>,
	git@vger.kernel.org
Subject: Re: [PATCH 5/5] path.c: don't call the match function without value in trie_find()
Date: Mon, 28 Oct 2019 11:57:10 +0100 (CET)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.1910281155220.46@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20191021160043.701-6-szeder.dev@gmail.com>

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

Hi Gábor,

On Mon, 21 Oct 2019, 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
>     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.
>
>   - When the match function check_common() is invoked with a NULL
>     value, it returns 0, which indicates that the queried path doesn't
>     belong to the common directory, ultimately resulting the bogus
>     path shown above.
>
> 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.
>
> I believe that there are no other paths that could cause similar bogus
> output.  AFAICT the only other key resulting in the match function
> being called with a NULL value is 'co' (because of the keys 'common'
> and 'config').  However, as they are not in a directory that belongs
> to the common directory the resulting working tree-specific path is
> expected.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>

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".

But as I said, this is just a nit-pick.

Thank you for working on this!
Dscho

> +	}
>
>  	/* 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;
> -
>  	if (dir->is_dir && (unmatched[0] == 0 || unmatched[0] == '/'))
>  		return dir->is_common;
>
> diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
> index c7b53e494b..501e1a288d 100755
> --- a/t/t0060-path-utils.sh
> +++ b/t/t0060-path-utils.sh
> @@ -288,6 +288,8 @@ test_git_path GIT_COMMON_DIR=bar index                    .git/index
>  test_git_path GIT_COMMON_DIR=bar HEAD                     .git/HEAD
>  test_git_path GIT_COMMON_DIR=bar logs/HEAD                .git/logs/HEAD
>  test_git_path GIT_COMMON_DIR=bar logs/refs/bisect/foo     .git/logs/refs/bisect/foo
> +test_git_path GIT_COMMON_DIR=bar logs/refs                bar/logs/refs
> +test_git_path GIT_COMMON_DIR=bar logs/refs/               bar/logs/refs/
>  test_git_path GIT_COMMON_DIR=bar logs/refs/bisec/foo      bar/logs/refs/bisec/foo
>  test_git_path GIT_COMMON_DIR=bar logs/refs/bisec          bar/logs/refs/bisec
>  test_git_path GIT_COMMON_DIR=bar logs/refs/bisectfoo      bar/logs/refs/bisectfoo
> --
> 2.24.0.rc0.472.ga6f06c86b4
>
>