git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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
>
>

  parent reply	other threads:[~2019-10-28 10:57 UTC|newest]

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-16  7:07 [PATCH 0/2] Handle git_path() with lock files correctly in worktrees Johannes Schindelin via GitGitGadget
2019-10-16  7:07 ` [PATCH 1/2] t1400: wrap setup code in test case Johannes Schindelin via GitGitGadget
2019-10-16  7:07 ` [PATCH 2/2] git_path(): handle `.lock` files correctly Johannes Schindelin via GitGitGadget
2019-10-16 11:04   ` SZEDER Gábor
2019-10-17  7:15     ` Junio C Hamano
2019-10-17 22:05     ` Johannes Schindelin
2019-10-18 11:06       ` SZEDER Gábor
2019-10-18 11:35         ` SZEDER Gábor
2019-10-21 16:00           ` [PATCH 0/5] path.c: a couple of common dir/trie fixes SZEDER Gábor
2019-10-21 16:00             ` [PATCH 1/5] Documentation: mention more worktree-specific exceptions SZEDER Gábor
2019-10-21 16:00             ` [PATCH 2/5] path.c: clarify trie_find()'s in-code comment SZEDER Gábor
2019-10-21 16:00             ` [PATCH 3/5] path.c: mark 'logs/HEAD' in 'common_list' as file SZEDER Gábor
2019-10-21 16:00             ` [PATCH 4/5] path.c: clarify two field names in 'struct common_dir' SZEDER Gábor
2019-10-21 16:00             ` [PATCH 5/5] path.c: don't call the match function without value in trie_find() SZEDER Gábor
2019-10-21 17:39               ` David Turner
2019-10-21 20:57               ` SZEDER Gábor
2019-10-23  4:01                 ` Junio C Hamano
2019-10-23 16:20                   ` SZEDER Gábor
2019-10-24  3:29                     ` Junio C Hamano
2019-10-28 10:57               ` Johannes Schindelin [this message]
2019-10-28 12:00                 ` SZEDER Gábor
2019-10-28 21:30                   ` Johannes Schindelin
2019-10-18 11:42         ` [PATCH 0/2] path.c: minor common_list fixes SZEDER Gábor
2019-10-18 11:42           ` [PATCH 1/2] path.c: fix field name in 'struct common_dir' SZEDER Gábor
2019-10-18 11:42           ` [PATCH 2/2] path.c: mark 'logs/HEAD' in 'common_list' as file SZEDER Gábor
2019-10-21 19:35           ` [PATCH 0/2] path.c: minor common_list fixes Johannes Schindelin
2019-10-17 22:07 ` [PATCH v2 0/2] Handle git_path() with lock files correctly in worktrees Johannes Schindelin via GitGitGadget
2019-10-17 22:07   ` [PATCH v2 1/2] t1400: wrap setup code in test case Johannes Schindelin via GitGitGadget
2019-10-17 22:07   ` [PATCH v2 2/2] git_path(): handle `.lock` files correctly Johannes Schindelin via GitGitGadget
2019-10-18  1:23     ` Junio C Hamano
2019-10-18 12:35       ` SZEDER Gábor
2019-10-21 20:26       ` Johannes Schindelin
2019-10-23  2:12         ` Junio C Hamano
2019-10-21 21:54   ` [PATCH v3 0/2] Handle git_path() with lock files correctly in worktrees Johannes Schindelin via GitGitGadget
2019-10-21 21:54     ` [PATCH v3 1/2] t1400: wrap setup code in test case Johannes Schindelin via GitGitGadget
2019-10-21 21:54     ` [PATCH v3 2/2] git_path(): handle `.lock` files correctly Johannes Schindelin via GitGitGadget
2019-10-22 16:01       ` SZEDER Gábor
2019-10-23  3:38         ` Junio C Hamano
2019-10-28 12:01         ` Johannes Schindelin
2019-10-28 12:32           ` SZEDER Gábor
2019-10-28 17:30           ` Junio C Hamano
2019-10-28 12:57     ` [PATCH v4 0/2] Handle git_path() with lock files correctly in worktrees Johannes Schindelin via GitGitGadget
2019-10-28 12:57       ` [PATCH v4 1/2] t1400: wrap setup code in test case Johannes Schindelin via GitGitGadget
2019-10-28 12:57       ` [PATCH v4 2/2] git_path(): handle `.lock` files correctly Johannes Schindelin via GitGitGadget
2019-10-29  3:39       ` [PATCH v4 0/2] Handle git_path() with lock files correctly in worktrees Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=nycvar.QRO.7.76.6.1910281155220.46@tvgsbejvaqbjf.bet \
    --to=johannes.schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=novalis@novalis.org \
    --cc=szeder.dev@gmail.com \
    --subject='Re: [PATCH 5/5] path.c: don'\''t call the match function without value in trie_find()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).