All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Handle git_path() with lock files correctly in worktrees
@ 2019-10-16  7:07 Johannes Schindelin via GitGitGadget
  2019-10-16  7:07 ` [PATCH 1/2] t1400: wrap setup code in test case Johannes Schindelin via GitGitGadget
                   ` (2 more replies)
  0 siblings, 3 replies; 45+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-10-16  7:07 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Junio C Hamano

I stumbled over this during my recent work in Git GUI
[https://github.com/gitgitgadget/git/pull/361] that was originally really
only intended to use the correct hooks directory.

Technically, the first patch is not needed (because I decided against adding
a test to t1400 in the end, in favor of t1500), but it shouldn't hurt,
either.

Johannes Schindelin (2):
  t1400: wrap setup code in test case
  git_path(): handle `.lock` files correctly

 path.c                |  4 ++--
 t/t1400-update-ref.sh | 18 ++++++++++--------
 t/t1500-rev-parse.sh  | 15 +++++++++++++++
 3 files changed, 27 insertions(+), 10 deletions(-)


base-commit: 108b97dc372828f0e72e56bbb40cae8e1e83ece6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-401%2Fdscho%2Flock-files-in-worktrees-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-401/dscho/lock-files-in-worktrees-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/401
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH 1/2] t1400: wrap setup code in test case
  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 ` Johannes Schindelin via GitGitGadget
  2019-10-16  7:07 ` [PATCH 2/2] git_path(): handle `.lock` files correctly Johannes Schindelin via GitGitGadget
  2019-10-17 22:07 ` [PATCH v2 0/2] Handle git_path() with lock files correctly in worktrees Johannes Schindelin via GitGitGadget
  2 siblings, 0 replies; 45+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-10-16  7:07 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Without this, you cannot use `--run=<...>` to skip that part, and a run
with `--run=0` (which is a common way to determine the test case number
corresponding to a given test case title).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t1400-update-ref.sh | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 1fbd940408..69a7f27311 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -344,14 +344,16 @@ test_expect_success "verifying $m's log (logged by config)" '
 	test_cmp expect .git/logs/$m
 '
 
-git update-ref $m $D
-cat >.git/logs/$m <<EOF
-$Z $C $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 -0500
-$C $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150350 -0500
-$A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 -0500
-$F $Z $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150680 -0500
-$Z $E $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 -0500
-EOF
+test_expect_success 'set up for querying the reflog' '
+	git update-ref $m $D &&
+	cat >.git/logs/$m <<-EOF
+	$Z $C $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 -0500
+	$C $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150350 -0500
+	$A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 -0500
+	$F $Z $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150680 -0500
+	$Z $E $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 -0500
+	EOF
+'
 
 ed="Thu, 26 May 2005 18:32:00 -0500"
 gd="Thu, 26 May 2005 18:33:00 -0500"
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [PATCH 2/2] git_path(): handle `.lock` files correctly
  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 ` Johannes Schindelin via GitGitGadget
  2019-10-16 11:04   ` SZEDER Gábor
  2019-10-17 22:07 ` [PATCH v2 0/2] Handle git_path() with lock files correctly in worktrees Johannes Schindelin via GitGitGadget
  2 siblings, 1 reply; 45+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-10-16  7:07 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 path.c               |  4 ++--
 t/t1500-rev-parse.sh | 15 +++++++++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/path.c b/path.c
index e3da1f3c4e..ff85692b45 100644
--- a/path.c
+++ b/path.c
@@ -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);
 
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 01abee533d..d318a1eeef 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -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 \
+		--git-path index --git-path index.lock &&
+	test_cmp expect actual
+'
+
 test_expect_success 'rev-parse --is-shallow-repository in shallow repo' '
 	test_commit test_commit &&
 	echo true >expect &&
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 45+ messages in thread

* Re: [PATCH 2/2] git_path(): handle `.lock` files correctly
  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
  0 siblings, 2 replies; 45+ messages in thread
From: SZEDER Gábor @ 2019-10-16 11:04 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Johannes Schindelin, Junio C Hamano

On Wed, Oct 16, 2019 at 07:07:17AM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 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.

> 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 <johannes.schindelin@gmx.de>
> ---
>  path.c               |  4 ++--
>  t/t1500-rev-parse.sh | 15 +++++++++++++++
>  2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/path.c b/path.c
> index e3da1f3c4e..ff85692b45 100644
> --- a/path.c
> +++ b/path.c
> @@ -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);
>  
> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> index 01abee533d..d318a1eeef 100755
> --- a/t/t1500-rev-parse.sh
> +++ b/t/t1500-rev-parse.sh
> @@ -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 \
> +		--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.

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

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 2/2] git_path(): handle `.lock` files correctly
  2019-10-16 11:04   ` SZEDER Gábor
@ 2019-10-17  7:15     ` Junio C Hamano
  2019-10-17 22:05     ` Johannes Schindelin
  1 sibling, 0 replies; 45+ messages in thread
From: Junio C Hamano @ 2019-10-17  7:15 UTC (permalink / raw)
  To: SZEDER Gábor
  Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin

SZEDER Gábor <szeder.dev@gmail.com> writes:

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

As all worktrees should share the same view of where 'master' (for
example) branch points at, what commit it was pointing at before,
etc., the reflogs should also be shared for refs.  The exception is
the HEAD where each worktree can point at its own commit (when detached)
or a branch (when not).

The above "should" does not mean "I know the code works that way so
if your git behaves differently your compiler is wrong"; it just
means "that's the logical conclusion of the basic design, and if
your git does not behave that way, we have a bug (or two)".

Thanks.

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 2/2] git_path(): handle `.lock` files correctly
  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
  1 sibling, 1 reply; 45+ messages in thread
From: Johannes Schindelin @ 2019-10-17 22:05 UTC (permalink / raw)
  To: SZEDER Gábor
  Cc: Johannes Schindelin via GitGitGadget, git, Junio C Hamano

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

Hi Gábor,

On Wed, 16 Oct 2019, SZEDER Gábor wrote:

> On Wed, Oct 16, 2019 at 07:07:17AM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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'?

Oh wow, this was a left-over from an early draft, before I got the
regression test to work... What I meant was of course logs/HEAD.lock.
Will fix.

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

> > 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 <johannes.schindelin@gmx.de>
> > ---
> >  path.c               |  4 ++--
> >  t/t1500-rev-parse.sh | 15 +++++++++++++++
> >  2 files changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/path.c b/path.c
> > index e3da1f3c4e..ff85692b45 100644
> > --- a/path.c
> > +++ b/path.c
> > @@ -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);
> >
> > diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> > index 01abee533d..d318a1eeef 100755
> > --- a/t/t1500-rev-parse.sh
> > +++ b/t/t1500-rev-parse.sh
> > @@ -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 \
> > +		--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.

Indeed, and this makes this patch much less important than I previosly
thought. It's not like it would break Git GUI in worktrees, which is
what I thought, which in turn is the reason I sent this so close to
-rc0.

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

Thanks,
Dscho

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

^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v2 0/2] Handle git_path() with lock files correctly in worktrees
  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-17 22:07 ` Johannes Schindelin via GitGitGadget
  2019-10-17 22:07   ` [PATCH v2 1/2] t1400: wrap setup code in test case Johannes Schindelin via GitGitGadget
                     ` (2 more replies)
  2 siblings, 3 replies; 45+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-10-17 22:07 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Junio C Hamano

