Re: [PATCH 2/2] git_path(): handle `.lock` files correctly

[SZEDER Gábor]

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;

Re: [PATCH 2/2] git_path(): handle `.lock` files correctly

[SZEDER Gábor]

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/

Re: [PATCH 2/2] git_path(): handle `.lock` files correctly

[Johannes Schindelin]

Hi Gábor,

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

> On Wed, Oct 16, 2019 at 07:07:17AM +, Johannes Schindelin via 
> GitGitGadget wrote:
> > From: Johannes Schindelin 
> >
> > 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 
> > ---
> >  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 +
>   +++ actual  2019-10-16 10:20:31.051229519 +
>   @@ -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

Re: [PATCH 2/2] git_path(): handle `.lock` files correctly

[Junio C Hamano]

SZEDER Gábor  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.

Re: [PATCH 2/2] git_path(): handle `.lock` files correctly

[SZEDER Gábor]

On Wed, Oct 16, 2019 at 07:07:17AM +, Johannes Schindelin via GitGitGadget 
wrote:
> From: Johannes Schindelin 
> 
> 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 
> ---
>  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 +
  +++ actual  2019-10-16 10:20:31.051229519 +
  @@ -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