threads / discuss / 50184

Regression: submodule worktrees can clobber core.worktree config

Subject: Regression: submodule worktrees can clobber core.worktree config

## tl;dr

8 messages between Jan 8, 2019 and Jan 11, 2019.

replies: 7people: 3as markdown or json

Tomasz Śniatowski· Jan 8, 2019, 22:16 UTC · lore
After upgrading to 2.20.1 I noticed in some submodule+worktree scenarios git
will break the submodule configuration. Reproducible with:
    git init a && (cd a; touch a; git add a; git commit -ma)
    git init b && (cd b; git submodule add ../a; git commit -mb)
    git -C b worktree add ../b2
    git -C b/a worktree add ../../b2/a
    git -C b status
    git -C b2 submodule update
    git -C b status
The submodule update in the _worktree_ puts an invalid core.worktree value in
the _original_ repository submodule config (b/.git/modules/a/config), causing
the last git status to error out with:
    fatal: cannot chdir to '../../../../../../b2/a': No such file or directory
    fatal: 'git status --porcelain=2' failed in submodule a
Looking at the config file itself, the submodule update operation applies the
following change (the new path is invalid):
    -       worktree = ../../../a
    +       worktree = ../../../../../../b2/a

This worked fine on 2.19.2 (no config change, no error), and was useful to have a worktree with (large) submodules that are also worktrees.

Bisects down to: 74d4731da1 submodule--helper: replace connect-gitdir-workingtree by ensure-core-worktree

-- Tomasz Śniatowski

Duy Nguyen· Jan 8, 2019, 23:22 UTC · re: Tomasz Śniatowski · lore

Re: Regression: submodule worktrees can clobber core.worktree config

On Wed, Jan 9, 2019 at 5:56 AM Tomasz Śniatowski <tsniatowski@vewd.com> wrote:
Show 24 quoted lines
>
> After upgrading to 2.20.1 I noticed in some submodule+worktree scenarios git
> will break the submodule configuration. Reproducible with:
>     git init a && (cd a; touch a; git add a; git commit -ma)
>     git init b && (cd b; git submodule add ../a; git commit -mb)
>     git -C b worktree add ../b2
>     git -C b/a worktree add ../../b2/a
>     git -C b status
>     git -C b2 submodule update
>     git -C b status
>
> The submodule update in the _worktree_ puts an invalid core.worktree value in
> the _original_ repository submodule config (b/.git/modules/a/config), causing
> the last git status to error out with:
>     fatal: cannot chdir to '../../../../../../b2/a': No such file or directory
>     fatal: 'git status --porcelain=2' failed in submodule a
>
> Looking at the config file itself, the submodule update operation applies the
> following change (the new path is invalid):
>     -       worktree = ../../../a
>     +       worktree = ../../../../../../b2/a
>
> This worked fine on 2.19.2 (no config change, no error), and was useful to have
> a worktree with (large) submodules that are also worktrees.

This scenario is not supported (or at least known to be broken in theory) so I wouldn't call this a regression even if it happens to work on 2.19.2 for some reason.

The good news is, I have something that should make it work reliably. But I don't know if it will make it to 2.21 or not.

Show 6 quoted lines
> Bisects down to:
> 74d4731da1 submodule--helper: replace connect-gitdir-workingtree by
> ensure-core-worktree
>
> --
> Tomasz Śniatowski
-- 
Duy
Tomasz Śniatowski· Jan 9, 2019, 06:40 UTC · re: Duy Nguyen · lore

Re: Regression: submodule worktrees can clobber core.worktree config