I stumbled over this during my recent work in Git GUI
[https://github.com/gitgitgadget/git/pull/361] that was originally really
only intended to use the correct hooks directory.

It turns out that my fears that index.lock was mishandled were unfounded,
hence this patch series has a lot lower priority for me than "OMG we must
push this into v2.24.0!".

Technically, the first patch is not needed (because I decided against adding
a test to t1400 in the end, in favor of t1500), but it shouldn't hurt,
either.

Changes since v1:

 * Clarified the commit message to state that index.lock is fine, it is 
   logs/HEAD.lock that isn't.

Johannes Schindelin (2):
  t1400: wrap setup code in test case
  git_path(): handle `.lock` files correctly

 path.c                |  4 ++--
 t/t1400-update-ref.sh | 18 ++++++++++--------
 t/t1500-rev-parse.sh  | 15 +++++++++++++++
 3 files changed, 27 insertions(+), 10 deletions(-)


base-commit: 108b97dc372828f0e72e56bbb40cae8e1e83ece6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-401%2Fdscho%2Flock-files-in-worktrees-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-401/dscho/lock-files-in-worktrees-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/401

Range-diff vs v1:

 1:  cf97c5182e = 1:  cf97c5182e t1400: wrap setup code in test case
 2:  f08c90ea02 ! 2:  93dba5a3a3 git_path(): handle `.lock` files correctly
     @@ -3,8 +3,9 @@
          git_path(): handle `.lock` files correctly
      
          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`.
     +    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
     @@ -13,10 +14,12 @@
          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`.
     +    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.
      
     -    So let's make that work as script writers would expect it to.
     +    Side note: Git GUI _does_ ask for `index.lock`, but that is already
     +    resolved correctly.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v2 1/2] t1400: wrap setup code in test case
  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   ` 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-21 21:54   ` [PATCH v3 0/2] Handle git_path() with lock files correctly in worktrees Johannes Schindelin via GitGitGadget
  2 siblings, 0 replies; 45+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-10-17 22:07 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Without this, you cannot use `--run=<...>` to skip that part, and a run
with `--run=0` (which is a common way to determine the test case number
corresponding to a given test case title).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t1400-update-ref.sh | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 1fbd940408..69a7f27311 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -344,14 +344,16 @@ test_expect_success "verifying $m's log (logged by config)" '
 	test_cmp expect .git/logs/$m
 '
 
-git update-ref $m $D
-cat >.git/logs/$m <<EOF
-$Z $C $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 -0500
-$C $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150350 -0500
-$A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 -0500
-$F $Z $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150680 -0500
-$Z $E $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 -0500
-EOF
+test_expect_success 'set up for querying the reflog' '
+	git update-ref $m $D &&
+	cat >.git/logs/$m <<-EOF
+	$Z $C $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 -0500
+	$C $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150350 -0500
+	$A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 -0500
+	$F $Z $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150680 -0500
+	$Z $E $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 -0500
+	EOF
+'
 
 ed="Thu, 26 May 2005 18:32:00 -0500"
 gd="Thu, 26 May 2005 18:33:00 -0500"
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [PATCH v2 2/2] git_path(): handle `.lock` files correctly
  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   ` Johannes Schindelin via GitGitGadget
  2019-10-18  1:23     ` 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
  2 siblings, 1 reply; 45+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-10-17 22:07 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

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.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 path.c               |  4 ++--
 t/t1500-rev-parse.sh | 15 +++++++++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/path.c b/path.c
index e3da1f3c4e..ff85692b45 100644
--- a/path.c
+++ b/path.c
@@ -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);
 
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 01abee533d..d318a1eeef 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -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 \
+		--git-path index --git-path index.lock &&
+	test_cmp expect actual
+'
+
 test_expect_success 'rev-parse --is-shallow-repository in shallow repo' '
 	test_commit test_commit &&
 	echo true >expect &&
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 45+ messages in thread

* Re: [PATCH v2 2/2] git_path(): handle `.lock` files correctly
  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
  0 siblings, 2 replies; 45+ messages in thread
From: Junio C Hamano @ 2019-10-18  1:23 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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.

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

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

Is that s/but/and/?

> diff --git a/path.c b/path.c
> index e3da1f3c4e..ff85692b45 100644
> --- a/path.c
> +++ b/path.c
> @@ -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 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?

Will queue.

Thanks.

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 2/2] git_path(): handle `.lock` files correctly
  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-18 11:42         ` [PATCH 0/2] path.c: minor common_list fixes SZEDER Gábor
  0 siblings, 2 replies; 45+ messages in thread
From: SZEDER Gábor @ 2019-10-18 11:06 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Johannes Schindelin via GitGitGadget, git, Junio C Hamano

On Fri, Oct 18, 2019 at 12:05:20AM +0200, Johannes Schindelin wrote:
> > 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.

Right.  I mistyped it a few times on the command line as well, but
then the command's output reminded me that I messed up.  Alas, no such
reminder when writing the email...

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

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/


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 2/2] git_path(): handle `.lock` files correctly
  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-18 11:42         ` [PATCH 0/2] path.c: minor common_list fixes SZEDER Gábor
  1 sibling, 1 reply; 45+ messages in thread
From: SZEDER Gábor @ 2019-10-18 11:35 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Johannes Schindelin via GitGitGadget, git, Junio C Hamano

On Fri, Oct 18, 2019 at 01:06:18PM +0200, SZEDER Gábor wrote:
> > > 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).
> 
> 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/

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/


diff --git a/path.c b/path.c
index 04b69b9feb..9019169418 100644
--- a/path.c
+++ b/path.c
@@ -335,7 +335,7 @@ static int check_common(const char *unmatched, void *value, void *baton)
 	struct common_dir *dir = value;
 
 	if (!dir)
-		return 0;
+		return -1;
 
 	if (dir->is_dir && (unmatched[0] == 0 || unmatched[0] == '/'))
 		return !dir->exclude;


^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [PATCH 0/2] path.c: minor common_list fixes
  2019-10-18 11:06       ` SZEDER Gábor
  2019-10-18 11:35         ` SZEDER Gábor
@ 2019-10-18 11:42         ` SZEDER Gábor
  2019-10-18 11:42           ` [PATCH 1/2] path.c: fix field name in 'struct common_dir' SZEDER Gábor
                             ` (2 more replies)
  1 sibling, 3 replies; 45+ messages in thread
From: SZEDER Gábor @ 2019-10-18 11:42 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin, git, SZEDER Gábor

On Fri, Oct 18, 2019 at 01:06:18PM +0200, SZEDER Gábor wrote:
> 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


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH 1/2] path.c: fix field name in 'struct common_dir'
  2019-10-18 11:42         ` [PATCH 0/2] path.c: minor common_list fixes SZEDER Gábor
