worktree add: be tolerant of corrupt worktrees
diff mbox series

Message ID 20190513104944.20367-1-pclouds@gmail.com
State New
Headers show
Series
  • worktree add: be tolerant of corrupt worktrees
Related show

Commit Message

Duy Nguyen May 13, 2019, 10:49 a.m. UTC
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(-)

Comments

Shaheed Haque May 13, 2019, 12:42 p.m. UTC | #1
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
>
Eric Sunshine May 17, 2019, 7:46 a.m. UTC | #2
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>
Duy Nguyen May 18, 2019, 11:49 a.m. UTC | #3
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.
Shaheed Haque June 1, 2019, 7:29 p.m. UTC | #4
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

Patch
diff mbox series

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;