{"thread":{"id":"50228","subject":"Worktree creation race","startedAt":"2019-01-15T14:03:41Z","lastAt":"2019-02-01T19:57:32Z","messageCount":7,"participants":["Marketa Calabkova","Eric Sunshine","Duy Nguyen","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"366710","messageId":"89985b60-4c28-ebf0-64b1-8da75263d745@suse.cz","threadId":"50228","inReplyTo":null,"subject":"Worktree creation race","fromName":"Marketa Calabkova","fromEmail":"mcalabkova@suse.cz","sentAt":"2019-01-15T14:03:36Z","receivedAt":"2019-01-15T14:03:41Z","isPatch":false,"sender":{"key":"mcalabkova@suse.cz","avatar":null},"body":"Hello,\n\nI am writing to report a bug. The original report is from my colleague, I am also providing his suggestions.\n\nThere is insufficient locking for worktree addition. Adding worktree may fail.\n\nThe problem is that git reads the directory entries in $GIT_DIR/worktrees, \nfinds a worktree name that does not exist, tries to create it, and if an \nerror is returned adding the worktree fails. When multiple git processes \ndo this in parallel only one adds a worktree and the others fail. Git should \nreread the directory and find a new name that does not exist when creating \nthe worktree directory fails because another git process already created it.\n\nI suppose adding PID in the tree name would mitigate the issue to the point it will be very unlikely to encounter.\n\nI need more than the tree in the temporary directory so using the temporary directory directly as a tree is out of question.\n\nto test:\n\ncd /dev/shm\nmkdir gittest\ncd gittest\ngit init gitrepo\ncd gitrepo\ngit commit --allow-empty -m Empty\nfor n in $(seq 10000) ; do ( tmp=$(mktemp -d /dev/shm/gittest/test.XXXXXXXXXXX) ; mkdir $tmp/test ; git worktree add --detach $tmp/test ; ) & done\n\n(you should see many messages like: \nfatal: could not create directory of '.git/worktrees/test284': File exists)\n\nGreetings,\nMarketa\n\n\n"},{"id":"367822","messageId":"2ee89ff3-e672-e940-f601-aa1d2647756a@suse.cz","threadId":"50228","inReplyTo":"89985b60-4c28-ebf0-64b1-8da75263d745@suse.cz","subject":"Re: Worktree creation race","fromName":"Marketa Calabkova","fromEmail":"mcalabkova@suse.cz","sentAt":"2019-01-28T12:58:05Z","receivedAt":"2019-01-28T12:58:09Z","isPatch":false,"sender":{"key":"mcalabkova@suse.cz","avatar":null},"body":"Hi,\n\nyou have probably overseen my email :) . Please, I would like to get it\nfixed.\n\nDoes anyone has a suggestion what to do with this bug? It looks like a\none-line fix probably in builtin/worktree.c, but I have no idea how to\ndo it. Sorry.\n\nThank you,\nMarketa\n\nOn 15/01/2019 15:03, Marketa Calabkova wrote:\n> Hello,\n>\n> I am writing to report a bug. The original report is from my colleague, I am also providing his suggestions.\n>\n> There is insufficient locking for worktree addition. Adding worktree may fail.\n>\n> The problem is that git reads the directory entries in $GIT_DIR/worktrees, \n> finds a worktree name that does not exist, tries to create it, and if an \n> error is returned adding the worktree fails. When multiple git processes \n> do this in parallel only one adds a worktree and the others fail. Git should \n> reread the directory and find a new name that does not exist when creating \n> the worktree directory fails because another git process already created it.\n>\n> I suppose adding PID in the tree name would mitigate the issue to the point it will be very unlikely to encounter.\n>\n> I need more than the tree in the temporary directory so using the temporary directory directly as a tree is out of question.\n>\n> to test:\n>\n> cd /dev/shm\n> mkdir gittest\n> cd gittest\n> git init gitrepo\n> cd gitrepo\n> git commit --allow-empty -m Empty\n> for n in $(seq 10000) ; do ( tmp=$(mktemp -d /dev/shm/gittest/test.XXXXXXXXXXX) ; mkdir $tmp/test ; git worktree add --detach $tmp/test ; ) & done\n>\n> (you should see many messages like: \n> fatal: could not create directory of '.git/worktrees/test284': File exists)\n>\n> Greetings,\n> Marketa\n>\n>\n\n\n"},{"id":"368300","messageId":"CAPig+cSdpq0Bfq3zSK8kJd6da3dKixK7qYQ24=ZwbuQtsaLNZw@mail.gmail.com","threadId":"50228","inReplyTo":"2ee89ff3-e672-e940-f601-aa1d2647756a@suse.cz","subject":"Re: Worktree creation race","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-02-01T06:27:31Z","receivedAt":"2019-02-01T06:27:44Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jan 28, 2019 at 7:58 AM Marketa Calabkova <mcalabkova@suse.cz> wrote:\n> On 15/01/2019 15:03, Marketa Calabkova wrote:\n> > I am writing to report a bug. The original report is from my colleague, I am also providing his suggestions.\n> >\n> > There is insufficient locking for worktree addition. Adding worktree may fail.\n> >\n> > The problem is that git reads the directory entries in $GIT_DIR/worktrees,\n> > finds a worktree name that does not exist, tries to create it, and if an\n> > error is returned adding the worktree fails. When multiple git processes\n> > do this in parallel only one adds a worktree and the others fail. Git should\n> > reread the directory and find a new name that does not exist when creating\n> > the worktree directory fails because another git process already created it.\n> >\n> > I suppose adding PID in the tree name would mitigate the issue to the point it will be very unlikely to encounter.\n> >\n> > I need more than the tree in the temporary directory so using the temporary directory directly as a tree is out of question.\n> >\n> > cd gitrepo\n> > git commit --allow-empty -m Empty\n> > for n in $(seq 10000) ; do ( tmp=$(mktemp -d /dev/shm/gittest/test.XXXXXXXXXXX) ; mkdir $tmp/test ; git worktree add --detach $tmp/test ; ) & done\n> >\n> > (you should see many messages like:\n> > fatal: could not create directory of '.git/worktrees/test284': File exists)\n> >\n> Does anyone has a suggestion what to do with this bug? It looks like a\n> one-line fix probably in builtin/worktree.c, but I have no idea how to\n> do it. Sorry.\n\nI doubt this is a one-line fix, and I don't think it has anything to\ndo with reading entries in $GIT_DIR/worktrees.\n\nadd_worktree() already attempts to give a unique identifier to each\nworktree by adding a numeric suffix and incrementing that suffix if\nthe name already exists (such as the 284 in your example error\nmessage) but there is definitely a race-condition between the time it\nstat()s the name and the time it mkdir()s it.\n\nOne possible fix would be to unconditionally use the PID, as you\nsuggest, though, this is not necessarily foolproof against races\neither (though it makes collisions very unlikely).\n\nAnother possibility would be to skip the stat() and instead do the\nmkdir() in a loop, incrementing the sequence number each time through\nthe loop. That should eliminate the race entirely (I think).\n"},{"id":"368302","messageId":"CACsJy8D+zvgtw8RuBefYQsYrvn_vQT1dUO3Veg5ZtfQa2u5QKA@mail.gmail.com","threadId":"50228","inReplyTo":"CAPig+cSdpq0Bfq3zSK8kJd6da3dKixK7qYQ24=ZwbuQtsaLNZw@mail.gmail.com","subject":"Re: Worktree creation race","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-01T07:06:43Z","receivedAt":"2019-02-01T07:07:12Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Thanks for including me. Apparently I did miss some emails :)\n\nOn Fri, Feb 1, 2019 at 1:27 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> On Mon, Jan 28, 2019 at 7:58 AM Marketa Calabkova <mcalabkova@suse.cz> wrote:\n> > On 15/01/2019 15:03, Marketa Calabkova wrote:\n> > > I am writing to report a bug. The original report is from my colleague, I am also providing his suggestions.\n> > >\n> > > There is insufficient locking for worktree addition. Adding worktree may fail.\n> > >\n> > > The problem is that git reads the directory entries in $GIT_DIR/worktrees,\n> > > finds a worktree name that does not exist, tries to create it, and if an\n> > > error is returned adding the worktree fails. When multiple git processes\n> > > do this in parallel only one adds a worktree and the others fail. Git should\n> > > reread the directory and find a new name that does not exist when creating\n> > > the worktree directory fails because another git process already created it.\n> > >\n> > > I suppose adding PID in the tree name would mitigate the issue to the point it will be very unlikely to encounter.\n> > >\n> > > I need more than the tree in the temporary directory so using the temporary directory directly as a tree is out of question.\n> > >\n> > > cd gitrepo\n> > > git commit --allow-empty -m Empty\n> > > for n in $(seq 10000) ; do ( tmp=$(mktemp -d /dev/shm/gittest/test.XXXXXXXXXXX) ; mkdir $tmp/test ; git worktree add --detach $tmp/test ; ) & done\n> > >\n> > > (you should see many messages like:\n> > > fatal: could not create directory of '.git/worktrees/test284': File exists)\n> > >\n> > Does anyone has a suggestion what to do with this bug? It looks like a\n> > one-line fix probably in builtin/worktree.c, but I have no idea how to\n> > do it. Sorry.\n>\n> I doubt this is a one-line fix, and I don't think it has anything to\n> do with reading entries in $GIT_DIR/worktrees.\n\nI never thought people would create worktrees \"like crazy\" to end up\nworrying about races like this. The mkdir loop would be one way to go.\nBut I'm going to add a new option to let the user control this\ndirectory name. This is necessary since this name is now exposed via\n\"worktrees/<name>\" reference space and should also be reported in \"git\nworktree list\". Avoiding the race is a nice bonus.\n\n> add_worktree() already attempts to give a unique identifier to each\n> worktree by adding a numeric suffix and incrementing that suffix if\n> the name already exists (such as the 284 in your example error\n> message) but there is definitely a race-condition between the time it\n> stat()s the name and the time it mkdir()s it.\n>\n> One possible fix would be to unconditionally use the PID, as you\n> suggest, though, this is not necessarily foolproof against races\n> either (though it makes collisions very unlikely).\n>\n> Another possibility would be to skip the stat() and instead do the\n> mkdir() in a loop, incrementing the sequence number each time through\n> the loop. That should eliminate the race entirely (I think).\n-- \nDuy\n"},{"id":"368325","messageId":"20190201131659.GA30564@ash","threadId":"50228","inReplyTo":"CACsJy8D+zvgtw8RuBefYQsYrvn_vQT1dUO3Veg5ZtfQa2u5QKA@mail.gmail.com","subject":"Re: Worktree creation race","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-01T13:16:59Z","receivedAt":"2019-02-01T13:17:06Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Feb 01, 2019 at 02:06:43PM +0700, Duy Nguyen wrote:\n> worrying about races like this. The mkdir loop would be one way to go.\n> But I'm going to add a new option to let the user control this\n> directory name. This is necessary since this name is now exposed via\n> \"worktrees/<name>\" reference space and should also be reported in \"git\n> worktree list\". Avoiding the race is a nice bonus.\n\nI'm not going to bother you with code yet (although if you want, you\ncan check out branch worktree-name on my gitlab repo), but this is\nwhat the user facing changes look like. Looking good?\n\nPS. I think this also calls for a command to rename working trees.\nSigh.. this worktree thingy never ends.\n\n-- 8< --\ndiff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\nindex 18469e202e..d5db69dec7 100644\n--- a/Documentation/git-worktree.txt\n+++ b/Documentation/git-worktree.txt\n@@ -9,7 +9,7 @@ git-worktree - Manage multiple working trees\n SYNOPSIS\n --------\n [verse]\n-'git worktree add' [-f] [--detach] [--checkout] [--lock] [-b <new-branch>] <path> [<commit-ish>]\n+'git worktree add' [-f] [--detach] [--checkout] [--lock] [-b <new-branch>] [--name <name>] <path> [<commit-ish>]\n 'git worktree list' [--porcelain]\n 'git worktree lock' [--reason <string>] <worktree>\n 'git worktree move' <worktree> <new-path>\n@@ -195,6 +195,13 @@ This can also be set up as the default behaviour by using the\n --reason <string>::\n \tWith `lock`, an explanation why the working tree is locked.\n \n+--name <name>::\n+\tName of the working tree. Each working tree must have a unique\n+\tname. This is also the directory name containing all working\n+\ttree's specific information under `$GIT_COMMON_DIR/worktrees`.\n+\tIf `--name` is not given, the name is based on basename(3)\n+\toptionally with a number suffix to make it unique.\n+\n <worktree>::\n \tWorking trees can be identified by path, either relative or\n \tabsolute.\n@@ -323,11 +330,15 @@ details on a single line with columns.  For example:\n \n ------------\n $ git worktree list\n-/path/to/bare-source            (bare)\n-/path/to/linked-worktree        abcd1234 [master]\n-/path/to/other-linked-worktree  1234abc  (detached HEAD)\n+(main) /path/to/bare-source            (bare)\n+linked /path/to/linked-worktree        abcd1234 [master]\n+other  /path/to/other-linked-worktree  1234abc  (detached HEAD)\n ------------\n \n+The first column is the name of the working tree. The second column is\n+the location of the working tree. The third column is the short commit\n+name of the current branch. The branch name is put in square brackets.\n+\n Porcelain Format\n ~~~~~~~~~~~~~~~~\n The porcelain format has a line per attribute.  Attributes are listed with a\n@@ -341,10 +352,12 @@ $ git worktree list --porcelain\n worktree /path/to/bare-source\n bare\n \n+name linked\n worktree /path/to/linked-worktree\n HEAD abcd1234abcd1234abcd1234abcd1234abcd1234\n branch refs/heads/master\n \n+name other\n worktree /path/to/other-linked-worktree\n HEAD 1234abc1234abc1234abc1234abc1234abc1234a\n detached\n-- 8< --\n--\nDuy\n"},{"id":"368362","messageId":"xmqqh8dnz8mr.fsf@gitster-ct.c.googlers.com","threadId":"50228","inReplyTo":"CAPig+cSdpq0Bfq3zSK8kJd6da3dKixK7qYQ24=ZwbuQtsaLNZw@mail.gmail.com","subject":"Re: Worktree creation race","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-01T18:06:04Z","receivedAt":"2019-02-01T18:06:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> Another possibility would be to skip the stat() and instead do the\n> mkdir() in a loop, incrementing the sequence number each time through\n> the loop. That should eliminate the race entirely (I think).\n\nYeah, I think that is the only sensible option to rely on the\natomicity of mkdir() call.\n"},{"id":"368367","messageId":"CAPig+cRg5E=_QuUz4j4sGNR=ziHth0Lv7hncm_hvZboBvv+bnQ@mail.gmail.com","threadId":"50228","inReplyTo":"20190201131659.GA30564@ash","subject":"Re: Worktree creation race","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-02-01T19:54:53Z","receivedAt":"2019-02-01T19:57:32Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Feb 1, 2019 at 8:17 AM Duy Nguyen <pclouds@gmail.com> wrote:\n> I'm not going to bother you with code yet (although if you want, you\n> can check out branch worktree-name on my gitlab repo), but this is\n> what the user facing changes look like. Looking good?\n>\n> -- 8< --\n> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt\n> @@ -9,7 +9,7 @@ git-worktree - Manage multiple working trees\n>  SYNOPSIS\n> +'git worktree add' [-f] [--detach] [--checkout] [--lock] [-b <new-branch>] [--name <name>] <path> [<commit-ish>]\n> @@ -195,6 +195,13 @@ This can also be set up as the default behaviour by using the\n> +--name <name>::\n> +       Name of the working tree. Each working tree must have a unique\n> +       name. This is also the directory name containing all working\n> +       tree's specific information under `$GIT_COMMON_DIR/worktrees`.\n> +       If `--name` is not given, the name is based on basename(3)\n> +       optionally with a number suffix to make it unique.\n\nI think the \"DETAILS\" section also needs to be updated. It presently says:\n\n    The private sub-directory’s name is usually the base name of the\n    linked working tree’s path, possibly appended with a number to\n    make it unique.\n\nPerhaps amend it to:\n\n    ...to make it unique, or the name provided by `--name`.\n\n>  Porcelain Format\n>  ~~~~~~~~~~~~~~~~\n>  The porcelain format has a line per attribute.  Attributes are listed with a\n> @@ -341,10 +352,12 @@ $ git worktree list --porcelain\n>  worktree /path/to/bare-source\n>  bare\n>\n> +name linked\n>  worktree /path/to/linked-worktree\n>  HEAD abcd1234abcd1234abcd1234abcd1234abcd1234\n>  branch refs/heads/master\n>\n> +name other\n>  worktree /path/to/other-linked-worktree\n>  HEAD 1234abc1234abc1234abc1234abc1234abc1234a\n>  detached\n\nUnfortunately, this will likely break existing tools. When I had\noutlined the proposed porcelain format, my suggestion was that a\n\"worktree\" line would indicate the start of a new stanza (with no\nblank line), however, as it eventually got implemented (not by me),\nthat recommendation wasn't quite followed. Instead, what has been\ndocumented \"officially\" in the git-worktree man-page is that each\nstanza will start with a \"worktree\" line and that there will be a\nblank line between stanzas. So, the new \"name\" line will need to be\nplaced somewhere after the \"worktree\" line in order to avoid tool\nbreakage.\n"}]}