@ 2019-10-18 11:42           ` 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
  2 siblings, 0 replies; 45+ messages in thread
From: SZEDER Gábor @ 2019-10-18 11:42 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin, git, SZEDER Gábor

An array of 'struct common_dir' instances is used to specify whether
various paths in the '.git' directory are worktree-specific or belong
to the main worktree.  Confusingly, the path is recorded in the
struct's 'dirname' field, even though it can be a regular file, e.g.
'gc.pid'.

Rename this 'dirname' field to 'path' to avoid future confusion.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 path.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/path.c b/path.c
index e3da1f3c4e..eeb43e1d25 100644
--- a/path.c
+++ b/path.c
@@ -103,7 +103,7 @@ struct common_dir {
 	unsigned is_dir:1;
 	/* Not common even though its parent is */
 	unsigned exclude:1;
-	const char *dirname;
+	const char *path;
 };
 
 static struct common_dir common_list[] = {
@@ -320,8 +320,8 @@ static void init_common_trie(void)
 	if (common_trie_done_setup)
 		return;
 
-	for (p = common_list; p->dirname; p++)
-		add_to_trie(&common_trie, p->dirname, p);
+	for (p = common_list; p->path; p++)
+		add_to_trie(&common_trie, p->path, p);
 
 	common_trie_done_setup = 1;
 }
@@ -365,8 +365,8 @@ void report_linked_checkout_garbage(void)
 		return;
 	strbuf_addf(&sb, "%s/", get_git_dir());
 	len = sb.len;
-	for (p = common_list; p->dirname; p++) {
-		const char *path = p->dirname;
+	for (p = common_list; p->path; p++) {
+		const char *path = p->path;
 		if (p->ignore_garbage)
 			continue;
 		strbuf_setlen(&sb, len);
-- 
2.23.0.1084.gae250eaa40


^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [PATCH 2/2] path.c: mark 'logs/HEAD' in 'common_list' as file
  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           ` SZEDER Gábor
  2019-10-21 19:35           ` [PATCH 0/2] path.c: minor common_list fixes Johannes Schindelin
  2 siblings, 0 replies; 45+ messages in thread
From: SZEDER Gábor @ 2019-10-18 11:42 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin, git, SZEDER Gábor

'logs/HEAD', i.e. HEAD's reflog, is a file, but its entry in
'common_list' has the 'is_dir' bit set.

Unset that bit to make it consistent with what 'logs/HEAD' is supposed
to be.

This doesn't make a difference in behavior: check_common() is the only
function that looks at the 'is_dir' bit, and that function either
returns 0, or '!exclude', which for 'logs/HEAD' results in 0 as well.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 path.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/path.c b/path.c
index eeb43e1d25..04b69b9feb 100644
--- a/path.c
+++ b/path.c
@@ -113,7 +113,7 @@ static struct common_dir common_list[] = {
 	{ 0, 1, 0, "info" },
 	{ 0, 0, 1, "info/sparse-checkout" },
 	{ 1, 1, 0, "logs" },
-	{ 1, 1, 1, "logs/HEAD" },
+	{ 1, 0, 1, "logs/HEAD" },
 	{ 0, 1, 1, "logs/refs/bisect" },
 	{ 0, 1, 1, "logs/refs/rewritten" },
 	{ 0, 1, 1, "logs/refs/worktree" },
-- 
2.23.0.1084.gae250eaa40


^ permalink raw reply related	[flat|nested] 45+ messages in thread

* Re: [PATCH v2 2/2] git_path(): handle `.lock` files correctly
  2019-10-18  1:23     ` Junio C Hamano
@ 2019-10-18 12:35       ` SZEDER Gábor
  2019-10-21 20:26       ` Johannes Schindelin
  1 sibling, 0 replies; 45+ messages in thread
From: SZEDER Gábor @ 2019-10-18 12:35 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin

On Fri, Oct 18, 2019 at 10:23:00AM +0900, Junio C Hamano wrote:
> > diff --git a/path.c b/path.c
> > index e3da1f3c4e..ff85692b45 100644
> > --- a/path.c
> > +++ b/path.c
> > @@ -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 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?

I was wondering the same.  Despite all of its functions being file
static, this is a fairly general trie implementation for paths, which
could potentially be useful in other parts of Git as well.  Special
casing any suffix within its implementation makes it less (re)usable
in other cases.


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH 0/5] path.c: a couple of common dir/trie fixes
  2019-10-18 11:35         ` SZEDER Gábor
@ 2019-10-21 16:00           ` SZEDER Gábor
  2019-10-21 16:00             ` [PATCH 1/5] Documentation: mention more worktree-specific exceptions SZEDER Gábor
                               ` (4 more replies)
  0 siblings, 5 replies; 45+ messages in thread
From: SZEDER Gábor @ 2019-10-21 16:00 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: David Turner, Johannes Schindelin, git, SZEDER Gábor

On Fri, Oct 18, 2019 at 01:35:57PM +0200, SZEDER Gábor wrote:
> > unfortunately, see two more bugs,

And there are documentation bugs as well, both user-visible (i.e. in
a man page) and in in-code comment.

> > 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/
> 
> 
> diff --git a/path.c b/path.c
> index 04b69b9feb..9019169418 100644
> --- a/path.c
> +++ b/path.c
> @@ -335,7 +335,7 @@ static int check_common(const char *unmatched, void *value, void *baton)
>       struct common_dir *dir = value;
>  
>       if (!dir)
> -             return 0;
> +             return -1;
>  
>       if (dir->is_dir && (unmatched[0] == 0 || unmatched[0] == '/'))
>               return !dir->exclude;

Now I made up mind: this isn't the right fix :)
The proper fix is in the last patch of this series.

Cc-ing David Turner, the trie's author; if I misunderstood anything,
then hopefully he can spot and clarify it.

SZEDER Gábor (5):
  Documentation: mention more worktree-specific exceptions
  path.c: clarify trie_find()'s in-code comment
  path.c: mark 'logs/HEAD' in 'common_list' as file
  path.c: clarify two field names in 'struct common_dir'
  path.c: don't call the match function without value in trie_find()

 Documentation/gitrepository-layout.txt |  10 +-
 path.c                                 | 122 ++++++++++++++-----------
 t/t0060-path-utils.sh                  |   2 +
 3 files changed, 74 insertions(+), 60 deletions(-)

-- 
2.24.0.rc0.472.ga6f06c86b4


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH 1/5] Documentation: mention more worktree-specific exceptions
  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             ` SZEDER Gábor
  2019-10-21 16:00             ` [PATCH 2/5] path.c: clarify trie_find()'s in-code comment SZEDER Gábor
                               ` (3 subsequent siblings)
  4 siblings, 0 replies; 45+ messages in thread
From: SZEDER Gábor @ 2019-10-21 16:00 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: David Turner, Johannes Schindelin, git, SZEDER Gábor

If a directory in $GIT_DIR is overridden when $GIT_COMMON_DIR is set,
then usually all paths within that directory are overridden as well.
There are a couple of exceptions, though, and two of them, namely
'refs/rewritten' and 'logs/HEAD' are not mentioned in
'gitrepository-layout'.  Document them as well.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 Documentation/gitrepository-layout.txt | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/gitrepository-layout.txt b/Documentation/gitrepository-layout.txt
index d6388f10bb..1a2ef4c150 100644
--- a/Documentation/gitrepository-layout.txt
+++ b/Documentation/gitrepository-layout.txt
@@ -96,9 +96,9 @@ refs::
 	directory.  The 'git prune' command knows to preserve
 	objects reachable from refs found in this directory and
 	its subdirectories.
-	This directory is ignored (except refs/bisect and
-	refs/worktree) if $GIT_COMMON_DIR is set and
-	"$GIT_COMMON_DIR/refs" will be used instead.
+	This directory is ignored (except refs/bisect,
+	refs/rewritten and refs/worktree) if $GIT_COMMON_DIR is
+	set and "$GIT_COMMON_DIR/refs" will be used instead.
 
 refs/heads/`name`::
 	records tip-of-the-tree commit objects of branch `name`
@@ -240,8 +240,8 @@ remotes::
 logs::
 	Records of changes made to refs are stored in this directory.
 	See linkgit:git-update-ref[1] for more information. This
-	directory is ignored if $GIT_COMMON_DIR is set and
-	"$GIT_COMMON_DIR/logs" will be used instead.
+	directory is ignored (except logs/HEAD) if $GIT_COMMON_DIR is
+	set and "$GIT_COMMON_DIR/logs" will be used instead.
 
 logs/refs/heads/`name`::
 	Records all changes made to the branch tip named `name`.
-- 
2.24.0.rc0.472.ga6f06c86b4


^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [PATCH 2/5] path.c: clarify trie_find()'s in-code comment
  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             ` SZEDER Gábor
  2019-10-21 16:00             ` [PATCH 3/5] path.c: mark 'logs/HEAD' in 'common_list' as file SZEDER Gábor
                               ` (2 subsequent siblings)
  4 siblings, 0 replies; 45+ messages in thread
From: SZEDER Gábor @ 2019-10-21 16:00 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: David Turner, Johannes Schindelin, git, SZEDER Gábor

A fairly long comment describes trie_find()'s behavior and shows
examples, but it's slightly incomplete/inaccurate.  Update this
comment to specify how trie_find() handles a negative return value
from the given match function.

