"add worktree" fails with "fatal: Invalid path" error

"add worktree" fails with "fatal: Invalid path" error

[Shaheed Haque]

Hi,

I'm running git v.2.20.1 on Ubuntu from a program which follows the pattern:

============
1. create a temporary directory /tmp/tmpabc
2. in a loop:
    2a. create a second level of temporary directory /tmp/tmpabc/tmpworktree123
    2b. use "git worktree add" on the second level directory
    2c. do something
3. cleanup
    3b. "git branch -D" on each basename(second level directory)
    3a. "git worktree prune"
============

The loop size is of the order of 8-20. In step 2b, I often get errors
like this (from a Bash reproducer):

============
$ git worktree add /tmp/tmpgtxug4y9/git_worktree.gBGqnfnU
Preparing worktree (new branch 'git_worktree.gBGqnfnU')
fatal: Invalid path '/tmp/tmp1q9ysvyl': No such file or directory
============

I can see that the problematic path exists in the "gitdir" file of
what must be an earlier worktree from an older run (the branch is
gone, but the tree is still there). The path appear to relate to the
older run's first level directory:

============
$ grep -r /tmp/tmp1q9ysvyl ../.git/worktrees/
../.git/worktrees/git_worktree.frcwtjt_/gitdir:/tmp/tmp1q9ysvyl/git_worktree.frcwtjt_/.git
$ git worktree list
...
/tmp/tmp1q9ysvyl/git_worktree.frcwtjt_  edde3f25 (detached HEAD)
...
$ git branch | grep frcwtjt_
<no matches>
============

NOTE: I've not yet had to try deleting the worktree, since "add
worktree" does appear to work some of the time, so I am able to limp
along.

I have these questions:

1. There is no branch or first level directory, but "git prune" has
not deleted the worktree, is this expected?
2. Is there something wrong with the sequence of steps I am following?

Thanks, Shaheed

P.S. I have an strace of a failing worktree add if needed.

Re: "add worktree" fails with "fatal: Invalid path" error

[Duy Nguyen]

On Sun, May 12, 2019 at 5:14 PM Shaheed Haque <shaheedhaque@gmail.com> wrote:
>
> Hi,
>
> I'm running git v.2.20.1 on Ubuntu from a program which follows the pattern:
>
> ============
> 1. create a temporary directory /tmp/tmpabc

When is this directory deleted? After step 3a?

> 2. in a loop:
>     2a. create a second level of temporary directory /tmp/tmpabc/tmpworktree123
>     2b. use "git worktree add" on the second level directory
>     2c. do something
> 3. cleanup
>     3b. "git branch -D" on each basename(second level directory)
>     3a. "git worktree prune"
> ============
>
> The loop size is of the order of 8-20. In step 2b, I often get errors
> like this (from a Bash reproducer):
>
> ============
> $ git worktree add /tmp/tmpgtxug4y9/git_worktree.gBGqnfnU
> Preparing worktree (new branch 'git_worktree.gBGqnfnU')
> fatal: Invalid path '/tmp/tmp1q9ysvyl': No such file or directory
> ============
>
> I can see that the problematic path exists in the "gitdir" file of
> what must be an earlier worktree from an older run (the branch is
> gone, but the tree is still there). The path appear to relate to the
> older run's first level directory:
>
> ============
> $ grep -r /tmp/tmp1q9ysvyl ../.git/worktrees/
> ../.git/worktrees/git_worktree.frcwtjt_/gitdir:/tmp/tmp1q9ysvyl/git_worktree.frcwtjt_/.git
> $ git worktree list
> ...
> /tmp/tmp1q9ysvyl/git_worktree.frcwtjt_  edde3f25 (detached HEAD)
> ...
> $ git branch | grep frcwtjt_
> <no matches>
> ============

Yeah I think I know where that "Invalid path" comes from and it should
not be there (at least it should not be a fatal error). I'll need to
reproduce this first. But I'm certain you've given me enough
information to do so.

> NOTE: I've not yet had to try deleting the worktree, since "add
> worktree" does appear to work some of the time, so I am able to limp
> along.

It's probably best to stay clean and delete things after you're done.
At least you should be able to avoid this problem this way until it's
fixed.

