Handle git_path() with lock files correctly in worktrees

From: Johannes Schindelin <[email protected]>

Ever since worktrees were introduced, the git_path() function _really_ needed to be called e.g. to get at the index. However, the wrong path is returned for index.lock.

This does not matter as long as the Git executable is doing the asking, as the path for that index.lock file is constructed from git_path("index") by appending the .lock suffix.

However, Git GUI just learned to use --git-path instead of appending relative paths to what git rev-parse --git-dir returns (and as a consequence not only using the correct hooks directory, but also using the correct paths in worktrees other than the main one). And one of the paths it is looking for is... you guessed it... index.lock.

So let's make that work as script writers would expect it to.

Signed-off-by Johannes Schindelin <[email protected]>

---
 path.c               |  4 ++--
 t/t1500-rev-parse.sh | 15 +++++++++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

path.c +2 -2

index e3da1f3c4e..ff85692b45 100644
@@ -268,7 +268,7 @@ static int trie_find(struct trie *root, const char *key, match_fn fn,
 	int result;
 	struct trie *child;
 
-	if (!*key) {
+	if (!*key || !strcmp(key, ".lock")) {
 		/* we have reached the end of the key */
 		if (root->value && !root->len)
 			return fn(key, root->value, baton);

@@ -288,7 +288,7 @@ static int trie_find(struct trie *root, const char *key, match_fn fn,
 
 	/* Matched the entire compressed section */
 	key += i;
-	if (!*key)
+	if (!*key || !strcmp(key, ".lock"))
 		/* End of key */
 		return fn(key, root->value, baton);
 

t/t1500-rev-parse.sh +3 -0

index 01abee533d..d318a1eeef 100755
@@ -116,6 +116,21 @@ test_expect_success 'git-path inside sub-dir' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git-path in worktree' '
+	test_tick &&
+	git commit --allow-empty -m empty &&

t/t1500-rev-parse.sh +2 -0

+'
+
 test_expect_success 'rev-parse --is-shallow-repository in shallow repo' '
 	test_commit test_commit &&
 	echo true >expect &&

Signature · 1 line

gitgitgadget

However, I'm not sure what the right path should be in the first place, given that each working tree has its own 'logs' directory, but only for HEAD's reflog, while everything else goes to the main working tree's 'logs' directory.

Quoted 5 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

Ever since worktrees were introduced, the git_path() function _really_ needed to be called e.g. to get at the index. However, the wrong path is returned for index.lock.

Could you give an example where it returns the wrong path for 'index.lock'?

I tried to reproduce this issue in a working tree, but
no matter what I've tried, 'git rev-parse --git-dir index.lock' always returned the right path.

Quoted 51 lines

This does not matter as long as the Git executable is doing the asking, as the path for that index.lock file is constructed from git_path("index") by appending the .lock suffix.

However, Git GUI just learned to use --git-path instead of appending relative paths to what git rev-parse --git-dir returns (and as a consequence not only using the correct hooks directory, but also using the correct paths in worktrees other than the main one). And one of the paths it is looking for is... you guessed it... index.lock.

So let's make that work as script writers would expect it to.

Signed-off-by Johannes Schindelin <[email protected]>

---
 path.c               |  4 ++--
 t/t1500-rev-parse.sh | 15 +++++++++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

path.c +2 -2

index e3da1f3c4e..ff85692b45 100644
@@ -268,7 +268,7 @@ static int trie_find(struct trie *root, const char *key, match_fn fn,
 	int result;
 	struct trie *child;
 
-	if (!*key) {
+	if (!*key || !strcmp(key, ".lock")) {
 		/* we have reached the end of the key */
 		if (root->value && !root->len)
 			return fn(key, root->value, baton);

@@ -288,7 +288,7 @@ static int trie_find(struct trie *root, const char *key, match_fn fn,
 
 	/* Matched the entire compressed section */
 	key += i;
-	if (!*key)
+	if (!*key || !strcmp(key, ".lock"))
 		/* End of key */
 		return fn(key, root->value, baton);
 

t/t1500-rev-parse.sh +3 -0

index 01abee533d..d318a1eeef 100755
@@ -116,6 +116,21 @@ test_expect_success 'git-path inside sub-dir' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git-path in worktree' '
+	test_tick &&
+	git commit --allow-empty -m empty &&

t/t1500-rev-parse.sh test_expect_success 'git-path inside sub-dir' ' · line 131

Quoted 10 lines

t/t1500-rev-parse.sh +10 -0

+	git worktree add --detach wt &&
+	test_write_lines >expect \
+		"$(pwd)/.git/worktrees/wt/logs/HEAD" \
+		"$(pwd)/.git/worktrees/wt/logs/HEAD.lock" \
+		"$(pwd)/.git/worktrees/wt/index" \
+		"$(pwd)/.git/worktrees/wt/index.lock" &&
+	git -C wt rev-parse >actual \
+		--git-path logs/HEAD --git-path logs/HEAD.lock \
+		--git-path index --git-path index.lock &&
+	test_cmp expect actual

Without the fix applied this test fails with:

+ test_cmp expect actual
--- expect 2019-10-16 10:20:31.047229423 +0000
+++ actual 2019-10-16 10:20:31.051229519 +0000
@@ -1,4 +1,4 @@
/home/szeder/src/git/t/trash directory.t1500-rev-parse/.git/worktrees/wt/logs/HEAD
-/home/szeder/src/git/t/trash directory.t1500-rev-parse/.git/worktrees/wt/logs/HEAD.lock +/home/szeder/src/git/t/trash directory.t1500-rev-parse/.git/logs/HEAD.lock
/home/szeder/src/git/t/trash directory.t1500-rev-parse/.git/worktrees/wt/index /home/szeder/src/git/t/trash directory.t1500-rev-parse/.git/worktrees/wt/index.lock
error: last command exited with $?=1

So the path of 'index.lock' seems to be fine already, it's the path of the lockfile for HEAD's reflog that's indeed wrong and makes the test fail.

On a related note, I'm not sure whether the path of the reflogs directory is right while in a different working tree... Both with and without this patch I get a path pointing to the main working tree:

  $ ./git -C WT/ rev-parse --git-path logs
  /home/szeder/src/git/.git/logs

However, I'm not sure what the right path should be in the first place, given that each working tree has its own 'logs' directory, but only for HEAD's reflog, while everything else goes to the main working tree's 'logs' directory.

Quoted 8 lines

t/t1500-rev-parse.sh +2 -1

+'
+
 test_expect_success 'rev-parse --is-shallow-repository in shallow repo' '
 	test_commit test_commit &&
 	echo true >expect &&
--

gitgitgadget

From: Johannes Schindelin <[email protected]>

Ever since worktrees were introduced, the git_path() function _really_ needed to be called e.g. to get at the path to logs/HEAD (HEAD is specific to the worktree). However, the wrong path is returned for logs/HEAD.lock.

This does not matter as long as the Git executable is doing the asking, as the path for that index.lock file is constructed from git_path("index") by appending the .lock suffix.

Side note: Git GUI _does_ ask for index.lock, but that is already resolved correctly.

Quoted 3 lines

I tried to reproduce this issue in a working tree, but
no matter what I've tried, 'git rev-parse --git-dir index.lock' always returned the right path.

With s/--git-dir/--git-path/, I agree.

Quoted 11 lines

On a related note, I'm not sure whether the path of the reflogs directory is right while in a different working tree... Both with and without this patch I get a path pointing to the main working tree:

  $ ./git -C WT/ rev-parse --git-path logs
  /home/szeder/src/git/.git/logs

However, I'm not sure what the right path should be in the first place, given that each working tree has its own 'logs' directory, but only for HEAD's reflog, while everything else goes to the main working tree's 'logs' directory.

It's like Junio said, the reflog for HEAD is special because HEAD is special. Look for common_list in path.c (it is a bit confusing, I admit, you have to look for the 3rd column of numbers: if it is a 1, then it is a worktree-specific path, if it is 0, it is supposed to live in the "commondir", i.e. in the gitdir of the main worktree).

Quoted 12 lines

Quoted 11 lines

On a related note, I'm not sure whether the path of the reflogs directory is right while in a different working tree... Both with and without this patch I get a path pointing to the main working tree:

  $ ./git -C WT/ rev-parse --git-path logs
  /home/szeder/src/git/.git/logs

However, I'm not sure what the right path should be in the first place, given that each working tree has its own 'logs' directory, but only for HEAD's reflog, while everything else goes to the main working tree's 'logs' directory.

Quoted 5 lines

It's like Junio said, the reflog for HEAD is special because HEAD is special. Look for common_list in path.c (it is a bit confusing, I admit, you have to look for the 3rd column of numbers: if it is a 1, then it is a worktree-specific path, if it is 0, it is supposed to live in the "commondir", i.e. in the gitdir of the main worktree).

OK, got it.

I didn't look yesterday at all, but now I did, and, unfortunately, see two more bugs, and one of them is a "proper" bug leading to bogus output:

  $ 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/

I didn't look yesterday at all, but now I did, and, unfortunately, see two more bugs

Quoted 8 lines

path.c +0 -0

index e3da1f3c4e..ff85692b45 100644
@@ -268,7 +268,7 @@ static int trie_find(struct trie *root, const char *key, match_fn fn,
 	int result;
 	struct trie *child;
 

path.c static int trie_find(struct trie *root, const char *key, match_fn fn, · line 271

Quoted 2 lines

path.c +1 -1

-	if (!*key) {
+	if (!*key || !strcmp(key, ".lock")) {

We only do strcmp for the tail part at the end of the path, so this should probably OK from performance point of view but semantically it is not very satisfying to see a special case for a single .suffix this deep in the callchain. I wonder if it is nicer to have the higher level callers notice ".lock" or whatever other suffixes they care about and ask the lower layer for a key with the suffix stripped?

Quoted 1 line

unfortunately, see two more bugs,

Quoted 6 lines

and one of them is a "proper" bug leading to bogus output:

  $ 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/

This one-liner below fixes it, but I haven't yet made up my mind about whether this is the right fix or whether there could be any fallout (at least the test suite doesn't show any).

  $ ./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/logs/refs/

path.c +0 -0

index 04b69b9feb..9019169418 100644

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.

Quoted 2 lines · SZEDER Gábor

I didn't look yesterday at all, but now I did, and, unfortunately, see two more bugs

The second patch is kind of a bugfix, though luckily the bug doesn't actually manifest in undesired behavior.
The first is a minor cleanup, so future readers won't have a 'Huh?' moment like I just did.

SZEDER Gábor (2):
path.c: fix field name in 'struct common_dir' path.c: mark 'logs/HEAD' in 'common_list' as file

 path.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

--
2.23.0.1084.gae250eaa40

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 7 lines · writes:

From: Johannes Schindelin <[email protected]>

Ever since worktrees were introduced, the git_path() function _really_ needed to be called e.g. to get at the path to logs/HEAD (HEAD is specific to the worktree). However, the wrong path is returned for logs/HEAD.lock.

Quoted 3 lines

This does not matter as long as the Git executable is doing the asking, as the path for that index.lock file is constructed from git_path("index") by appending the .lock suffix.

Is this still git_path("index") or is it now HEAD?

Quoted 2 lines

Side note: Git GUI _does_ ask for index.lock, but that is already resolved correctly.

Is that s/but/and/?

Quoted 8 lines

path.c +0 -0

index e3da1f3c4e..ff85692b45 100644
@@ -268,7 +268,7 @@ static int trie_find(struct trie *root, const char *key, match_fn fn,
 	int result;
 	struct trie *child;
 

path.c static int trie_find(struct trie *root, const char *key, match_fn fn, · line 271

Quoted 2 lines

path.c +1 -1

-	if (!*key) {
+	if (!*key || !strcmp(key, ".lock")) {

We only do strcmp for the tail part at the end of the path, so this should probably OK from performance point of view but semantically it is not very satisfying to see a special case for a single .suffix this deep in the callchain. I wonder if it is nicer to have the higher level callers notice ".lock" or whatever other suffixes they care about and ask the lower layer for a key with the suffix stripped?

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

From: Johannes Schindelin <[email protected]>

Ever since worktrees were introduced, the git_path() function _really_ needed to be called e.g. to get at the path to logs/HEAD (HEAD is specific to the worktree, and therefore so is its reflog). However, the wrong path is returned for logs/HEAD.lock.

This does not matter as long as the Git executable is doing the asking, as the path for that logs/HEAD.lock file is constructed from git_path("logs/HEAD") by appending the .lock suffix.

However, Git GUI just learned to use --git-path instead of appending relative paths to what git rev-parse --git-dir returns (and as a consequence not only using the correct hooks directory, but also using the correct paths in worktrees other than the main one). While it does not seem as if Git GUI in particular is asking for logs/HEAD.lock, let's be safe rather than sorry.

Side note: Git GUI _does_ ask for index.lock, but that is already resolved correctly, due to update_common_dir() preferring to leave unknown paths in the (worktree-specific) git directory.

Signed-off-by Johannes Schindelin <[email protected]>

---
 path.c               |  8 +++++++-
 t/t1500-rev-parse.sh | 15 +++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

path.c +0 -0

index e3da1f3c4e..5ff64e7a8a 100644

path.c +2 -0

 	init_common_trie();
 	if (trie_find(&common_trie, base, check_common, NULL) > 0)
 		replace_dir(buf, git_dir_len, common_dir);
+
+	free(base2);
 }
 
 void report_linked_checkout_garbage(void)

t/t1500-rev-parse.sh +4 -0

index 01abee533d..d318a1eeef 100755
@@ -116,6 +116,21 @@ test_expect_success 'git-path inside sub-dir' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git-path in worktree' '
+	test_tick &&
+	git commit --allow-empty -m empty &&
+	git worktree add --detach wt &&

t/t1500-rev-parse.sh +0 -0

 test_expect_success 'rev-parse --is-shallow-repository in shallow repo' '
 	test_commit test_commit &&
 	echo true >expect &&

Signature · 1 line

gitgitgadget

However, I think this is _really_ ugly and intrusive. The opposite of what my goals were.

So I think I'll just bite the bullet and use a temporary copy if the argument ends in .lock.

path.c static int check_common(const char *unmatched, void *value, void *baton) · line 355

Quoted 4 lines

path.c +4 -0

+	char *base = buf->buf + git_dir_len, *base2 = NULL;
+
+	if (ends_with(base, ".lock"))
+		base = base2 = xstrndup(base, strlen(base) - 5);

Hm, this adds the magic number 5 and calls strlen(base) twice, because ends_with() calls strip_suffix(), which calls strlen(). Calling strip_suffix() directly would allow us to avoid the magic number and the second strlen():

  size_t len;
  if (strip_suffix(base, ".lock", &len))
          base = base2 = xstrndup(base, len);

Quoted 4 lines

- 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/'!

Quoted 4 lines

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.

Quoted 11 lines · SZEDER Gábor

Quoted 4 lines

- 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/'!

Quoted 4 lines

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.

'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 <[email protected]>

path.c +0 -0

index cf57bd52dd..e21b00c4d4 100644
@@ -299,9 +299,13 @@ static int trie_find(struct trie *root, const char *key, match_fn fn,
 

path.c +1 -3

+	}
 
 	/* 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;
 

t/t0060-path-utils.sh +2 -1

index c7b53e494b..501e1a288d 100755
@@ -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

Quoted 6 lines

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

Thank you for this entire patch series. Just one nit:

Quoted 6 lines

path.c +0 -0

index cf57bd52dd..e21b00c4d4 100644
@@ -299,9 +299,13 @@ static int trie_find(struct trie *root, const char *key, match_fn fn,
 

path.c static int trie_find(struct trie *root, const char *key, match_fn fn, · line 307

Quoted 10 lines

path.c +5 -2

 	/* 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:

(quoted diff) +3 -0

+		if (!root->value)
+			return -1;
+		return fn(key, root->value, baton);

... as it would more accurately reflect my mental model of an "early out".

Quoted 33 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

Ever since worktrees were introduced, the git_path() function _really_ needed to be called e.g. to get at the path to logs/HEAD (HEAD is specific to the worktree, and therefore so is its reflog). However, the wrong path is returned for logs/HEAD.lock.

This does not matter as long as the Git executable is doing the asking, as the path for that logs/HEAD.lock file is constructed from git_path("logs/HEAD") by appending the .lock suffix.

However, Git GUI just learned to use --git-path instead of appending relative paths to what git rev-parse --git-dir returns (and as a consequence not only using the correct hooks directory, but also using the correct paths in worktrees other than the main one). While it does not seem as if Git GUI in particular is asking for logs/HEAD.lock, let's be safe rather than sorry.

Side note: Git GUI _does_ ask for index.lock, but that is already resolved correctly, due to update_common_dir() preferring to leave unknown paths in the (worktree-specific) git directory.

Signed-off-by Johannes Schindelin <[email protected]>

---
 path.c               |  8 +++++++-
 t/t1500-rev-parse.sh | 15 +++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

path.c +0 -0

index e3da1f3c4e..5ff64e7a8a 100644

path.c static int check_common(const char *unmatched, void *value, void *baton) · line 355

Quoted 8 lines

path.c +4 -1

@@ -349,10 +349,16 @@ static int check_common(const char *unmatched, void *value, void *baton)
 static void update_common_dir(struct strbuf *buf, int git_dir_len,
 			      const char *common_dir)
 {
-	char *base = buf->buf + git_dir_len;
+	char *base = buf->buf + git_dir_len, *base2 = NULL;
+
+	if (ends_with(base, ".lock"))
+		base = base2 = xstrndup(base, strlen(base) - 5);

Hm, this adds the magic number 5 and calls strlen(base) twice, because ends_with() calls strip_suffix(), which calls strlen(). Calling strip_suffix() directly would allow us to avoid the magic number and the second strlen():

  size_t len;
  if (strip_suffix(base, ".lock", &len))
          base = base2 = xstrndup(base, len);

Quoted 28 lines

path.c +2 -0

 	init_common_trie();
 	if (trie_find(&common_trie, base, check_common, NULL) > 0)
 		replace_dir(buf, git_dir_len, common_dir);
+
+	free(base2);
 }
 
 void report_linked_checkout_garbage(void)

t/t1500-rev-parse.sh +11 -0

index 01abee533d..d318a1eeef 100755
@@ -116,6 +116,21 @@ test_expect_success 'git-path inside sub-dir' '
 	test_cmp expect actual
 '
 
+test_expect_success 'git-path in worktree' '
+	test_tick &&
+	git commit --allow-empty -m empty &&
+	git worktree add --detach wt &&
+	test_write_lines >expect \
+		"$(pwd)/.git/worktrees/wt/logs/HEAD" \
+		"$(pwd)/.git/worktrees/wt/logs/HEAD.lock" \
+		"$(pwd)/.git/worktrees/wt/index" \
+		"$(pwd)/.git/worktrees/wt/index.lock" &&
+	git -C wt rev-parse >actual \
+		--git-path logs/HEAD --git-path logs/HEAD.lock \

t/t1500-rev-parse.sh test_expect_success 'git-path inside sub-dir' ' · line 132

Quoted 3 lines

t/t1500-rev-parse.sh +3 -0

+		--git-path index --git-path index.lock &&
+	test_cmp expect actual
+'

I think this test better fits into 't0060-path-utils.sh': it already checks 'logs/HEAD' and 'index' in a different working tree (well, with GIT_COMMON_DIR set, but that's the same), and it has a helper function to make each of the two new .lock tests a one-liner.

Quoted 6 lines

t/t1500-rev-parse.sh +0 -1

 test_expect_success 'rev-parse --is-shallow-repository in shallow repo' '
 	test_commit test_commit &&
 	echo true >expect &&
--

gitgitgadget

Quoted 15 lines

path.c static int check_common(const char *unmatched, void *value, void *baton) · line 355

Quoted 9 lines

path.c +4 -1

@@ -349,10 +349,16 @@ static int check_common(const char *unmatched, void *value, void *baton)
 static void update_common_dir(struct strbuf *buf, int git_dir_len,
 			      const char *common_dir)
 {
-	char *base = buf->buf + git_dir_len;
+	char *base = buf->buf + git_dir_len, *base2 = NULL;
+
+	if (ends_with(base, ".lock"))
+		base = base2 = xstrndup(base, strlen(base) - 5);

Hm, this adds the magic number 5 and calls strlen(base) twice, because ends_with() calls strip_suffix(), which calls strlen(). Calling strip_suffix() directly would allow us to avoid the magic number and the second strlen():

Quoted 3 lines

  size_t len;
  if (strip_suffix(base, ".lock", &len))
          base = base2 = xstrndup(base, len);

Actually, we should probably avoid the extra allocation. Earlier, I was concerned about multi-threading issues when attempting to modify the strbuf. But then, we modify that strbuf a couple of lines later anyway,

so my fears were totally unfounded. Therefore, my current version reads like this:

-- snip --

	int has_lock_suffix = strbuf_strip_suffix(buf, LOCK_SUFFIX);

	init_common_trie();
	if (trie_find(&common_trie, base, check_common, NULL) > 0)
		replace_dir(buf, git_dir_len, common_dir);

	if (has_lock_suffix)
		strbuf_addstr(buf, LOCK_SUFFIX);

-- snap --

Quoted 3 lines

  size_t len;
  if (strip_suffix(base, ".lock", &len))
          base = base2 = xstrndup(base, len);

Actually, we should probably avoid the extra allocation. Earlier, I was concerned about multi-threading issues when attempting to modify the strbuf. But then, we modify that strbuf a couple of lines later anyway, so my fears were totally unfounded. Therefore, my current version reads like this:

-- snip --

	int has_lock_suffix = strbuf_strip_suffix(buf, LOCK_SUFFIX);

	init_common_trie();
	if (trie_find(&common_trie, base, check_common, NULL) > 0)
		replace_dir(buf, git_dir_len, common_dir);

	if (has_lock_suffix)
		strbuf_addstr(buf, LOCK_SUFFIX);

-- snap --

Quoted 33 lines · Johannes Schindelin

Quoted 6 lines

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

Thank you for this entire patch series. Just one nit:

Quoted 6 lines

path.c +0 -0

index cf57bd52dd..e21b00c4d4 100644
@@ -299,9 +299,13 @@ static int trie_find(struct trie *root, const char *key, match_fn fn,
 

path.c static int trie_find(struct trie *root, const char *key, match_fn fn, · line 307

Quoted 10 lines

path.c +5 -2

 	/* 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:

(quoted diff) +3 -0

+		if (!root->value)
+			return -1;
+		return fn(key, root->value, baton);

Quoted 2 lines

... as it would more accurately reflect my mental model of an "early out".

The checks at the other two of those three callsites look like this, and I just followed suit for the sake of consistency.

Changes since v3:

* Instead of duplicating the path when it has a .lock suffix, we now use strbuf manipulation to strip the suffix temporarily.
* The test case in t1500 was scrapped in favor of a much simpler pair of test cases in t0060.