Furthermore, update the list of examples to include not only two but
three levels of path components.  This makes the examples slightly
more complicated, but it can illustrate the behavior in more corner
cases.

Finally, basically everything refers to the data stored for a key as
"value", with two confusing exceptions:

  - The type definition of the match function calls its corresponding
    parameter 'data'.
    Rename that parameter to 'value'.  (check_common(), the only
    function of this type already calls it 'value').

  - The table of examples above trie_find() has a "val from node"
    column, which has nothing to do with the value stored in the trie:
    it's a "prefix of the key for which the trie contains a value"
    that led to that node.
    Rename that column header to "prefix to node".

Note that neither the original nor the updated description and
examples correspond 100% to the current implementation, because the
implementation is a bit buggy, but the comment describes the desired
behavior.  The bug will be fixed in the last patch of this series.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 path.c | 45 ++++++++++++++++++++++++++++-----------------
 1 file changed, 28 insertions(+), 17 deletions(-)

diff --git a/path.c b/path.c
index e3da1f3c4e..4ac9a101f5 100644
--- a/path.c
+++ b/path.c
@@ -236,30 +236,41 @@ static void *add_to_trie(struct trie *root, const char *key, void *value)
 	return old;
 }
 
-typedef int (*match_fn)(const char *unmatched, void *data, void *baton);
+typedef int (*match_fn)(const char *unmatched, void *value, void *baton);
 
 /*
  * Search a trie for some key.  Find the longest /-or-\0-terminated
- * prefix of the key for which the trie contains a value.  Call fn
- * with the unmatched portion of the key and the found value, and
- * return its return value.  If there is no such prefix, return -1.
+ * prefix of the key for which the trie contains a value.  If there is
+ * no such prefix, return -1.  Otherwise call fn with the unmatched
+ * portion of the key and the found value.  If fn returns 0 or
+ * positive, then return its return value.  If fn returns negative,
+ * then call fn with the next-longest /-terminated prefix of the key
+ * (i.e. a parent directory) for which the trie contains a value, and
+ * handle its return value the same way.  If there is no shorter
+ * /-terminated prefix with a value left, then return the negative
+ * return value of the most recent fn invocation.
  *
  * The key is partially normalized: consecutive slashes are skipped.
  *
- * For example, consider the trie containing only [refs,
- * refs/worktree] (both with values).
- *
- * | key             | unmatched  | val from node | return value |
- * |-----------------|------------|---------------|--------------|
- * | a               | not called | n/a           | -1           |
- * | refs            | \0         | refs          | as per fn    |
- * | refs/           | /          | refs          | as per fn    |
- * | refs/w          | /w         | refs          | as per fn    |
- * | refs/worktree   | \0         | refs/worktree | as per fn    |
- * | refs/worktree/  | /          | refs/worktree | as per fn    |
- * | refs/worktree/a | /a         | refs/worktree | as per fn    |
- * |-----------------|------------|---------------|--------------|
+ * For example, consider the trie containing only [logs,
+ * logs/refs/bisect], both with values, but not logs/refs.
  *
+ * | key                | unmatched      | prefix to node   | return value |
+ * |--------------------|----------------|------------------|--------------|
+ * | a                  | not called     | n/a              | -1           |
+ * | logstore           | not called     | n/a              | -1           |
+ * | logs               | \0             | logs             | as per fn    |
+ * | logs/              | /              | logs             | as per fn    |
+ * | logs/refs          | /refs          | logs             | as per fn    |
+ * | logs/refs/         | /refs/         | logs             | as per fn    |
+ * | logs/refs/b        | /refs/b        | logs             | as per fn    |
+ * | logs/refs/bisected | /refs/bisected | logs             | as per fn    |
+ * | logs/refs/bisect   | \0             | logs/refs/bisect | as per fn    |
+ * | logs/refs/bisect/  | /              | logs/refs/bisect | as per fn    |
+ * | logs/refs/bisect/a | /a             | logs/refs/bisect | as per fn    |
+ * | (If fn in the previous line returns -1, then fn is called once more:) |
+ * | logs/refs/bisect/a | /refs/bisect/a | logs             | as per fn    |
+ * |--------------------|----------------|------------------|--------------|
  */
 static int trie_find(struct trie *root, const char *key, match_fn fn,
 		     void *baton)
-- 
2.24.0.rc0.472.ga6f06c86b4


^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [PATCH 3/5] path.c: mark 'logs/HEAD' in 'common_list' as file
  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             ` 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
  4 siblings, 0 replies; 45+ messages in thread
From: SZEDER Gábor @ 2019-10-21 16:00 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: David Turner, Johannes Schindelin, git, SZEDER Gábor

'logs/HEAD', i.e. HEAD's reflog, is a file, but its entry in
'common_list' has the 'is_dir' bit set.

Unset that bit to make it consistent with what 'logs/HEAD' is supposed
to be.

This doesn't make a difference in behavior: check_common() is the only
function that looks at the 'is_dir' bit, and that function either
returns 0, or '!exclude', which for 'logs/HEAD' results in 0 as well.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 path.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/path.c b/path.c
index 4ac9a101f5..3fac5f5726 100644
--- a/path.c
+++ b/path.c
@@ -113,7 +113,7 @@ static struct common_dir common_list[] = {
 	{ 0, 1, 0, "info" },
 	{ 0, 0, 1, "info/sparse-checkout" },
 	{ 1, 1, 0, "logs" },
-	{ 1, 1, 1, "logs/HEAD" },
+	{ 1, 0, 1, "logs/HEAD" },
 	{ 0, 1, 1, "logs/refs/bisect" },
 	{ 0, 1, 1, "logs/refs/rewritten" },
 	{ 0, 1, 1, "logs/refs/worktree" },
-- 
2.24.0.rc0.472.ga6f06c86b4


^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [PATCH 4/5] path.c: clarify two field names in 'struct common_dir'
  2019-10-21 16:00           ` [PATCH 0/5] path.c: a couple of common dir/trie fixes SZEDER Gábor
                               ` (2 preceding siblings ...)
  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             ` 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
  4 siblings, 0 replies; 45+ messages in thread
From: SZEDER Gábor @ 2019-10-21 16:00 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: David Turner, Johannes Schindelin, git, SZEDER Gábor

An array of 'struct common_dir' instances is used to specify whether
various paths in $GIT_DIR are specific to a worktree, or are common,
i.e. belong to main worktree.  The names of two fields in this
struct are somewhat confusing or ambigious:

  - The path is recorded in the struct's 'dirname' field, even though
    several entries are regular files e.g. 'gc.pid', 'packed-refs',
    etc.

    Rename this field to 'path' to reduce confusion.

  - The field 'exclude' tells whether the path is excluded...  from
    where?  Excluded from the common dir or from the worktree?  It
    means the former, but it's ambigious.

    Rename this field to 'is_common' to make it unambigious what it
    means.  This, however, means the exact opposite of what 'exclude'
    meant, so we have to negate the field's value in all entries as
    well.

The diff is best viewed with '--color-words'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 path.c | 66 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