>
> I have these questions:
>
> 1. There is no branch or first level directory, but "git prune" has
> not deleted the worktree, is this expected?

I assume you meant "git worktree prune", not "git prune". See
gc.worktreePruneExpire. Dead worktree info stays for a while until
it's deleted, so that you can recover stuff if you need to.

> 2. Is there something wrong with the sequence of steps I am following?

Nope. I mean, you could try "git worktree remove" to be on the safe
side. But it should work even without that. To me this looks very much
like a bug.

> Thanks, Shaheed
>
> P.S. I have an strace of a failing worktree add if needed.
-- 
Duy

Re: "add worktree" fails with "fatal: Invalid path" error

[Shaheed Haque]

Hi Duy,

On Mon, 13 May 2019 at 10:21, Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Sun, May 12, 2019 at 5:14 PM Shaheed Haque <shaheedhaque@gmail.com> wrote:
> >
> > Hi,
> >
> > I'm running git v.2.20.1 on Ubuntu from a program which follows the pattern:
> >
> > ============
> > 1. create a temporary directory /tmp/tmpabc
>
> When is this directory deleted? After step 3a?

Yes. I note that I screwed up the numbering in my note - the order is
["git branch -D", "git worktree prune", delete directory].

> > 2. in a loop:
> >     2a. create a second level of temporary directory /tmp/tmpabc/tmpworktree123
> >     2b. use "git worktree add" on the second level directory
> >     2c. do something
> > 3. cleanup
> >     3b. "git branch -D" on each basename(second level directory)
> >     3a. "git worktree prune"
> > ============
> >
> > The loop size is of the order of 8-20. In step 2b, I often get errors
> > like this (from a Bash reproducer):
> >
> > ============
> > $ git worktree add /tmp/tmpgtxug4y9/git_worktree.gBGqnfnU
> > Preparing worktree (new branch 'git_worktree.gBGqnfnU')
> > fatal: Invalid path '/tmp/tmp1q9ysvyl': No such file or directory
> > ============
> >
> > I can see that the problematic path exists in the "gitdir" file of
> > what must be an earlier worktree from an older run (the branch is
> > gone, but the tree is still there). The path appear to relate to the
> > older run's first level directory:
> >
> > ============
> > $ grep -r /tmp/tmp1q9ysvyl ../.git/worktrees/
> > ../.git/worktrees/git_worktree.frcwtjt_/gitdir:/tmp/tmp1q9ysvyl/git_worktree.frcwtjt_/.git
> > $ git worktree list
> > ...
> > /tmp/tmp1q9ysvyl/git_worktree.frcwtjt_  edde3f25 (detached HEAD)
> > ...
> > $ git branch | grep frcwtjt_
> > <no matches>
> > ============
>
> Yeah I think I know where that "Invalid path" comes from and it should
> not be there (at least it should not be a fatal error). I'll need to
> reproduce this first. But I'm certain you've given me enough
> information to do so.
>
> > NOTE: I've not yet had to try deleting the worktree, since "add
> > worktree" does appear to work some of the time, so I am able to limp
> > along.
>
> It's probably best to stay clean and delete things after you're done.
> At least you should be able to avoid this problem this way until it's
> fixed.

Ack.

> > I have these questions:
> >
> > 1. There is no branch or first level directory, but "git prune" has
> > not deleted the worktree, is this expected?
>
> I assume you meant "git worktree prune", not "git prune". See
> gc.worktreePruneExpire. Dead worktree info stays for a while until
> it's deleted, so that you can recover stuff if you need to.

Yes, sorry for typo.

> > 2. Is there something wrong with the sequence of steps I am following?
>
> Nope. I mean, you could try "git worktree remove" to be on the safe
> side. But it should work even without that. To me this looks very much
> like a bug.

The original code used the more obvious "git worktree remove" rather
than "git worktree prune" but I switched partly because remove seemed
slow (I cannot now quantify what caused me to think that), and partly
because I was having other issues which, I now realise, you probably
addressed in your recent "stat versus mkdir race" change.

BTW: I *love* worktrees. I have used/wrapped/developed multiple source
control systems over the years, and IMHO, this is one of the killer
features of git.

