Regression: submodule worktrees can clobber core.worktree config

Regression: submodule worktrees can clobber core.worktree config

[Tomasz Śniatowski]

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

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

[Duy Nguyen]

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.

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.

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



-- 
Duy

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

[Tomasz Śniatowski]

On Wed, 9 Jan 2019 at 00:23, Duy Nguyen <pclouds@gmail.com> wrote:
>
> 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

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

[Duy Nguyen]

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

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

[Stefan Beller]

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.

Stefan

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

[Tomasz Śniatowski]

On Wed, 9 Jan 2019 at 18:42, Stefan Beller <sbeller@google.com> wrote:
>
> 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

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

[Stefan Beller]

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


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

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

[Duy Nguyen]

On Fri, Jan 11, 2019 at 3:07 AM Stefan Beller <sbeller@google.com> wrote:
>
> > 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