diff --git a/path.c b/path.c
index 3fac5f5726..cf57bd52dd 100644
--- a/path.c
+++ b/path.c
@@ -101,36 +101,36 @@ struct common_dir {
 	/* Not considered garbage for report_linked_checkout_garbage */
 	unsigned ignore_garbage:1;
 	unsigned is_dir:1;
-	/* Not common even though its parent is */
-	unsigned exclude:1;
-	const char *dirname;
+	/* Belongs to the common dir, though it may contain paths that don't */
+	unsigned is_common:1;
+	const char *path;
 };
 
 static struct common_dir common_list[] = {
-	{ 0, 1, 0, "branches" },
-	{ 0, 1, 0, "common" },
-	{ 0, 1, 0, "hooks" },
-	{ 0, 1, 0, "info" },
-	{ 0, 0, 1, "info/sparse-checkout" },
-	{ 1, 1, 0, "logs" },
-	{ 1, 0, 1, "logs/HEAD" },
-	{ 0, 1, 1, "logs/refs/bisect" },
-	{ 0, 1, 1, "logs/refs/rewritten" },
-	{ 0, 1, 1, "logs/refs/worktree" },
-	{ 0, 1, 0, "lost-found" },
-	{ 0, 1, 0, "objects" },
-	{ 0, 1, 0, "refs" },
-	{ 0, 1, 1, "refs/bisect" },
-	{ 0, 1, 1, "refs/rewritten" },
-	{ 0, 1, 1, "refs/worktree" },
-	{ 0, 1, 0, "remotes" },
-	{ 0, 1, 0, "worktrees" },
-	{ 0, 1, 0, "rr-cache" },
-	{ 0, 1, 0, "svn" },
-	{ 0, 0, 0, "config" },
-	{ 1, 0, 0, "gc.pid" },
-	{ 0, 0, 0, "packed-refs" },
-	{ 0, 0, 0, "shallow" },
+	{ 0, 1, 1, "branches" },
+	{ 0, 1, 1, "common" },
+	{ 0, 1, 1, "hooks" },
+	{ 0, 1, 1, "info" },
+	{ 0, 0, 0, "info/sparse-checkout" },
+	{ 1, 1, 1, "logs" },
+	{ 1, 0, 0, "logs/HEAD" },
+	{ 0, 1, 0, "logs/refs/bisect" },
+	{ 0, 1, 0, "logs/refs/rewritten" },
+	{ 0, 1, 0, "logs/refs/worktree" },
+	{ 0, 1, 1, "lost-found" },
+	{ 0, 1, 1, "objects" },
+	{ 0, 1, 1, "refs" },
+	{ 0, 1, 0, "refs/bisect" },
+	{ 0, 1, 0, "refs/rewritten" },
+	{ 0, 1, 0, "refs/worktree" },
+	{ 0, 1, 1, "remotes" },
+	{ 0, 1, 1, "worktrees" },
+	{ 0, 1, 1, "rr-cache" },
+	{ 0, 1, 1, "svn" },
+	{ 0, 0, 1, "config" },
+	{ 1, 0, 1, "gc.pid" },
+	{ 0, 0, 1, "packed-refs" },
+	{ 0, 0, 1, "shallow" },
 	{ 0, 0, 0, NULL }
 };
 
@@ -331,8 +331,8 @@ static void init_common_trie(void)
 	if (common_trie_done_setup)
 		return;
 
-	for (p = common_list; p->dirname; p++)
-		add_to_trie(&common_trie, p->dirname, p);
+	for (p = common_list; p->path; p++)
+		add_to_trie(&common_trie, p->path, p);
 
 	common_trie_done_setup = 1;
 }
@@ -349,10 +349,10 @@ static int check_common(const char *unmatched, void *value, void *baton)
 		return 0;
 
 	if (dir->is_dir && (unmatched[0] == 0 || unmatched[0] == '/'))
-		return !dir->exclude;
+		return dir->is_common;
 
 	if (!dir->is_dir && unmatched[0] == 0)
-		return !dir->exclude;
+		return dir->is_common;
 
 	return 0;
 }
@@ -376,8 +376,8 @@ void report_linked_checkout_garbage(void)
 		return;
 	strbuf_addf(&sb, "%s/", get_git_dir());
 	len = sb.len;