Thanks, Shaheed

> > Thanks, Shaheed
> >
> > P.S. I have an strace of a failing worktree add if needed.
> --
> Duy

Re: "add worktree" fails with "fatal: Invalid path" error

[Duy Nguyen]

On Mon, May 13, 2019 at 7:55 PM Shaheed Haque <shaheedhaque@gmail.com> wrote:
> The original code used the more obvious "git worktree remove" rather
> than "git worktree prune" but I switched partly because remove seemed
> slow (I cannot now quantify what caused me to think that), and partly
> because I was having other issues which, I now realise, you probably
> addressed in your recent "stat versus mkdir race" change.

It should be as slow as "git status; rm -r". The first command _could_
be slow. But if you find it significantly slower than that, I will be
glad to receive another bug report.
-- 
Duy

Re: "add worktree" fails with "fatal: Invalid path" error

[Shaheed Haque]

On Tue, 14 May 2019 at 13:34, Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Mon, May 13, 2019 at 7:55 PM Shaheed Haque <shaheedhaque@gmail.com> wrote:
> > The original code used the more obvious "git worktree remove" rather
> > than "git worktree prune" but I switched partly because remove seemed
> > slow (I cannot now quantify what caused me to think that), and partly
> > because I was having other issues which, I now realise, you probably
> > addressed in your recent "stat versus mkdir race" change.
>
> It should be as slow as "git status; rm -r". The first command _could_
> be slow. But if you find it significantly slower than that, I will be
> glad to receive another bug report.

After I wrote, I went back and checked, and I have no idea why I
thought it slow. It seems just fine (by the time I have spawned out of
Python and all), and so I have switched the code back to
(synchronous/predictable) "git worktree remove".

Thanks, Shaheed

> --
> Duy

[PATCH] worktree add: be tolerant of corrupt worktrees

[Nguyễn Thái Ngọc Duy]

find_worktree() can die() unexpectedly because it uses real_path()
instead of the gentler version. When it's used in 'git worktree add' [1]
and there's a bad worktree, this die() could prevent people from adding
new worktrees.

The "bad" condition to trigger this is when a parent of the worktree's
location is deleted. Then real_path() will complain.

Use the other version so that bad worktrees won't affect 'worktree
add'. The bad ones will eventually be pruned, we just have to tolerate
them for a bit.

[1] added in cb56f55c16 (worktree: disallow adding same path multiple
    times, 2018-08-28), or since v2.20.0. Though the real bug in
    find_worktree() is much older.

Reported-by: Shaheed Haque <shaheedhaque@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t2025-worktree-add.sh | 12 ++++++++++++
 worktree.c              |  7 +++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index 286bba35d8..d83a9f0fdc 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -570,4 +570,16 @@ test_expect_success '"add" an existing locked but missing worktree' '
 	git worktree add --force --force --detach gnoo
 '
 
+test_expect_success '"add" should not fail because of another bad worktree' '
+	git init add-fail &&
+	(
+		cd add-fail &&
+		test_commit first &&
+		mkdir sub &&
+		git worktree add sub/to-be-deleted &&
+		rm -rf sub &&
+		git worktree add second
+	)
+'
+
 test_done
diff --git a/worktree.c b/worktree.c
index d6a0ee7f73..c79b3e42bb 100644
--- a/worktree.c
+++ b/worktree.c
@@ -222,9 +222,12 @@ struct worktree *find_worktree(struct worktree **list,
 		free(to_free);
 		return NULL;
 	}
-	for (; *list; list++)
-		if (!fspathcmp(path, real_path((*list)->path)))
+	for (; *list; list++) {
+		const char *wt_path = real_path_if_valid((*list)->path);
+
+		if (wt_path && !fspathcmp(path, wt_path))
 			break;
+	}
 	free(path);
 	free(to_free);
 	return *list;
-- 
2.21.0.1141.gd54ac2cb17

Re: [PATCH] worktree add: be tolerant of corrupt worktrees

[Shaheed Haque]

Hi Nguyễn,

Thanks for the quick response. While I leave the code to the experts,
I can confirm that restoring the missing directory (but no content in
it) does allow "worktree add" to function again.

