git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
* Regression: submodule worktrees can clobber core.worktree config
@ 2019-01-08 22:16 Tomasz Śniatowski
  2019-01-08 23:22 ` Duy Nguyen
  2019-01-09 17:42 ` Stefan Beller
  0 siblings, 2 replies; 8+ messages in thread
From: Tomasz Śniatowski @ 2019-01-08 22:16 UTC (permalink / raw)
  To: git

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Regression: submodule worktrees can clobber core.worktree config
  2019-01-08 22:16 Regression: submodule worktrees can clobber core.worktree config Tomasz Śniatowski
@ 2019-01-08 23:22 ` Duy Nguyen
  2019-01-09  6:40   ` Tomasz Śniatowski
  2019-01-09 17:42 ` Stefan Beller
  1 sibling, 1 reply; 8+ messages in thread
From: Duy Nguyen @ 2019-01-08 23:22 UTC (permalink / raw)
  To: Tomasz Śniatowski; +Cc: Git Mailing List

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Regression: submodule worktrees can clobber core.worktree config
  2019-01-08 23:22 ` Duy Nguyen
@ 2019-01-09  6:40   ` Tomasz Śniatowski
  2019-01-09  9:12     ` Duy Nguyen
  0 siblings, 1 reply; 8+ messages in thread
From: Tomasz Śniatowski @ 2019-01-09  6:40 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Git Mailing List

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Regression: submodule worktrees can clobber core.worktree config
  2019-01-09  6:40   ` Tomasz Śniatowski
@ 2019-01-09  9:12     ` Duy Nguyen
  0 siblings, 0 replies; 8+ messages in thread
From: Duy Nguyen @ 2019-01-09  9:12 UTC (permalink / raw)
  To: Tomasz Śniatowski; +Cc: Git Mailing List

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Regression: submodule worktrees can clobber core.worktree config
  2019-01-08 22:16 Regression: submodule worktrees can clobber core.worktree config Tomasz Śniatowski
  2019-01-08 23:22 ` Duy Nguyen
@ 2019-01-09 17:42 ` Stefan Beller
  2019-01-09 23:57   ` Tomasz Śniatowski
  1 sibling, 1 reply; 8+ messages in thread
From: Stefan Beller @ 2019-01-09 17:42 UTC (permalink / raw)
  To: Tomasz Śniatowski, Duy Nguyen; +Cc: git

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Regression: submodule worktrees can clobber core.worktree config
  2019-01-09 17:42 ` Stefan Beller
@ 2019-01-09 23:57   ` Tomasz Śniatowski
  2019-01-10 20:07     ` Stefan Beller
  0 siblings, 1 reply; 8+ messages in thread
From: Tomasz Śniatowski @ 2019-01-09 23:57 UTC (permalink / raw)
  To: Stefan Beller; +Cc: Duy Nguyen, git

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Regression: submodule worktrees can clobber core.worktree config
  2019-01-09 23:57   ` Tomasz Śniatowski
@ 2019-01-10 20:07     ` Stefan Beller
  2019-01-11  0:07       ` Duy Nguyen
  0 siblings, 1 reply; 8+ messages in thread
From: Stefan Beller @ 2019-01-10 20:07 UTC (permalink / raw)
  To: Tomasz Śniatowski; +Cc: Duy Nguyen, git

> 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Regression: submodule worktrees can clobber core.worktree config
  2019-01-10 20:07     ` Stefan Beller
@ 2019-01-11  0:07       ` Duy Nguyen
  0 siblings, 0 replies; 8+ messages in thread
From: Duy Nguyen @ 2019-01-11  0:07 UTC (permalink / raw)
  To: Stefan Beller; +Cc: Tomasz Śniatowski, git

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-08 22:16 Regression: submodule worktrees can clobber core.worktree config Tomasz Śniatowski
2019-01-08 23:22 ` Duy Nguyen
2019-01-09  6:40   ` Tomasz Śniatowski
2019-01-09  9:12     ` Duy Nguyen
2019-01-09 17:42 ` Stefan Beller
2019-01-09 23:57   ` Tomasz Śniatowski
2019-01-10 20:07     ` Stefan Beller
2019-01-11  0:07       ` Duy Nguyen

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox