{"thread":{"id":"50184","subject":"Regression: submodule worktrees can clobber core.worktree config","startedAt":"2019-01-08T22:16:19Z","lastAt":"2019-01-11T00:07:33Z","messageCount":8,"participants":["Tomasz Śniatowski","Duy Nguyen","Stefan Beller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"366380","messageId":"CAG0vfyQeA3Hm7AsYgYtP4v-Yg0=rKXW0YYfg_emAwEscZha4VA@mail.gmail.com","threadId":"50184","inReplyTo":null,"subject":"Regression: submodule worktrees can clobber core.worktree config","fromName":"Tomasz Śniatowski","fromEmail":"tsniatowski@vewd.com","sentAt":"2019-01-08T22:16:06Z","receivedAt":"2019-01-08T22:16:19Z","isPatch":false,"sender":{"key":"tsniatowski@vewd.com","avatar":null},"body":"After upgrading to 2.20.1 I noticed in some submodule+worktree scenarios git\nwill break the submodule configuration. Reproducible with:\n    git init a && (cd a; touch a; git add a; git commit -ma)\n    git init b && (cd b; git submodule add ../a; git commit -mb)\n    git -C b worktree add ../b2\n    git -C b/a worktree add ../../b2/a\n    git -C b status\n    git -C b2 submodule update\n    git -C b status\n\nThe submodule update in the _worktree_ puts an invalid core.worktree value in\nthe _original_ repository submodule config (b/.git/modules/a/config), causing\nthe last git status to error out with:\n    fatal: cannot chdir to '../../../../../../b2/a': No such file or directory\n    fatal: 'git status --porcelain=2' failed in submodule a\n\nLooking at the config file itself, the submodule update operation applies the\nfollowing change (the new path is invalid):\n    -       worktree = ../../../a\n    +       worktree = ../../../../../../b2/a\n\nThis worked fine on 2.19.2 (no config change, no error), and was useful to have\na worktree with (large) submodules that are also worktrees.\n\nBisects down to:\n74d4731da1 submodule--helper: replace connect-gitdir-workingtree by\nensure-core-worktree\n\n--\nTomasz Śniatowski\n"},{"id":"366384","messageId":"CACsJy8Cvc8v_4OEmpgKPWSO5csV6jRya7mnSQjEs4mMhHRq4AQ@mail.gmail.com","threadId":"50184","inReplyTo":"CAG0vfyQeA3Hm7AsYgYtP4v-Yg0=rKXW0YYfg_emAwEscZha4VA@mail.gmail.com","subject":"Re: Regression: submodule worktrees can clobber core.worktree config","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-08T23:22:38Z","receivedAt":"2019-01-08T23:23:06Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jan 9, 2019 at 5:56 AM Tomasz Śniatowski <tsniatowski@vewd.com> wrote:\n>\n> After upgrading to 2.20.1 I noticed in some submodule+worktree scenarios git\n> will break the submodule configuration. Reproducible with:\n>     git init a && (cd a; touch a; git add a; git commit -ma)\n>     git init b && (cd b; git submodule add ../a; git commit -mb)\n>     git -C b worktree add ../b2\n>     git -C b/a worktree add ../../b2/a\n>     git -C b status\n>     git -C b2 submodule update\n>     git -C b status\n>\n> The submodule update in the _worktree_ puts an invalid core.worktree value in\n> the _original_ repository submodule config (b/.git/modules/a/config), causing\n> the last git status to error out with:\n>     fatal: cannot chdir to '../../../../../../b2/a': No such file or directory\n>     fatal: 'git status --porcelain=2' failed in submodule a\n>\n> Looking at the config file itself, the submodule update operation applies the\n> following change (the new path is invalid):\n>     -       worktree = ../../../a\n>     +       worktree = ../../../../../../b2/a\n>\n> This worked fine on 2.19.2 (no config change, no error), and was useful to have\n> a worktree with (large) submodules that are also worktrees.\n\nThis scenario is not supported (or at least known to be broken in\ntheory) so I wouldn't call this a regression even if it happens to\nwork on 2.19.2 for some reason.\n\nThe good news is, I have something that should make it work reliably.\nBut I don't know if it will make it to 2.21 or not.\n\n> Bisects down to:\n> 74d4731da1 submodule--helper: replace connect-gitdir-workingtree by\n> ensure-core-worktree\n>\n> --\n> Tomasz Śniatowski\n\n\n\n-- \nDuy\n"},{"id":"366398","messageId":"CAG0vfyTdAyEeAuNUpjTrMjUpmT0XNx1ffdbQwYS3fs13UFnP6w@mail.gmail.com","threadId":"50184","inReplyTo":"CACsJy8Cvc8v_4OEmpgKPWSO5csV6jRya7mnSQjEs4mMhHRq4AQ@mail.gmail.com","subject":"Re: Regression: submodule worktrees can clobber core.worktree config","fromName":"Tomasz Śniatowski","fromEmail":"tsniatowski@vewd.com","sentAt":"2019-01-09T06:40:04Z","receivedAt":"2019-01-09T06:42:14Z","isPatch":false,"sender":{"key":"tsniatowski@vewd.com","avatar":null},"body":"On Wed, 9 Jan 2019 at 00:23, Duy Nguyen <pclouds@gmail.com> wrote:\n>\n> On Wed, Jan 9, 2019 at 5:56 AM Tomasz Śniatowski <tsniatowski@vewd.com> wrote:\n> >\n> > After upgrading to 2.20.1 I noticed in some submodule+worktree scenarios git\n> > will break the submodule configuration. Reproducible with:\n> >     git init a && (cd a; touch a; git add a; git commit -ma)\n> >     git init b && (cd b; git submodule add ../a; git commit -mb)\n> >     git -C b worktree add ../b2\n> >     git -C b/a worktree add ../../b2/a\n> >     git -C b status\n> >     git -C b2 submodule update\n> >     git -C b status\n> >\n> > The submodule update in the _worktree_ puts an invalid core.worktree value in\n> > the _original_ repository submodule config (b/.git/modules/a/config), causing\n> > the last git status to error out with:\n> >     fatal: cannot chdir to '../../../../../../b2/a': No such file or directory\n> >     fatal: 'git status --porcelain=2' failed in submodule a\n> >\n> > Looking at the config file itself, the submodule update operation applies the\n> > following change (the new path is invalid):\n> >     -       worktree = ../../../a\n> >     +       worktree = ../../../../../../b2/a\n> >\n> > This worked fine on 2.19.2 (no config change, no error), and was useful to have\n> > a worktree with (large) submodules that are also worktrees.\n>\n> This scenario is not supported (or at least known to be broken in\n> theory) so I wouldn't call this a regression even if it happens to\n> work on 2.19.2 for some reason.\n\nThis scenario worked fine for quite a while now, at least since around 2.15\n(as I started using this early 2018). It \"just worked\", to be honest, just\nneeded the worktree submodules to be manually set up as worktrees too.\n\n> The good news is, I have something that should make it work reliably.\n> But I don't know if it will make it to 2.21 or not.\n\nThat's good to hear, is there something I can try out or track?\n\n--\nTomasz Śniatowski\n"},{"id":"366402","messageId":"CACsJy8B=XyGaDS4XA7fhBqfnWAORJhhKc2giiOf+bm9fWJgoUQ@mail.gmail.com","threadId":"50184","inReplyTo":"CAG0vfyTdAyEeAuNUpjTrMjUpmT0XNx1ffdbQwYS3fs13UFnP6w@mail.gmail.com","subject":"Re: Regression: submodule worktrees can clobber core.worktree config","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-09T09:12:46Z","receivedAt":"2019-01-09T09:13:14Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jan 9, 2019 at 1:40 PM Tomasz Śniatowski <tsniatowski@vewd.com> wrote:\n> > The good news is, I have something that should make it work reliably.\n> > But I don't know if it will make it to 2.21 or not.\n>\n> That's good to hear, is there something I can try out or track?\n\nYou can try this\n\nhttps://gitlab.com/pclouds/git/commits/submodules-in-worktrees\n\nwhich should support multiple worktrees in either submodules or\nsupermodules. Be careful though, that branch is only reviewed by me\nand I'm not a heavy submodule user to give it more day-to-day testing.\n\nFor tracking, if you want I can CC you when I send these patches here\nfor review. Otherwise you can check Junio's \"what's cooking\" mails\nfrom time to time and search \"submodule\".\n-- \nDuy\n"},{"id":"366430","messageId":"CAGZ79kZBwocC=UzjW+DxodwJkQZ2mNMYNjsk6sL4SCqdhGoQ7w@mail.gmail.com","threadId":"50184","inReplyTo":"CAG0vfyQeA3Hm7AsYgYtP4v-Yg0=rKXW0YYfg_emAwEscZha4VA@mail.gmail.com","subject":"Re: Regression: submodule worktrees can clobber core.worktree config","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2019-01-09T17:42:14Z","receivedAt":"2019-01-09T17:42:29Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jan 8, 2019 at 2:16 PM Tomasz Śniatowski <tsniatowski@vewd.com> wrote:\n>\n> After upgrading to 2.20.1 I noticed in some submodule+worktree scenarios git\n> will break the submodule configuration. Reproducible with:\n>     git init a && (cd a; touch a; git add a; git commit -ma)\n>     git init b && (cd b; git submodule add ../a; git commit -mb)\n>     git -C b worktree add ../b2\n>     git -C b/a worktree add ../../b2/a\n>     git -C b status\n>     git -C b2 submodule update\n>     git -C b status\n>\n> The submodule update in the _worktree_ puts an invalid core.worktree value in\n> the _original_ repository submodule config (b/.git/modules/a/config), causing\n> the last git status to error out with:\n>     fatal: cannot chdir to '../../../../../../b2/a': No such file or directory\n>     fatal: 'git status --porcelain=2' failed in submodule a\n>\n> Looking at the config file itself, the submodule update operation applies the\n> following change (the new path is invalid):\n>     -       worktree = ../../../a\n>     +       worktree = ../../../../../../b2/a\n>\n> This worked fine on 2.19.2 (no config change, no error), and was useful to have\n> a worktree with (large) submodules that are also worktrees.\n\nThanks for reporting the issue!\n\n>\n> Bisects down to:\n> 74d4731da1 submodule--helper: replace connect-gitdir-workingtree by\n> ensure-core-worktree\n\nSo this would need to update the worktree config, not the generic config.\n\nWe'd need to replace the line\n    cfg_file = repo_git_path(&subrepo, \"config\");\nin builtin/submodule--helper.c::ensure_core_worktree()\nto be a worktree specific call.\n\nOr the other way round we'd want to make repo_git_path to\nbe worktree specific and introduce repo_common_path for\nthe main working tree.\n\nLooking at Duys tree,\nhttps://gitlab.com/pclouds/git/commit/94751ada7c32eb6fb2c67dd7723161d1955a5683\nis pretty much what we need.\n\nReverting that topic that introduced this (4d6d6e,\nMerge branch 'sb/submodule-update-in-c'), might be possible but\nthat would conflict with another followup that fixes issues in\nthat series\n(see sb/submodule-unset-core-worktree-when-worktree-is-lost\nhttps://github.com/gitster/git/commits/sb/submodule-unset-core-worktree-when-worktree-is-lost)\nso I'd rather just cherry-pick the commit from Duy.\n\nStefan\n"},{"id":"366462","messageId":"CAG0vfyR3KnDDBrpyG-n-RFbu-xgCLFUa6HUXQ+dk8E4HutR+ow@mail.gmail.com","threadId":"50184","inReplyTo":"CAGZ79kZBwocC=UzjW+DxodwJkQZ2mNMYNjsk6sL4SCqdhGoQ7w@mail.gmail.com","subject":"Re: Regression: submodule worktrees can clobber core.worktree config","fromName":"Tomasz Śniatowski","fromEmail":"tsniatowski@vewd.com","sentAt":"2019-01-09T23:57:42Z","receivedAt":"2019-01-09T23:57:56Z","isPatch":false,"sender":{"key":"tsniatowski@vewd.com","avatar":null},"body":"On Wed, 9 Jan 2019 at 18:42, Stefan Beller <sbeller@google.com> wrote:\n>\n> On Tue, Jan 8, 2019 at 2:16 PM Tomasz Śniatowski <tsniatowski@vewd.com> wrote:\n> >\n> > After upgrading to 2.20.1 I noticed in some submodule+worktree scenarios git\n> > will break the submodule configuration. Reproducible with:\n> >     git init a && (cd a; touch a; git add a; git commit -ma)\n> >     git init b && (cd b; git submodule add ../a; git commit -mb)\n> >     git -C b worktree add ../b2\n> >     git -C b/a worktree add ../../b2/a\n> >     git -C b status\n> >     git -C b2 submodule update\n> >     git -C b status\n> >\n> > The submodule update in the _worktree_ puts an invalid core.worktree value in\n> > the _original_ repository submodule config (b/.git/modules/a/config), causing\n> > the last git status to error out with:\n> >     fatal: cannot chdir to '../../../../../../b2/a': No such file or directory\n> >     fatal: 'git status --porcelain=2' failed in submodule a\n> >\n> > Looking at the config file itself, the submodule update operation applies the\n> > following change (the new path is invalid):\n> >     -       worktree = ../../../a\n> >     +       worktree = ../../../../../../b2/a\n> >\n> > This worked fine on 2.19.2 (no config change, no error), and was useful to have\n> > a worktree with (large) submodules that are also worktrees.\n>\n> Thanks for reporting the issue!\n>\n> >\n> > Bisects down to:\n> > 74d4731da1 submodule--helper: replace connect-gitdir-workingtree by\n> > ensure-core-worktree\n>\n> So this would need to update the worktree config, not the generic config.\n>\n> We'd need to replace the line\n>     cfg_file = repo_git_path(&subrepo, \"config\");\n> in builtin/submodule--helper.c::ensure_core_worktree()\n> to be a worktree specific call.\n>\n> Or the other way round we'd want to make repo_git_path to\n> be worktree specific and introduce repo_common_path for\n> the main working tree.\n>\n> Looking at Duys tree,\n> https://gitlab.com/pclouds/git/commit/94751ada7c32eb6fb2c67dd7723161d1955a5683\n> is pretty much what we need.\n>\n> Reverting that topic that introduced this (4d6d6e,\n> Merge branch 'sb/submodule-update-in-c'), might be possible but\n> that would conflict with another followup that fixes issues in\n> that series\n> (see sb/submodule-unset-core-worktree-when-worktree-is-lost\n> https://github.com/gitster/git/commits/sb/submodule-unset-core-worktree-when-worktree-is-lost)\n> so I'd rather just cherry-pick the commit from Duy.\n\nI had a look at https://gitlab.com/pclouds/git/commits/submodules-in-worktrees,\nand it doesn't seem to be quite all okay.\n\nThe submodule update step of the repro (that breaks the config on 2.20) emits\nan error message instead, and leaves the config unchanged:\n   git -C b2 submodule update\n   fatal: could not set 'core.worktree' to '../../../../../../b2/a'\nIt looks a bit like it's still trying to do the wrong thing, but errors out\nduring the attempt (repo_config_set_worktree_gently returns false).\n\nCuriously, even though it says \"fatal\", it will then perform the actual\nsubmodule update if it's required.\n\nSame behavior on master with a subset of that branch cherry-picked, that is:\nhttps://gitlab.com/pclouds/git/commit/94751ada7c32eb6fb2c67dd7723161d1955a5683\nalong with two others it needed to build:\nhttps://gitlab.com/pclouds/git/commit/d26ab4c5013f6117814161be3e87c8d2b73561a4\nhttps://gitlab.com/pclouds/git/commit/b2e21eece6b35e00707ed3a8377a84a95da6b778\n\n--\nTomasz Śniatowski\n"},{"id":"366530","messageId":"CAGZ79kZ9ibM4eDyK=M6YWEDsjt+JfqJH-Gm56+092VATGuZDaw@mail.gmail.com","threadId":"50184","inReplyTo":"CAG0vfyR3KnDDBrpyG-n-RFbu-xgCLFUa6HUXQ+dk8E4HutR+ow@mail.gmail.com","subject":"Re: Regression: submodule worktrees can clobber core.worktree config","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2019-01-10T20:07:34Z","receivedAt":"2019-01-10T20:07:49Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> I had a look at https://gitlab.com/pclouds/git/commits/submodules-in-worktrees,\n> and it doesn't seem to be quite all okay.\n>\n> The submodule update step of the repro (that breaks the config on 2.20) emits\n> an error message instead, and leaves the config unchanged:\n>    git -C b2 submodule update\n>    fatal: could not set 'core.worktree' to '../../../../../../b2/a'\n> It looks a bit like it's still trying to do the wrong thing, but errors out\n> during the attempt (repo_config_set_worktree_gently returns false).\n\nThere is more than just that. After adding the worktrees,\n(and after the first status call)\n\n    $ cat b2/.git\ngitdir: /u/git/t/trash directory.t7419-submodule-worktrees/b/.git/worktrees/b2\n    $ cat b2/a/.git\ngitdir: /u/git/t/trash\ndirectory.t7419-submodule-worktrees/b/.git/modules/a/worktrees/a\n\nAre worktrees using absolute path for their gitlinks?\nSubmodules themselves try really hard to use relative path:\n\n    $ cat b/a/.git\ngitdir: ../.git/modules/a\n\n> Curiously, even though it says \"fatal\", it will then perform the actual\n> submodule update if it's required.\n\nOh. :/ I think we should solve that by either warning\n(but that gives bad UX) or actually aborting, by adding\na \"|| exit 1\" in git-submodule.sh in cmd_update where we\ncall \"git submodule--helper ensure-core-worktree\".\n\nWhen we run \"git -C b2 submodule update\", it calls\n\"git submodule--helper ensure-core-worktree a\" which\ncurrently would make sure that b2/a/.git points to\nb2/.git/modules/a, but that is not the case as b2 and b2/a\nare worktrees, whose git directories are housed in\nb/.git/worktrees.\n\nSo maybe we need to be a bit more careful and check\nif b2/a/.git resolves to a worktree and if so we'd not\ntouch it at all (and warn about it?).\n\n\n>\n> Same behavior on master with a subset of that branch cherry-picked, that is:\n> https://gitlab.com/pclouds/git/commit/94751ada7c32eb6fb2c67dd7723161d1955a5683\n> along with two others it needed to build:\n> https://gitlab.com/pclouds/git/commit/d26ab4c5013f6117814161be3e87c8d2b73561a4\n> https://gitlab.com/pclouds/git/commit/b2e21eece6b35e00707ed3a8377a84a95da6b778\n>\n> --\n> Tomasz Śniatowski\n"},{"id":"366548","messageId":"CACsJy8DQTFCO=nKZ9T02RDMZGGwiO1wGK1YERYaXSriiszf74w@mail.gmail.com","threadId":"50184","inReplyTo":"CAGZ79kZ9ibM4eDyK=M6YWEDsjt+JfqJH-Gm56+092VATGuZDaw@mail.gmail.com","subject":"Re: Regression: submodule worktrees can clobber core.worktree config","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-11T00:07:05Z","receivedAt":"2019-01-11T00:07:33Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jan 11, 2019 at 3:07 AM Stefan Beller <sbeller@google.com> wrote:\n>\n> > I had a look at https://gitlab.com/pclouds/git/commits/submodules-in-worktrees,\n> > and it doesn't seem to be quite all okay.\n> >\n> > The submodule update step of the repro (that breaks the config on 2.20) emits\n> > an error message instead, and leaves the config unchanged:\n> >    git -C b2 submodule update\n> >    fatal: could not set 'core.worktree' to '../../../../../../b2/a'\n> > It looks a bit like it's still trying to do the wrong thing, but errors out\n> > during the attempt (repo_config_set_worktree_gently returns false).\n>\n> There is more than just that. After adding the worktrees,\n> (and after the first status call)\n>\n>     $ cat b2/.git\n> gitdir: /u/git/t/trash directory.t7419-submodule-worktrees/b/.git/worktrees/b2\n>     $ cat b2/a/.git\n> gitdir: /u/git/t/trash\n> directory.t7419-submodule-worktrees/b/.git/modules/a/worktrees/a\n>\n> Are worktrees using absolute path for their gitlinks?\n\nYes. Moving to relative paths is on my todo list and I probably should\nget to it after I'm mostly done (*) with submodule support\n\n(*) Sharing submodule repos between worktrees is still something not addressed.\n-- \nDuy\n"}]}