-	for (p = common_list; p->dirname; p++) {
-		const char *path = p->dirname;
+	for (p = common_list; p->path; p++) {
+		const char *path = p->path;
 		if (p->ignore_garbage)
 			continue;
 		strbuf_setlen(&sb, len);
-- 
2.24.0.rc0.472.ga6f06c86b4


^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [PATCH 5/5] path.c: don't call the match function without value in trie_find()
  2019-10-21 16:00           ` [PATCH 0/5] path.c: a couple of common dir/trie fixes SZEDER Gábor
                               ` (3 preceding siblings ...)
  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             ` SZEDER Gábor
  2019-10-21 17:39               ` David Turner
                                 ` (2 more replies)
  4 siblings, 3 replies; 45+ messages in thread
From: SZEDER Gábor @ 2019-10-21 16:00 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: David Turner, Johannes Schindelin, git, SZEDER Gábor

'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>
---
 path.c                | 11 ++++++-----
 t/t0060-path-utils.sh |  2 ++
 2 files changed, 8 insertions(+), 5 deletions(-)

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


^ permalink raw reply related	[flat|nested] 45+ messages in thread

* Re: [PATCH 5/5] path.c: don't call the match function without value in trie_find()
  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-28 10:57               ` Johannes Schindelin
  2 siblings, 0 replies; 45+ messages in thread
From: David Turner @ 2019-10-21 17:39 UTC (permalink / raw)
  To: SZEDER Gábor, Junio C Hamano; +Cc: Johannes Schindelin, git

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.


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 0/2] path.c: minor common_list fixes
  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           ` Johannes Schindelin
  2 siblings, 0 replies; 45+ messages in thread
From: Johannes Schindelin @ 2019-10-21 19:35 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Junio C Hamano, git

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

Hi Gábor,

On Fri, 18 Oct 2019, SZEDER Gábor wrote:

> On Fri, Oct 18, 2019 at 01:06:18PM +0200, SZEDER Gábor wrote:
> > 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

Those patches look very reasonable to me.

Thanks,
Dscho

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

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v2 2/2] git_path(): handle `.lock` files correctly
  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
  1 sibling, 1 reply; 45+ messages in thread
From: Johannes Schindelin @ 2019-10-21 20:26 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin via GitGitGadget, git

Hi Junio,

On Fri, 18 Oct 2019, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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.
>
> Is this still git_path("index") or is it now HEAD?

Darn. It is "logs/HEAD", of course.

> > Side note: Git GUI _does_ ask for `index.lock`, but that is already
> > resolved correctly.
>
> Is that s/but/and/?

It sounds better with an "and", you're right.

> > diff --git a/path.c b/path.c
> > index e3da1f3c4e..ff85692b45 100644
> > --- a/path.c
> > +++ b/path.c
> > @@ -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 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?

Hmm. The parameter is provided as a `const char *`, so I cannot just
edit it. My first attempt at fixing this resulted in this commit:

-- snip --
    path: switch `trie_find()` to a counted string

    Before this patch, the `trie_find()` function would be fed a regular,
    NUL-terminated string.

    However, in the next commit, we want to strip any `.lock` suffix from
    the argument, and the argument is provided as a `const char *` (i.e. we
    do not want to modify it).

    Let's use a string + length pair of parameters instead of a single
    NUL-terminated string to open the door for this kind of manipulation.

    Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

diff --git a/path.c b/path.c
index e3da1f3c4e2..b18d552c890 100644
--- a/path.c
+++ b/path.c
@@ -236,7 +236,8 @@ static void *add_to_trie(struct trie *root, const char *key, void *value)
 	return old;
 }

-typedef int (*match_fn)(const char *unmatched, void *data, void *baton);
+typedef int (*match_fn)(const char *unmatched, size_t unmatched_len,
+			void *data, void *baton);

 /*
  * Search a trie for some key.  Find the longest /-or-\0-terminated
@@ -261,22 +262,22 @@ typedef int (*match_fn)(const char *unmatched, void *data, void *baton);
  * |-----------------|------------|---------------|--------------|
  *
  */
-static int trie_find(struct trie *root, const char *key, match_fn fn,
-		     void *baton)
+static int trie_find(struct trie *root, const char *key, size_t key_len,
+		     match_fn fn, void *baton)
 {
 	int i;
 	int result;
 	struct trie *child;

-	if (!*key) {
+	if (!key_len) {
 		/* we have reached the end of the key */
 		if (root->value && !root->len)
-			return fn(key, root->value, baton);
+			return fn(key, key_len, root->value, baton);
 		else
 			return -1;
 	}

-	for (i = 0; i < root->len; i++) {
+	for (i = 0; i < key_len && i < root->len; i++) {
 		/* Partial path normalization: skip consecutive slashes. */
 		if (key[i] == '/' && key[i+1] == '/') {
 			key++;
@@ -288,24 +289,26 @@ static int trie_find(struct trie *root, const char *key, match_fn fn,

 	/* Matched the entire compressed section */
 	key += i;
-	if (!*key)
+	key_len -= i;
+
+	if (!key_len)
 		/* End of key */
-		return fn(key, root->value, baton);
+		return fn(key, key_len, root->value, baton);

 	/* Partial path normalization: skip consecutive slashes */
-	while (key[0] == '/' && key[1] == '/')
-		key++;
+	while (key_len > 0 && key[0] == '/' && key[1] == '/')
+		key++, key_len--;

-	child = root->children[(unsigned char)*key];
+	child = root->children[!key_len ? '0' : (unsigned char)*key];
 	if (child)
-		result = trie_find(child, key + 1, fn, baton);
+		result = trie_find(child, key + 1, key_len - 1, fn, baton);
 	else
 		result = -1;

-	if (result >= 0 || (*key != '/' && *key != 0))
+	if (result >= 0 || (key_len && *key != '/'))
 		return result;
 	if (root->value)
-		return fn(key, root->value, baton);
+		return fn(key, key_len, root->value, baton);
 	else
 		return -1;
 }
@@ -330,17 +333,18 @@ static void init_common_trie(void)
  * Helper function for update_common_dir: returns 1 if the dir
  * prefix is common.
  */
-static int check_common(const char *unmatched, void *value, void *baton)
+static int check_common(const char *unmatched, size_t unmatched_len,
+			void *value, void *baton)
 {
 	struct common_dir *dir = value;

 	if (!dir)
 		return 0;

-	if (dir->is_dir && (unmatched[0] == 0 || unmatched[0] == '/'))
+	if (dir->is_dir && (!unmatched_len || unmatched[0] == '/'))
 		return !dir->exclude;

-	if (!dir->is_dir && unmatched[0] == 0)
+	if (!dir->is_dir && !unmatched_len)
 		return !dir->exclude;

 	return 0;
@@ -350,8 +354,10 @@ static void update_common_dir(struct strbuf *buf, int git_dir_len,
 			      const char *common_dir)
 {
 	char *base = buf->buf + git_dir_len;
+	size_t len = strlen(base);
+
 	init_common_trie();
-	if (trie_find(&common_trie, base, check_common, NULL) > 0)
+	if (trie_find(&common_trie, base, len, check_common, NULL) > 0)
 		replace_dir(buf, git_dir_len, common_dir);
 }
-- snap --

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

Ciao,
Dscho

^ permalink raw reply related	[flat|nested] 45+ messages in thread

* Re: [PATCH 5/5] path.c: don't call the match function without value in trie_find()
  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-28 10:57               ` Johannes Schindelin
  2 siblings, 1 reply; 45+ messages in thread
From: SZEDER Gábor @ 2019-10-21 20:57 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: David Turner, Johannes Schindelin, git

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.

^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v3 0/2] Handle git_path() with lock files correctly in worktrees
  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-21 21:54   ` Johannes Schindelin via GitGitGadget
  2019-10-21 21:54     ` [PATCH v3 1/2] t1400: wrap setup code in test case Johannes Schindelin via GitGitGadget
                       ` (2 more replies)
  2 siblings, 3 replies; 45+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-10-21 21:54 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Junio C Hamano

I stumbled over this during my recent work in Git GUI
[https://github.com/gitgitgadget/git/pull/361] that was originally really
only intended to use the correct hooks directory.

It turns out that my fears that index.lock was mishandled were unfounded,
hence this patch series has a lot lower priority for me than "OMG we must
push this into v2.24.0!".

Technically, the first patch is not needed (because I decided against adding
a test to t1400 in the end, in favor of t1500), but it shouldn't hurt,
either.

Changes since v2:

 * Adjusted the commit message to really not talk about index.lock.
 * Instead of modifying the code inside trie_find() to special-case .lock, I
   now copy the string without the suffix .lock (if any) before handing it
   off to trie_find().

Changes since v1:

 * Clarified the commit message to state that index.lock is fine, it is 
   logs/HEAD.lock that isn't.

Johannes Schindelin (2):
  t1400: wrap setup code in test case
  git_path(): handle `.lock` files correctly

 path.c                |  8 +++++++-
 t/t1400-update-ref.sh | 18 ++++++++++--------
 t/t1500-rev-parse.sh  | 15 +++++++++++++++
 3 files changed, 32 insertions(+), 9 deletions(-)


base-commit: 108b97dc372828f0e72e56bbb40cae8e1e83ece6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-401%2Fdscho%2Flock-files-in-worktrees-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-401/dscho/lock-files-in-worktrees-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/401

Range-diff vs v2:

 1:  cf97c5182e = 1:  cf97c5182e t1400: wrap setup code in test case
 2:  93dba5a3a3 ! 2:  8b1f4f089a git_path(): handle `.lock` files correctly
     @@ -4,12 +4,12 @@
      
          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`.
     +    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 `index.lock` file is constructed from
     -    `git_path("index")` by appending the `.lock` suffix.
     +    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
     @@ -19,7 +19,8 @@
          let's be safe rather than sorry.
      
          Side note: Git GUI _does_ ask for `index.lock`, but that is already
     -    resolved correctly.
     +    resolved correctly, due to `update_common_dir()` preferring to leave
     +    unknown paths in the (worktree-specific) git directory.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
     @@ -27,23 +28,23 @@
       --- a/path.c
       +++ b/path.c
      @@
     - 	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);
     -@@
     - 
     - 	/* Matched the entire compressed section */
     - 	key += i;
     --	if (!*key)
     -+	if (!*key || !strcmp(key, ".lock"))
     - 		/* End of key */
     - 		return fn(key, root->value, 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);
     ++
     + 	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)
      
       diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
       --- a/t/t1500-rev-parse.sh

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v3 1/2] t1400: wrap setup code in test case
  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     ` 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-28 12:57     ` [PATCH v4 0/2] Handle git_path() with lock files correctly in worktrees Johannes Schindelin via GitGitGadget
  2 siblings, 0 replies; 45+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-10-21 21:54 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Without this, you cannot use `--run=<...>` to skip that part, and a run
with `--run=0` (which is a common way to determine the test case number
corresponding to a given test case title).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t1400-update-ref.sh | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 1fbd940408..69a7f27311 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -344,14 +344,16 @@ test_expect_success "verifying $m's log (logged by config)" '
 	test_cmp expect .git/logs/$m
 '
 