One point may be worth clarifying...

On Mon, 13 May 2019 at 11:50, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>
> find_worktree() can die() unexpectedly because it uses real_path()
> instead of the gentler version. When it's used in 'git worktree add' [1]
> and there's a bad worktree, this die() could prevent people from adding
> new worktrees.
>
> The "bad" condition to trigger this is when a parent of the worktree's
> location is deleted. Then real_path() will complain.
>
> Use the other version so that bad worktrees won't affect 'worktree
> add'. The bad ones will eventually be pruned, we just have to tolerate
> them for a bit.

...as I mentioned, from my experiments, trying a "worktree prune" did
NOT resolve the issue for me. But since I don't know the logic that
prune uses, there may have been some other reason for this.

Thanks again, Shaheed

> [1] added in cb56f55c16 (worktree: disallow adding same path multiple
>     times, 2018-08-28), or since v2.20.0. Though the real bug in
>     find_worktree() is much older.
>
> Reported-by: Shaheed Haque <shaheedhaque@gmail.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  t/t2025-worktree-add.sh | 12 ++++++++++++
>  worktree.c              |  7 +++++--
>  2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> index 286bba35d8..d83a9f0fdc 100755
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -570,4 +570,16 @@ test_expect_success '"add" an existing locked but missing worktree' '
>         git worktree add --force --force --detach gnoo
>  '
>
> +test_expect_success '"add" should not fail because of another bad worktree' '
> +       git init add-fail &&
> +       (
> +               cd add-fail &&
> +               test_commit first &&
> +               mkdir sub &&
> +               git worktree add sub/to-be-deleted &&
> +               rm -rf sub &&
> +               git worktree add second
> +       )
> +'
> +
>  test_done
> diff --git a/worktree.c b/worktree.c
> index d6a0ee7f73..c79b3e42bb 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -222,9 +222,12 @@ struct worktree *find_worktree(struct worktree **list,
>                 free(to_free);
>                 return NULL;
>         }
> -       for (; *list; list++)
> -               if (!fspathcmp(path, real_path((*list)->path)))
> +       for (; *list; list++) {
> +               const char *wt_path = real_path_if_valid((*list)->path);
> +
> +               if (wt_path && !fspathcmp(path, wt_path))
>                         break;
> +       }
>         free(path);
>         free(to_free);
>         return *list;
> --
> 2.21.0.1141.gd54ac2cb17
>

Re: [PATCH] worktree add: be tolerant of corrupt worktrees

[Eric Sunshine]

On Mon, May 13, 2019 at 6:50 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> find_worktree() can die() unexpectedly because it uses real_path()
> instead of the gentler version. When it's used in 'git worktree add' [1]
> and there's a bad worktree, this die() could prevent people from adding
> new worktrees.

This is good to know because, to fix [1], I think we'll want to add a
new function[2] akin to find_worktree(), but without magic suffix
matching (that is, just literal absolute path comparison).

[1]: https://public-inbox.org/git/0308570E-AAA3-43B8-A592-F4DA9760DBED@synopsys.com/
[2]: https://public-inbox.org/git/CAPig+cQh8hxeoVjLHDKhAcZVQPpPT5v0AUY8gsL9=qfJ7z-L2A@mail.gmail.com/

> The "bad" condition to trigger this is when a parent of the worktree's
> location is deleted. Then real_path() will complain.
>
> Use the other version so that bad worktrees won't affect 'worktree
> add'. The bad ones will eventually be pruned, we just have to tolerate
> them for a bit.

The patch itself makes sense, though, as Shaheed noted in his
response, pruning seems to get short-circuited somehow under this
situation; perhaps that needs its own fix, but certainly shouldn't
hold up this fix.

> Reported-by: Shaheed Haque <shaheedhaque@gmail.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Re: [PATCH] worktree add: be tolerant of corrupt worktrees

[Duy Nguyen]