On Wed, 9 Jan 2019 at 00:23, Duy Nguyen <pclouds@gmail.com> wrote:
Show 30 quoted lines
>
> On Wed, Jan 9, 2019 at 5:56 AM Tomasz Śniatowski <tsniatowski@vewd.com> wrote:
> >
> > After upgrading to 2.20.1 I noticed in some submodule+worktree scenarios git
> > will break the submodule configuration. Reproducible with:
> >     git init a && (cd a; touch a; git add a; git commit -ma)
> >     git init b && (cd b; git submodule add ../a; git commit -mb)
> >     git -C b worktree add ../b2
> >     git -C b/a worktree add ../../b2/a
> >     git -C b status
> >     git -C b2 submodule update
> >     git -C b status
> >
> > The submodule update in the _worktree_ puts an invalid core.worktree value in
> > the _original_ repository submodule config (b/.git/modules/a/config), causing
> > the last git status to error out with:
> >     fatal: cannot chdir to '../../../../../../b2/a': No such file or directory
> >     fatal: 'git status --porcelain=2' failed in submodule a
> >
> > Looking at the config file itself, the submodule update operation applies the
> > following change (the new path is invalid):
> >     -       worktree = ../../../a
> >     +       worktree = ../../../../../../b2/a
> >
> > This worked fine on 2.19.2 (no config change, no error), and was useful to have
> > a worktree with (large) submodules that are also worktrees.
>
> This scenario is not supported (or at least known to be broken in
> theory) so I wouldn't call this a regression even if it happens to
> work on 2.19.2 for some reason.

This scenario worked fine for quite a while now, at least since around 2.15 (as I started using this early 2018). It "just worked", to be honest, just needed the worktree submodules to be manually set up as worktrees too.

> The good news is, I have something that should make it work reliably.
> But I don't know if it will make it to 2.21 or not.
That's good to hear, is there something I can try out or track?

-- Tomasz Śniatowski

Duy Nguyen· Jan 9, 2019, 09:12 UTC · re: Tomasz Śniatowski · lore

Re: Regression: submodule worktrees can clobber core.worktree config

On Wed, Jan 9, 2019 at 1:40 PM Tomasz Śniatowski <tsniatowski@vewd.com> wrote:
> > The good news is, I have something that should make it work reliably.
> > But I don't know if it will make it to 2.21 or not.
>
> That's good to hear, is there something I can try out or track?
You can try this
https://gitlab.com/pclouds/git/commits/submodules-in-worktrees

which should support multiple worktrees in either submodules or supermodules. Be careful though, that branch is only reviewed by me and I'm not a heavy submodule user to give it more day-to-day testing.

For tracking, if you want I can CC you when I send these patches here for review. Otherwise you can check Junio's "what's cooking" mails from time to time and search "submodule".

-- 
Duy
Stefan Beller· Jan 9, 2019, 17:42 UTC · re: Tomasz Śniatowski · lore

Re: Regression: submodule worktrees can clobber core.worktree config

On Tue, Jan 8, 2019 at 2:16 PM Tomasz Śniatowski <tsniatowski@vewd.com> wrote:
Show 24 quoted lines
>
> After upgrading to 2.20.1 I noticed in some submodule+worktree scenarios git
> will break the submodule configuration. Reproducible with:
>     git init a && (cd a; touch a; git add a; git commit -ma)
>     git init b && (cd b; git submodule add ../a; git commit -mb)
>     git -C b worktree add ../b2
>     git -C b/a worktree add ../../b2/a
>     git -C b status
>     git -C b2 submodule update
>     git -C b status
>
> The submodule update in the _worktree_ puts an invalid core.worktree value in
> the _original_ repository submodule config (b/.git/modules/a/config), causing
> the last git status to error out with:
>     fatal: cannot chdir to '../../../../../../b2/a': No such file or directory
>     fatal: 'git status --porcelain=2' failed in submodule a
>
> Looking at the config file itself, the submodule update operation applies the
> following change (the new path is invalid):
>     -       worktree = ../../../a
>     +       worktree = ../../../../../../b2/a
>
> This worked fine on 2.19.2 (no config change, no error), and was useful to have
> a worktree with (large) submodules that are also worktrees.
Thanks for reporting the issue!
>
> Bisects down to:
> 74d4731da1 submodule--helper: replace connect-gitdir-workingtree by
> ensure-core-worktree
So this would need to update the worktree config, not the generic config.
We'd need to replace the line
    cfg_file = repo_git_path(&subrepo, "config");
in builtin/submodule--helper.c::ensure_core_worktree()
to be a worktree specific call.

Or the other way round we'd want to make repo_git_path to be worktree specific and introduce repo_common_path for the main working tree.