-git update-ref $m $D
-cat >.git/logs/$m <<EOF
-$Z $C $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 -0500
-$C $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150350 -0500
-$A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 -0500
-$F $Z $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150680 -0500
-$Z $E $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 -0500
-EOF
+test_expect_success 'set up for querying the reflog' '
+	git update-ref $m $D &&
+	cat >.git/logs/$m <<-EOF
+	$Z $C $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 -0500
+	$C $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150350 -0500
+	$A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 -0500
+	$F $Z $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150680 -0500
+	$Z $E $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 -0500
+	EOF
+'
 
 ed="Thu, 26 May 2005 18:32:00 -0500"
 gd="Thu, 26 May 2005 18:33:00 -0500"
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [PATCH v3 2/2] git_path(): handle `.lock` files correctly
  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     ` Johannes Schindelin via GitGitGadget
  2019-10-22 16:01       ` SZEDER Gábor
  2019-10-28 12:57     ` [PATCH v4 0/2] Handle git_path() with lock files correctly in worktrees Johannes Schindelin via GitGitGadget
  2 siblings, 1 reply; 45+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-10-21 21:54 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 path.c               |  8 +++++++-
 t/t1500-rev-parse.sh | 15 +++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/path.c b/path.c
index e3da1f3c4e..5ff64e7a8a 100644
--- a/path.c
+++ b/path.c
@@ -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);
+
 	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)
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 01abee533d..d318a1eeef 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -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 \
+		--git-path index --git-path index.lock &&
+	test_cmp expect actual
+'
+
 test_expect_success 'rev-parse --is-shallow-repository in shallow repo' '
 	test_commit test_commit &&
 	echo true >expect &&
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 2/2] git_path(): handle `.lock` files correctly
  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
  0 siblings, 2 replies; 45+ messages in thread
From: SZEDER Gábor @ 2019-10-22 16:01 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget
  Cc: git, Johannes Schindelin, Junio C Hamano

On Mon, Oct 21, 2019 at 09:54:42PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 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 <johannes.schindelin@gmx.de>
> ---
>  path.c               |  8 +++++++-
>  t/t1500-rev-parse.sh | 15 +++++++++++++++
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/path.c b/path.c
> index e3da1f3c4e..5ff64e7a8a 100644
> --- a/path.c
> +++ b/path.c
> @@ -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);

>  	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)
> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> index 01abee533d..d318a1eeef 100755
> --- a/t/t1500-rev-parse.sh
> +++ b/t/t1500-rev-parse.sh
> @@ -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 \
> +		--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.

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

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v2 2/2] git_path(): handle `.lock` files correctly
  2019-10-21 20:26       ` Johannes Schindelin
@ 2019-10-23  2:12         ` Junio C Hamano
  0 siblings, 0 replies; 45+ messages in thread
From: Junio C Hamano @ 2019-10-23  2:12 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Johannes Schindelin via GitGitGadget, git

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

Sounds like a quite sensible design decision to me.

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 2/2] git_path(): handle `.lock` files correctly
  2019-10-22 16:01       ` SZEDER Gábor
@ 2019-10-23  3:38         ` Junio C Hamano
  2019-10-28 12:01         ` Johannes Schindelin
  1 sibling, 0 replies; 45+ messages in thread
From: Junio C Hamano @ 2019-10-23  3:38 UTC (permalink / raw)
  To: SZEDER Gábor
  Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin

SZEDER Gábor <szeder.dev@gmail.com> writes:

>> +	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);

Makes sense, and is easy to squash in.

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 5/5] path.c: don't call the match function without value in trie_find()
  2019-10-21 20:57               ` SZEDER Gábor
@ 2019-10-23  4:01                 ` Junio C Hamano
  2019-10-23 16:20                   ` SZEDER Gábor
  0 siblings, 1 reply; 45+ messages in thread
From: Junio C Hamano @ 2019-10-23  4:01 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: David Turner, Johannes Schindelin, git

SZEDER Gábor <szeder.dev@gmail.com> 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.




^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 5/5] path.c: don't call the match function without value in trie_find()
  2019-10-23  4:01                 ` Junio C Hamano
@ 2019-10-23 16:20                   ` SZEDER Gábor
  2019-10-24  3:29                     ` Junio C Hamano
  0 siblings, 1 reply; 45+ messages in thread
From: SZEDER Gábor @ 2019-10-23 16:20 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: David Turner, Johannes Schindelin, git

On Wed, Oct 23, 2019 at 01:01:00PM +0900, Junio C Hamano wrote:
> SZEDER Gábor <szeder.dev@gmail.com> 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.


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 5/5] path.c: don't call the match function without value in trie_find()
  2019-10-23 16:20                   ` SZEDER Gábor
@ 2019-10-24  3:29                     ` Junio C Hamano
  0 siblings, 0 replies; 45+ messages in thread
From: Junio C Hamano @ 2019-10-24  3:29 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: David Turner, Johannes Schindelin, git

SZEDER Gábor <szeder.dev@gmail.com> writes:

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

I saw the change to the code as a band-aid, which wouldn't have been
necessary if we had the missing refs/ entry.  Fully populating the
leading levels explicitly may give the application more flexibility
by yielding results different from hardcoded -1 like the patched
code gives.  

But perhaps treating a path that matches a missing intermediate
level just like a path that did not match anything in the trie is
what we want anyway, so I guess the code change was the right thing
(as opposed to a band-aid).

Thanks.


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 5/5] path.c: don't call the match function without value in trie_find()
  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-28 10:57               ` Johannes Schindelin
  2019-10-28 12:00                 ` SZEDER Gábor
  2 siblings, 1 reply; 45+ messages in thread
From: Johannes Schindelin @ 2019-10-28 10:57 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Junio C Hamano, David Turner, git

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

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 5/5] path.c: don't call the match function without value in trie_find()
  2019-10-28 10:57               ` Johannes Schindelin
@ 2019-10-28 12:00                 ` SZEDER Gábor
  2019-10-28 21:30                   ` Johannes Schindelin
  0 siblings, 1 reply; 45+ messages in thread
From: SZEDER Gábor @ 2019-10-28 12:00 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Junio C Hamano, David Turner, git

On Mon, Oct 28, 2019 at 11:57:10AM +0100, Johannes Schindelin wrote:
> >   - 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:
> 
> 
> > 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".

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


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 2/2] git_path(): handle `.lock` files correctly
  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
  1 sibling, 2 replies; 45+ messages in thread
From: Johannes Schindelin @ 2019-10-28 12:01 UTC (permalink / raw)
  To: SZEDER Gábor
  Cc: Johannes Schindelin via GitGitGadget, git, Junio C Hamano

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

Hi,

On Tue, 22 Oct 2019, SZEDER Gábor wrote:

> On Mon, Oct 21, 2019 at 09:54:42PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > 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 <johannes.schindelin@gmx.de>
> > ---
> >  path.c               |  8 +++++++-
> >  t/t1500-rev-parse.sh | 15 +++++++++++++++
> >  2 files changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/path.c b/path.c
> > index e3da1f3c4e..5ff64e7a8a 100644
> > --- a/path.c
> > +++ b/path.c
> > @@ -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);

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


>
> >  	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)
> > diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> > index 01abee533d..d318a1eeef 100755
> > --- a/t/t1500-rev-parse.sh
> > +++ b/t/t1500-rev-parse.sh
> > @@ -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 \
> > +		--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.

Excellent point. Thank you for helping me improve the patch!

Ciao,
Dscho

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

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 2/2] git_path(): handle `.lock` files correctly
  2019-10-28 12:01         ` Johannes Schindelin
@ 2019-10-28 12:32           ` SZEDER Gábor
  2019-10-28 17:30           ` Junio C Hamano
  1 sibling, 0 replies; 45+ messages in thread
From: SZEDER Gábor @ 2019-10-28 12:32 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Johannes Schindelin via GitGitGadget, git, Junio C Hamano

On Mon, Oct 28, 2019 at 01:01:49PM +0100, Johannes Schindelin wrote:
> > > @@ -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);
> 
> 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,

Do we?  You're right, we do indeed, because in replace_dir(buf, ...)
we call strbuf_splice(buf, ...).
OK, then your suggestion below is even better.

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

