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
>
>
next prev 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
List information: http://vger.kernel.org/majordomo-info.html
* 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 \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).