Looking at Duys tree, https://gitlab.com/pclouds/git/commit/94751ada7c32eb6fb2c67dd7723161d1955a5683 is pretty much what we need.

Reverting that topic that introduced this (4d6d6e, Merge branch 'sb/submodule-update-in-c'), might be possible but that would conflict with another followup that fixes issues in that series (see sb/submodule-unset-core-worktree-when-worktree-is-lost https://github.com/gitster/git/commits/sb/submodule-unset-core-worktree-when-worktree-is-lost) so I'd rather just cherry-pick the commit from Duy.

Stefan
Tomasz Śniatowski· Jan 9, 2019, 23:57 UTC · re: Stefan Beller · lore

Re: Regression: submodule worktrees can clobber core.worktree config

On Wed, 9 Jan 2019 at 18:42, Stefan Beller <sbeller@google.com> wrote:
Show 56 quoted lines
>
> On Tue, Jan 8, 2019 at 2:16 PM Tomasz Śniatowski <tsniatowski@vewd.com> wrote:
> >
> > After upgrading to 2.20.1 I noticed in some submodule+worktree scenarios git
> > will break the submodule configuration. Reproducible with:
> >     git init a && (cd a; touch a; git add a; git commit -ma)
> >     git init b && (cd b; git submodule add ../a; git commit -mb)
> >     git -C b worktree add ../b2
> >     git -C b/a worktree add ../../b2/a
> >     git -C b status
> >     git -C b2 submodule update
> >     git -C b status
> >
> > The submodule update in the _worktree_ puts an invalid core.worktree value in
> > the _original_ repository submodule config (b/.git/modules/a/config), causing
> > the last git status to error out with:
> >     fatal: cannot chdir to '../../../../../../b2/a': No such file or directory
> >     fatal: 'git status --porcelain=2' failed in submodule a
> >
> > Looking at the config file itself, the submodule update operation applies the
> > following change (the new path is invalid):
> >     -       worktree = ../../../a
> >     +       worktree = ../../../../../../b2/a
> >
> > This worked fine on 2.19.2 (no config change, no error), and was useful to have
> > a worktree with (large) submodules that are also worktrees.
>
> Thanks for reporting the issue!
>
> >
> > Bisects down to:
> > 74d4731da1 submodule--helper: replace connect-gitdir-workingtree by
> > ensure-core-worktree
>
> So this would need to update the worktree config, not the generic config.
>
> We'd need to replace the line
>     cfg_file = repo_git_path(&subrepo, "config");
> in builtin/submodule--helper.c::ensure_core_worktree()
> to be a worktree specific call.
>
> Or the other way round we'd want to make repo_git_path to
> be worktree specific and introduce repo_common_path for
> the main working tree.
>
> Looking at Duys tree,
> https://gitlab.com/pclouds/git/commit/94751ada7c32eb6fb2c67dd7723161d1955a5683
> is pretty much what we need.
>
> Reverting that topic that introduced this (4d6d6e,
> Merge branch 'sb/submodule-update-in-c'), might be possible but
> that would conflict with another followup that fixes issues in
> that series
> (see sb/submodule-unset-core-worktree-when-worktree-is-lost
> https://github.com/gitster/git/commits/sb/submodule-unset-core-worktree-when-worktree-is-lost)
> so I'd rather just cherry-pick the commit from Duy.

I had a look at https://gitlab.com/pclouds/git/commits/submodules-in-worktrees, and it doesn't seem to be quite all okay.

The submodule update step of the repro (that breaks the config on 2.20) emits
an error message instead, and leaves the config unchanged:
   git -C b2 submodule update
   fatal: could not set 'core.worktree' to '../../../../../../b2/a'
It looks a bit like it's still trying to do the wrong thing, but errors out
during the attempt (repo_config_set_worktree_gently returns false).

Curiously, even though it says "fatal", it will then perform the actual submodule update if it's required.

Same behavior on master with a subset of that branch cherry-picked, that is: https://gitlab.com/pclouds/git/commit/94751ada7c32eb6fb2c67dd7723161d1955a5683 along with two others it needed to build: https://gitlab.com/pclouds/git/commit/d26ab4c5013f6117814161be3e87c8d2b73561a4 https://gitlab.com/pclouds/git/commit/b2e21eece6b35e00707ed3a8377a84a95da6b778

-- Tomasz Śniatowski

Stefan Beller· Jan 10, 2019, 20:07 UTC · re: Tomasz Śniatowski · lore

Re: Regression: submodule worktrees can clobber core.worktree config

Show 9 quoted lines
> I had a look at https://gitlab.com/pclouds/git/commits/submodules-in-worktrees,
> and it doesn't seem to be quite all okay.
>
> The submodule update step of the repro (that breaks the config on 2.20) emits
> an error message instead, and leaves the config unchanged:
>    git -C b2 submodule update
>    fatal: could not set 'core.worktree' to '../../../../../../b2/a'
> It looks a bit like it's still trying to do the wrong thing, but errors out
> during the attempt (repo_config_set_worktree_gently returns false).

There is more than just that. After adding the worktrees, (and after the first status call)

    $ cat b2/.git
gitdir: /u/git/t/trash directory.t7419-submodule-worktrees/b/.git/worktrees/b2
    $ cat b2/a/.git
gitdir: /u/git/t/trash
directory.t7419-submodule-worktrees/b/.git/modules/a/worktrees/a

Are worktrees using absolute path for their gitlinks? Submodules themselves try really hard to use relative path:

    $ cat b/a/.git
gitdir: ../.git/modules/a
> Curiously, even though it says "fatal", it will then perform the actual
> submodule update if it's required.

Oh. :/ I think we should solve that by either warning (but that gives bad UX) or actually aborting, by adding a "|| exit 1" in git-submodule.sh in cmd_update where we call "git submodule--helper ensure-core-worktree".

When we run "git -C b2 submodule update", it calls "git submodule--helper ensure-core-worktree a" which currently would make sure that b2/a/.git points to b2/.git/modules/a, but that is not the case as b2 and b2/a are worktrees, whose git directories are housed in b/.git/worktrees.

So maybe we need to be a bit more careful and check if b2/a/.git resolves to a worktree and if so we'd not touch it at all (and warn about it?).

Show 9 quoted lines
>
> Same behavior on master with a subset of that branch cherry-picked, that is:
> https://gitlab.com/pclouds/git/commit/94751ada7c32eb6fb2c67dd7723161d1955a5683
> along with two others it needed to build:
> https://gitlab.com/pclouds/git/commit/d26ab4c5013f6117814161be3e87c8d2b73561a4
> https://gitlab.com/pclouds/git/commit/b2e21eece6b35e00707ed3a8377a84a95da6b778
>
> --
> Tomasz Śniatowski
Duy Nguyen· Jan 11, 2019, 00:07 UTC · re: Stefan Beller · lore

Re: Regression: submodule worktrees can clobber core.worktree config

On Fri, Jan 11, 2019 at 3:07 AM Stefan Beller <sbeller@google.com> wrote:
Show 21 quoted lines
>
> > I had a look at https://gitlab.com/pclouds/git/commits/submodules-in-worktrees,
> > and it doesn't seem to be quite all okay.
> >
> > The submodule update step of the repro (that breaks the config on 2.20) emits
> > an error message instead, and leaves the config unchanged:
> >    git -C b2 submodule update
> >    fatal: could not set 'core.worktree' to '../../../../../../b2/a'
> > It looks a bit like it's still trying to do the wrong thing, but errors out
> > during the attempt (repo_config_set_worktree_gently returns false).
>
> There is more than just that. After adding the worktrees,
> (and after the first status call)
>
>     $ cat b2/.git
> gitdir: /u/git/t/trash directory.t7419-submodule-worktrees/b/.git/worktrees/b2
>     $ cat b2/a/.git
> gitdir: /u/git/t/trash
> directory.t7419-submodule-worktrees/b/.git/modules/a/worktrees/a
>
> Are worktrees using absolute path for their gitlinks?

Yes. Moving to relative paths is on my todo list and I probably should get to it after I'm mostly done (*) with submodule support

(*) Sharing submodule repos between worktrees is still something not addressed.
-- 
Duy

← back to recent threads