^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v4 0/2] Handle git_path() with lock files correctly in worktrees
  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-28 12:57     ` Johannes Schindelin via GitGitGadget
  2019-10-28 12:57       ` [PATCH v4 1/2] t1400: wrap setup code in test case Johannes Schindelin via GitGitGadget
                         ` (2 more replies)
  2 siblings, 3 replies; 45+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-10-28 12:57 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Junio C Hamano

I stumbled over this during my recent work in Git GUI
[https://github.com/gitgitgadget/git/pull/361] that was originally really
only intended to use the correct hooks directory.

It turns out that my fears that index.lock was mishandled were unfounded,
hence this patch series has a lot lower priority for me than "OMG we must
push this into v2.24.0!".

Technically, the first patch is not needed (because I decided against adding
a test to t1400 in the end, in favor of t1500), but it shouldn't hurt,
either.

I verified locally that this patch pair does not conflict with 
sg/dir-trie-fixes.

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.

Changes since v2:

 * Adjusted the commit message to really not talk about index.lock.
 * Instead of modifying the code inside trie_find() to special-case .lock, I
   now copy the string without the suffix .lock (if any) before handing it
   off to trie_find().

Changes since v1:

 * Clarified the commit message to state that index.lock is fine, it is 
   logs/HEAD.lock that isn't.

Johannes Schindelin (2):
  t1400: wrap setup code in test case
  git_path(): handle `.lock` files correctly

 path.c                |  6 ++++++
 t/t0060-path-utils.sh |  2 ++
 t/t1400-update-ref.sh | 18 ++++++++++--------
 3 files changed, 18 insertions(+), 8 deletions(-)


base-commit: 108b97dc372828f0e72e56bbb40cae8e1e83ece6
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-401%2Fdscho%2Flock-files-in-worktrees-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-401/dscho/lock-files-in-worktrees-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/401

Range-diff vs v3:

 1:  cf97c5182e = 1:  cf97c5182e t1400: wrap setup code in test case
 2:  8b1f4f089a ! 2:  d98810875e git_path(): handle `.lock` files correctly
     @@ -28,46 +28,40 @@
       --- a/path.c
       +++ b/path.c
      @@
     - static void update_common_dir(struct strbuf *buf, int git_dir_len,
     + #include "path.h"
     + #include "packfile.h"
     + #include "object-store.h"
     ++#include "lockfile.h"
     + 
     + static int get_st_mode_bits(const char *path, int *mode)
     + {
     +@@
       			      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);
     + 	char *base = buf->buf + git_dir_len;
     ++	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);
      +
     -+	free(base2);
     ++	if (has_lock_suffix)
     ++		strbuf_addstr(buf, LOCK_SUFFIX);
       }
       
       void report_linked_checkout_garbage(void)
      
     - diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
     - --- a/t/t1500-rev-parse.sh
     - +++ b/t/t1500-rev-parse.sh
     + diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
     + --- a/t/t0060-path-utils.sh
     + +++ b/t/t0060-path-utils.sh
      @@
     - 	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 \
     -+		--git-path index --git-path index.lock &&
     -+	test_cmp expect actual
     -+'
     -+
     - test_expect_success 'rev-parse --is-shallow-repository in shallow repo' '
     - 	test_commit test_commit &&
     - 	echo true >expect &&
     + test_git_path GIT_OBJECT_DIRECTORY=foo objects2 .git/objects2
     + test_expect_success 'setup common repository' 'git --git-dir=bar init'
     + test_git_path GIT_COMMON_DIR=bar index                    .git/index
     ++test_git_path GIT_COMMON_DIR=bar index.lock               .git/index.lock
     + 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/HEAD.lock           .git/logs/HEAD.lock
     + 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/bisec/foo      bar/logs/refs/bisec/foo
     + test_git_path GIT_COMMON_DIR=bar logs/refs/bisec          bar/logs/refs/bisec

-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v4 1/2] t1400: wrap setup code in test case
  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       ` 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
  2 siblings, 0 replies; 45+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-10-28 12:57 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

Without this, you cannot use `--run=<...>` to skip that part, and a run
with `--run=0` (which is a common way to determine the test case number
corresponding to a given test case title).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t1400-update-ref.sh | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 1fbd940408..69a7f27311 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -344,14 +344,16 @@ test_expect_success "verifying $m's log (logged by config)" '
 	test_cmp expect .git/logs/$m
 '
 
-git update-ref $m $D
-cat >.git/logs/$m <<EOF
-$Z $C $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 -0500
-$C $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150350 -0500
-$A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 -0500
-$F $Z $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150680 -0500
-$Z $E $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 -0500
-EOF
+test_expect_success 'set up for querying the reflog' '
+	git update-ref $m $D &&
+	cat >.git/logs/$m <<-EOF
+	$Z $C $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150320 -0500
+	$C $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150350 -0500
+	$A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 -0500
+	$F $Z $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150680 -0500
+	$Z $E $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 -0500
+	EOF
+'
 
 ed="Thu, 26 May 2005 18:32:00 -0500"
 gd="Thu, 26 May 2005 18:33:00 -0500"
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 45+ messages in thread

* [PATCH v4 2/2] git_path(): handle `.lock` files correctly
  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       ` 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
  2 siblings, 0 replies; 45+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2019-10-28 12:57 UTC (permalink / raw)
  To: git; +Cc: Johannes Schindelin, Junio C Hamano, Johannes Schindelin

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 path.c                | 6 ++++++
 t/t0060-path-utils.sh | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/path.c b/path.c
index e3da1f3c4e..198a867017 100644
--- a/path.c
+++ b/path.c
@@ -11,6 +11,7 @@
 #include "path.h"
 #include "packfile.h"
 #include "object-store.h"
+#include "lockfile.h"
 
 static int get_st_mode_bits(const char *path, int *mode)
 {
@@ -350,9 +351,14 @@ static void update_common_dir(struct strbuf *buf, int git_dir_len,
 			      const char *common_dir)
 {
 	char *base = buf->buf + git_dir_len;
+	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);
 }
 
 void report_linked_checkout_garbage(void)
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index c7b53e494b..2aca8ccff9 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -285,8 +285,10 @@ test_git_path GIT_OBJECT_DIRECTORY=foo objects/foo foo/foo
 test_git_path GIT_OBJECT_DIRECTORY=foo objects2 .git/objects2
 test_expect_success 'setup common repository' 'git --git-dir=bar init'
 test_git_path GIT_COMMON_DIR=bar index                    .git/index
+test_git_path GIT_COMMON_DIR=bar index.lock               .git/index.lock
 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/HEAD.lock           .git/logs/HEAD.lock
 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/bisec/foo      bar/logs/refs/bisec/foo
 test_git_path GIT_COMMON_DIR=bar logs/refs/bisec          bar/logs/refs/bisec
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 2/2] git_path(): handle `.lock` files correctly
  2019-10-28 12:01         ` Johannes Schindelin
  2019-10-28 12:32           ` SZEDER Gábor
@ 2019-10-28 17:30           ` Junio C Hamano
  1 sibling, 0 replies; 45+ messages in thread
From: Junio C Hamano @ 2019-10-28 17:30 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: SZEDER Gábor, Johannes Schindelin via GitGitGadget, git

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

Makes sense.  Thanks for thinking it through.

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 5/5] path.c: don't call the match function without value in trie_find()
  2019-10-28 12:00                 ` SZEDER Gábor
@ 2019-10-28 21:30                   ` Johannes Schindelin
  0 siblings, 0 replies; 45+ messages in thread
From: Johannes Schindelin @ 2019-10-28 21:30 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Junio C Hamano, David Turner, git

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

Hi Gábor,

On Mon, 28 Oct 2019, SZEDER Gábor wrote:

> On Mon, Oct 28, 2019 at 11:57:10AM +0100, Johannes Schindelin wrote:
> > >   - 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:
> >
> >
> > > 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".
>
> The checks at the other two of those three callsites look like this,
> and I just followed suit for the sake of consistency.

Oh, okay. Sorry for the noise, then.

Thanks,
Dscho

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v4 0/2] Handle git_path() with lock files correctly in worktrees
  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       ` Junio C Hamano
  2 siblings, 0 replies; 45+ messages in thread
From: Junio C Hamano @ 2019-10-29  3:39 UTC (permalink / raw)
  To: Johannes Schindelin via GitGitGadget; +Cc: git, Johannes Schindelin

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

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

Makes sense.  Will replace.

Thanks.

^ permalink raw reply	[flat|nested] 45+ messages in thread

end of thread, other threads:[~2019-10-29  3:39 UTC | newest]

Thread overview: 45+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.