On Fri, May 17, 2019 at 2:46 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Mon, May 13, 2019 at 6:50 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> > find_worktree() can die() unexpectedly because it uses real_path()
> > instead of the gentler version. When it's used in 'git worktree add' [1]
> > and there's a bad worktree, this die() could prevent people from adding
> > new worktrees.
>
> This is good to know because, to fix [1], I think we'll want to add a
> new function[2] akin to find_worktree(), but without magic suffix
> matching (that is, just literal absolute path comparison).

Yeah. find_worktree() was made to handle command line options from
worktree's move/remove, it's probably a bit too magical for this case.

I still want to store relative path in "gitdir" files at some point,
which would complicate the last "absolute path comparison" part a bit.
But it should be manageable.

> [1]: https://public-inbox.org/git/0308570E-AAA3-43B8-A592-F4DA9760DBED@synopsys.com/
> [2]: https://public-inbox.org/git/CAPig+cQh8hxeoVjLHDKhAcZVQPpPT5v0AUY8gsL9=qfJ7z-L2A@mail.gmail.com/
>
> > The "bad" condition to trigger this is when a parent of the worktree's
> > location is deleted. Then real_path() will complain.
> >
> > Use the other version so that bad worktrees won't affect 'worktree
> > add'. The bad ones will eventually be pruned, we just have to tolerate
> > them for a bit.
>
> The patch itself makes sense, though, as Shaheed noted in his
> response, pruning seems to get short-circuited somehow under this
> situation; perhaps that needs its own fix, but certainly shouldn't
> hold up this fix.

I might have missed that detail. Thanks for pointing out. Will get another look.
-- 
Duy

Re: [PATCH] worktree add: be tolerant of corrupt worktrees

[Shaheed Haque]

On Sat, 18 May 2019 at 12:50, Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Fri, May 17, 2019 at 2:46 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> >
> > On Mon, May 13, 2019 at 6:50 AM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> > > find_worktree() can die() unexpectedly because it uses real_path()
> > > instead of the gentler version. When it's used in 'git worktree add' [1]
> > > and there's a bad worktree, this die() could prevent people from adding
> > > new worktrees.
> >
> > This is good to know because, to fix [1], I think we'll want to add a
> > new function[2] akin to find_worktree(), but without magic suffix
> > matching (that is, just literal absolute path comparison).
>
> Yeah. find_worktree() was made to handle command line options from
> worktree's move/remove, it's probably a bit too magical for this case.
>
> I still want to store relative path in "gitdir" files at some point,
> which would complicate the last "absolute path comparison" part a bit.
> But it should be manageable.
>
> > [1]: https://public-inbox.org/git/0308570E-AAA3-43B8-A592-F4DA9760DBED@synopsys.com/
> > [2]: https://public-inbox.org/git/CAPig+cQh8hxeoVjLHDKhAcZVQPpPT5v0AUY8gsL9=qfJ7z-L2A@mail.gmail.com/
> >
> > > The "bad" condition to trigger this is when a parent of the worktree's
> > > location is deleted. Then real_path() will complain.
> > >
> > > Use the other version so that bad worktrees won't affect 'worktree
> > > add'. The bad ones will eventually be pruned, we just have to tolerate
> > > them for a bit.
> >
> > The patch itself makes sense, though, as Shaheed noted in his
> > response, pruning seems to get short-circuited somehow under this
> > situation; perhaps that needs its own fix, but certainly shouldn't
> > hold up this fix.
>
> I might have missed that detail. Thanks for pointing out. Will get another look.

On the off-chance this is not obvious to you experts, 'worktree
remove' also hits this issue, as you can see from this little example
I just caught:

$ git worktree remove --force /tmp/tmp56t59s2k/git_worktree.pa44kgs0
fatal: Invalid path '/tmp/tmpy4f98pwj': No such file or directory
$ mkdir /tmp/tmpy4f98pwj
$ git worktree remove --force /tmp/tmp56t59s2k/git_worktree.pa44kgs0
fatal: Invalid path '/tmp/tmp_q3p2mon': No such file or directory
$ mkdir /tmp/tmp_q3p2mon
...
several more suppressed
...
$ git worktree remove --force /tmp/tmp56t59s2k/git_worktree.pa44kgs0
fatal: '/tmp/tmp56t59s2k/git_worktree.pa44kgs0' is not a working tree

Thanks, Shaheed

>
> --
> Duy