{"thread":{"id":"50535","subject":"[PATCH 0/2] worktree add race fix","startedAt":"2019-02-18T17:05:23Z","lastAt":"2019-03-11T01:55:51Z","messageCount":27,"participants":["Michal Suchanek","Eric Sunshine","Michal Suchánek","Duy Nguyen","Phillip Wood","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"369538","messageId":"cover.1550508544.git.msuchanek@suse.de","threadId":"50535","inReplyTo":null,"subject":"[PATCH 0/2] worktree add race fix","fromName":"Michal Suchanek","fromEmail":"msuchanek@suse.de","sentAt":"2019-02-18T17:04:55Z","receivedAt":"2019-02-18T17:05:23Z","isPatch":true,"sender":{"key":"msuchanek@suse.de","avatar":"https://avatars.githubusercontent.com/u/787652?v=4"},"body":"Hello,\n\nI am running a git automation script that crates a tree and commmits it into a\ngit repository repeatedly.\n\nI noticed that the step which creates a tree is most time-consuming part of the\nscript and when a lot of data is to be automatically added to the repository it\nis benefical to parallelize this part.\n\nTo do so I had the script create a dozen worktrees and share the work between\nthem. The problem is automatically creating several worktrees occasioanlly\nfails.\n\nThe most common problem is in the worktree add implementation itself which\ntries to find an available directory name and then mkdir() it. Of course, doing\nthat several times in parallel causes issues.\n\nWhen running stress-test to make sure the fix is effective I uncovered\nadditional issues in get_common_dir_noenv. This function is used on each\nworktree to build a worktree list.\n\nApparently it can happen that stat() claims there is a commondir file but when\ntrying to open the file it is missing.\n\nAnother even rarer issue is that the file might be zero size because another\nprocess initializing a worktree opened the file but has not written is content\nyet.\n\nWhen any of this happnes git aborts failing to create a worktree because\nunrelated worktree is not yet fully initialized.\n\nI have tested that these patches fix the issue. However, I expect race against\nremoving/pruning worktrees is still possible.\n\nFor previous discussion see\n\nhttp://public-inbox.org/git/CAPig+cSdpq0Bfq3zSK8kJd6da3dKixK7qYQ24=ZwbuQtsaLNZw@mail.gmail.com/\n\nMichal Suchanek (2):\n worktree: fix worktree add race.\n setup: don't fail if commondir reference is deleted.\n\n builtin/worktree.c | 12 +++++++-----\n setup.c | 16 +++++++++++-----\n 2 files changed, 18 insertions(+), 10 deletions(-)\n\n-- \n2.20.1\n\n"},{"id":"369539","messageId":"c7c66ca1f954e20dad8eeec3fb10c53ad844c800.1550508544.git.msuchanek@suse.de","threadId":"50535","inReplyTo":"cover.1550508544.git.msuchanek@suse.de","subject":"[PATCH 1/2] worktree: fix worktree add race.","fromName":"Michal Suchanek","fromEmail":"msuchanek@suse.de","sentAt":"2019-02-18T17:04:56Z","receivedAt":"2019-02-18T17:05:23Z","isPatch":true,"sender":{"key":"msuchanek@suse.de","avatar":"https://avatars.githubusercontent.com/u/787652?v=4"},"body":"Git runs a stat loop to find a worktree name that's available and then does\nmkdir on the found name. Turn it to mkdir loop to avoid another invocation of\nworktree add finding the same free name and creating the directory first.\n\nSigned-off-by: Michal Suchanek \n---\nv2:\n- simplify loop exit condition\n- exit early if the mkdir fails for reason other than already present\nworktree\n- make counter unsigned\n---\n builtin/worktree.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 3f9907fcc994..85a604cfe98c 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -268,10 +268,10 @@ static int add_worktree(const char *path, const char *refname,\n \tstruct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n \tconst char *name;\n-\tstruct stat st;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct argv_array child_env = ARGV_ARRAY_INIT;\n-\tint counter = 0, len, ret;\n+\tunsigned int counter = 0;\n+\tint len, ret;\n \tstruct strbuf symref = STRBUF_INIT;\n \tstruct commit *commit = NULL;\n \tint is_branch = 0;\n@@ -295,8 +295,12 @@ static int add_worktree(const char *path, const char *refname,\n \tif (safe_create_leading_directories_const(sb_repo.buf))\n \t\tdie_errno(_(\"could not create leading directories of '%s'\"),\n \t\t\t sb_repo.buf);\n-\twhile (!stat(sb_repo.buf, &st)) {\n+\n+\twhile (mkdir(sb_repo.buf, 0777)) {\n \t\tcounter++;\n+\t\tif ((errno != EEXIST) || !counter /* overflow */)\n+\t\t\tdie_errno(_(\"could not create directory of '%s'\"),\n+\t\t\t\t sb_repo.buf);\n \t\tstrbuf_setlen(&sb_repo, len);\n \t\tstrbuf_addf(&sb_repo, \"%d\", counter);\n \t}\n@@ -306,8 +310,6 @@ static int add_worktree(const char *path, const char *refname,\n \tatexit(remove_junk);\n \tsigchain_push_common(remove_junk_on_signal);\n \n-\tif (mkdir(sb_repo.buf, 0777))\n-\t\tdie_errno(_(\"could not create directory of '%s'\"), sb_repo.buf);\n \tjunk_git_dir = xstrdup(sb_repo.buf);\n \tis_junk = 1;\n \n-- \n2.20.1\n\n"},{"id":"369540","messageId":"6f9c8775817117c2b36539eb048e2462a650ab8f.1550508544.git.msuchanek@suse.de","threadId":"50535","inReplyTo":"cover.1550508544.git.msuchanek@suse.de","subject":"[PATCH 2/2] setup: don't fail if commondir reference is deleted.","fromName":"Michal Suchanek","fromEmail":"msuchanek@suse.de","sentAt":"2019-02-18T17:04:57Z","receivedAt":"2019-02-18T17:05:24Z","isPatch":true,"sender":{"key":"msuchanek@suse.de","avatar":"https://avatars.githubusercontent.com/u/787652?v=4"},"body":"When adding wotktrees git can die in get_common_dir_noenv while\nexamining existing worktrees because the commondir file does not exist.\nRather than testing if the file exists before reading it handle ENOENT.\n\nSigned-off-by: Michal Suchanek \n---\nv2:\n- do not test file existence first, just read it and handle ENOENT.\n- handle zero size file correctly\n---\n setup.c | 16 +++++++++++-----\n 1 file changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex ca9e8a949ed8..dd865f280d34 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -270,12 +270,20 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)\n {\n \tstruct strbuf data = STRBUF_INIT;\n \tstruct strbuf path = STRBUF_INIT;\n-\tint ret = 0;\n+\tint ret;\n \n \tstrbuf_addf(&path, \"%s/commondir\", gitdir);\n-\tif (file_exists(path.buf)) {\n-\t\tif (strbuf_read_file(&data, path.buf, 0) <= 0)\n+\tret = strbuf_read_file(&data, path.buf, 0);\n+\tif (ret <= 0) {\n+\t\t/*\n+\t\t * if file is missing or zero size (just being written)\n+\t\t * assume default, bail otherwise\n+\t\t */\n+\t\tif (ret && errno != ENOENT)\n \t\t\tdie_errno(_(\"failed to read %s\"), path.buf);\n+\t\tstrbuf_addstr(sb, gitdir);\n+\t\tret = 0;\n+\t} else {\n \t\twhile (data.len && (data.buf[data.len - 1] == '\\n' ||\n \t\t\t\t data.buf[data.len - 1] == '\\r'))\n \t\t\tdata.len--;\n@@ -286,8 +294,6 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)\n \t\tstrbuf_addbuf(&path, &data);\n \t\tstrbuf_add_real_path(sb, path.buf);\n \t\tret = 1;\n-\t} else {\n-\t\tstrbuf_addstr(sb, gitdir);\n \t}\n \n \tstrbuf_release(&data);\n-- \n2.20.1\n\n"},{"id":"369564","messageId":"CAPig+cTA1hNXMiKb5Jwk3k+xfgCv-+B2HzdoJFLN9Fa5cN4NKg@mail.gmail.com","threadId":"50535","inReplyTo":"6f9c8775817117c2b36539eb048e2462a650ab8f.1550508544.git.msuchanek@suse.de","subject":"Re: [PATCH 2/2] setup: don't fail if commondir reference is deleted.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-02-18T21:00:27Z","receivedAt":"2019-02-18T21:00:41Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Feb 18, 2019 at 12:05 PM Michal Suchanek wrote:\n> When adding wotktrees git can die in get_common_dir_noenv while\n> examining existing worktrees because the commondir file does not exist.\n> Rather than testing if the file exists before reading it handle ENOENT.\n\nThis commit message leaves the reader wondering under what conditions\n\"commondir file\" might not exist. For instance, the reader might\nwonder \"iIs it simply a condition of normal operation or does it arise\nunder odd circumstances?\". Without this information, it is difficult\nfor someone reading the explanation to understand if or how this code\nmight validly be changed in the future. Your cover letter contained\nexplanation which likely ought to be duplicated here as an aid to\nfuture readers.\n\n> Signed-off-by: Michal Suchanek \n"},{"id":"369722","messageId":"e134801d570d0a0c85424eb80b41893f4d8383ca.1550679076.git.msuchanek@suse.de","threadId":"50535","inReplyTo":"cover.1550508544.git.msuchanek@suse.de","subject":"[PATCH v3 1/2] worktree: fix worktree add race.","fromName":"Michal Suchanek","fromEmail":"msuchanek@suse.de","sentAt":"2019-02-20T16:16:48Z","receivedAt":"2019-02-20T16:16:55Z","isPatch":true,"sender":{"key":"msuchanek@suse.de","avatar":"https://avatars.githubusercontent.com/u/787652?v=4"},"body":"Git runs a stat loop to find a worktree name that's available and then does\nmkdir on the found name. Turn it to mkdir loop to avoid another invocation of\nworktree add finding the same free name and creating the directory first.\n\nSigned-off-by: Michal Suchanek \n---\nv2:\n- simplify loop exit condition\n- exit early if the mkdir fails for reason other than already present\nworktree\n- make counter unsigned\n---\n builtin/worktree.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 3f9907fcc994..85a604cfe98c 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -268,10 +268,10 @@ static int add_worktree(const char *path, const char *refname,\n \tstruct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n \tconst char *name;\n-\tstruct stat st;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct argv_array child_env = ARGV_ARRAY_INIT;\n-\tint counter = 0, len, ret;\n+\tunsigned int counter = 0;\n+\tint len, ret;\n \tstruct strbuf symref = STRBUF_INIT;\n \tstruct commit *commit = NULL;\n \tint is_branch = 0;\n@@ -295,8 +295,12 @@ static int add_worktree(const char *path, const char *refname,\n \tif (safe_create_leading_directories_const(sb_repo.buf))\n \t\tdie_errno(_(\"could not create leading directories of '%s'\"),\n \t\t\t sb_repo.buf);\n-\twhile (!stat(sb_repo.buf, &st)) {\n+\n+\twhile (mkdir(sb_repo.buf, 0777)) {\n \t\tcounter++;\n+\t\tif ((errno != EEXIST) || !counter /* overflow */)\n+\t\t\tdie_errno(_(\"could not create directory of '%s'\"),\n+\t\t\t\t sb_repo.buf);\n \t\tstrbuf_setlen(&sb_repo, len);\n \t\tstrbuf_addf(&sb_repo, \"%d\", counter);\n \t}\n@@ -306,8 +310,6 @@ static int add_worktree(const char *path, const char *refname,\n \tatexit(remove_junk);\n \tsigchain_push_common(remove_junk_on_signal);\n \n-\tif (mkdir(sb_repo.buf, 0777))\n-\t\tdie_errno(_(\"could not create directory of '%s'\"), sb_repo.buf);\n \tjunk_git_dir = xstrdup(sb_repo.buf);\n \tis_junk = 1;\n \n-- \n2.20.1\n\n"},{"id":"369723","messageId":"37df7fd81c3dee990bd7723f18c94713a0d842b6.1550679076.git.msuchanek@suse.de","threadId":"50535","inReplyTo":"cover.1550508544.git.msuchanek@suse.de","subject":"[PATCH v3 2/2] setup: don't fail if commondir reference is deleted.","fromName":"Michal Suchanek","fromEmail":"msuchanek@suse.de","sentAt":"2019-02-20T16:16:49Z","receivedAt":"2019-02-20T16:16:56Z","isPatch":true,"sender":{"key":"msuchanek@suse.de","avatar":"https://avatars.githubusercontent.com/u/787652?v=4"},"body":"Apparently it can happen that stat() claims there is a commondir file but when\ntrying to open the file it is missing.\n\nAnother even rarer issue is that the file might be zero size because another\nprocess initializing a worktree opened the file but has not written is content\nyet.\n\nWhen any of this happnes git aborts failing to perform perfectly valid\ncommand because unrelated worktree is not yet fully initialized.\n\nRather than testing if the file exists before reading it handle ENOENT\nand ENOTDIR.\n\nSigned-off-by: Michal Suchanek \n---\nv2:\n- do not test file existence first, just read it and handle ENOENT.\n- handle zero size file correctly\nv3:\n- handle ENOTDIR as well\n- add more details to commit message\n---\n setup.c | 16 +++++++++++-----\n 1 file changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex ca9e8a949ed8..49306e36990d 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -270,12 +270,20 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)\n {\n \tstruct strbuf data = STRBUF_INIT;\n \tstruct strbuf path = STRBUF_INIT;\n-\tint ret = 0;\n+\tint ret;\n \n \tstrbuf_addf(&path, \"%s/commondir\", gitdir);\n-\tif (file_exists(path.buf)) {\n-\t\tif (strbuf_read_file(&data, path.buf, 0) <= 0)\n+\tret = strbuf_read_file(&data, path.buf, 0);\n+\tif (ret <= 0) {\n+\t\t/*\n+\t\t * if file is missing or zero size (just being written)\n+\t\t * assume default, bail otherwise\n+\t\t */\n+\t\tif (ret && errno != ENOENT && errno != ENOTDIR)\n \t\t\tdie_errno(_(\"failed to read %s\"), path.buf);\n+\t\tstrbuf_addstr(sb, gitdir);\n+\t\tret = 0;\n+\t} else {\n \t\twhile (data.len && (data.buf[data.len - 1] == '\\n' ||\n \t\t\t\t data.buf[data.len - 1] == '\\r'))\n \t\t\tdata.len--;\n@@ -286,8 +294,6 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)\n \t\tstrbuf_addbuf(&path, &data);\n \t\tstrbuf_add_real_path(sb, path.buf);\n \t\tret = 1;\n-\t} else {\n-\t\tstrbuf_addstr(sb, gitdir);\n \t}\n \n \tstrbuf_release(&data);\n-- \n2.20.1\n\n"},{"id":"369725","messageId":"CAPig+cSdA8XRwCJQD3o6DZLwesBLRTys7OV6u0wy9Ve3Hp6XPA@mail.gmail.com","threadId":"50535","inReplyTo":"e134801d570d0a0c85424eb80b41893f4d8383ca.1550679076.git.msuchanek@suse.de","subject":"Re: [PATCH v3 1/2] worktree: fix worktree add race.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-02-20T16:34:54Z","receivedAt":"2019-02-20T16:35:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 20, 2019 at 11:17 AM Michal Suchanek wrote:\n> Git runs a stat loop to find a worktree name that's available and then does\n> mkdir on the found name. Turn it to mkdir loop to avoid another invocation of\n> worktree add finding the same free name and creating the directory first.\n>\n> Signed-off-by: Michal Suchanek \n> ---\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> @@ -295,8 +295,12 @@ static int add_worktree(const char *path, const char *refname,\n> if (safe_create_leading_directories_const(sb_repo.buf))\n> die_errno(_(\"could not create leading directories of '%s'\"),\n> sb_repo.buf);\n> - while (!stat(sb_repo.buf, &st)) {\n> + while (mkdir(sb_repo.buf, 0777)) {\n> counter++;\n> + if ((errno != EEXIST) || !counter /* overflow */)\n> + die_errno(_(\"could not create directory of '%s'\"),\n> + sb_repo.buf);\n> strbuf_setlen(&sb_repo, len);\n> strbuf_addf(&sb_repo, \"%d\", counter);\n> }\n> @@ -306,8 +310,6 @@ static int add_worktree(const char *path, const char *refname,\n> atexit(remove_junk);\n> sigchain_push_common(remove_junk_on_signal);\n> - if (mkdir(sb_repo.buf, 0777))\n> - die_errno(_(\"could not create directory of '%s'\"), sb_repo.buf);\n> junk_git_dir = xstrdup(sb_repo.buf);\n> is_junk = 1;\n\nDid you audit this \"junk\" handling to verify that stuff which ought to\nbe cleaned up still is cleaned up now that the mkdir() and die() have\nbeen moved above the atexit(remove_junk) invocation?\n\nI did just audit it, and I _think_ that it still works as expected,\nbut it would be good to hear that someone else has come to the same\nconclusion.\n"},{"id":"369727","messageId":"CAPig+cQZNOWvaa5H2PKOs149KvRtEYRzrdLvzvFRDo4Qxaecaw@mail.gmail.com","threadId":"50535","inReplyTo":"37df7fd81c3dee990bd7723f18c94713a0d842b6.1550679076.git.msuchanek@suse.de","subject":"Re: [PATCH v3 2/2] setup: don't fail if commondir reference is deleted.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-02-20T16:55:46Z","receivedAt":"2019-02-20T16:55:59Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 20, 2019 at 11:17 AM Michal Suchanek wrote:\n> Apparently it can happen that stat() claims there is a commondir file but when\n> trying to open the file it is missing.\n\nUnder what circumstances?\n\n> Another even rarer issue is that the file might be zero size because another\n> process initializing a worktree opened the file but has not written is content\n> yet.\n\nBased upon the explanation thus far, I'm having trouble understanding\nunder what circumstances these race conditions can arise. Are you\ntrying to invoke Git commands in a particular worktree even as the\nworktree itself is being created?\n\nWithout this information being spelled out clearly, it is going to be\ndifficult for someone in the future to reason about why the code is\nthe way it is following this change.\n\n> When any of this happnes git aborts failing to perform perfectly valid\n> command because unrelated worktree is not yet fully initialized.\n\ns/happnes/happens/\n\n> Rather than testing if the file exists before reading it handle ENOENT\n> and ENOTDIR.\n\nOne more comment below...\n\n> Signed-off-by: Michal Suchanek \n> ---\n> diff --git a/setup.c b/setup.c\n> @@ -270,12 +270,20 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)\n> {\n> strbuf_addf(&path, \"%s/commondir\", gitdir);\n> - if (file_exists(path.buf)) {\n> - if (strbuf_read_file(&data, path.buf, 0) <= 0)\n> + ret = strbuf_read_file(&data, path.buf, 0);\n> + if (ret <= 0) {\n> + /*\n> + * if file is missing or zero size (just being written)\n> + * assume default, bail otherwise\n> + */\n> + if (ret && errno != ENOENT && errno != ENOTDIR)\n> die_errno(_(\"failed to read %s\"), path.buf);\n\nIt's not clear from the explanation given in the commit message if the\nnew behavior is indeed sensible. The original intent of the code, as I\nunderstand it, is to validate \"commondir\", to ensure that it is not\nsomehow corrupt (such as the user editing it and making it empty).\nFollowing this change, that particular validation no longer takes\nplace. But, more importantly, what does it mean to fall back to\n\"default\" for this particular worktree? I'm having trouble\nunderstanding how the new behavior can be correct or desirable. (Am I\nmissing something obvious?)\n\n> + strbuf_addstr(sb, gitdir);\n> + ret = 0;\n> + } else {\n"},{"id":"369728","messageId":"20190220181605.60bbc28d@kitsune.suse.cz","threadId":"50535","inReplyTo":"CAPig+cQZNOWvaa5H2PKOs149KvRtEYRzrdLvzvFRDo4Qxaecaw@mail.gmail.com","subject":"Re: [PATCH v3 2/2] setup: don't fail if commondir reference is deleted.","fromName":"Michal Suchánek","fromEmail":"msuchanek@suse.de","sentAt":"2019-02-20T17:16:05Z","receivedAt":"2019-02-20T17:16:11Z","isPatch":true,"sender":{"key":"msuchanek@suse.de","avatar":"https://avatars.githubusercontent.com/u/787652?v=4"},"body":"On Wed, 20 Feb 2019 11:55:46 -0500\nEric Sunshine wrote:\n\n> On Wed, Feb 20, 2019 at 11:17 AM Michal Suchanek wrote:\n> > Apparently it can happen that stat() claims there is a commondir file but when\n> > trying to open the file it is missing. \n> \n> Under what circumstances?\n\nI would like to know that as well. The only command tested was worktree\nadd which should not remove the file. Nonetheless running many woktree\nadd commands in parallel can cause the file to go away for some of\nthem. For many commands git calls itself recursively so there is\nprobably much more going on than the single function that creates the\nworktree.\n\n> \n> > Another even rarer issue is that the file might be zero size because another\n> > process initializing a worktree opened the file but has not written is content\n> > yet. \n> \n> Based upon the explanation thus far, I'm having trouble understanding\n> under what circumstances these race conditions can arise. Are you\n> trying to invoke Git commands in a particular worktree even as the\n> worktree itself is being created?\n\nIt's explained in the following paragraph. If you have multiple\nworktrees some *other* worktreee may be uninitialized.\n\n> \n> Without this information being spelled out clearly, it is going to be\n> difficult for someone in the future to reason about why the code is\n> the way it is following this change.\n> \n> > When any of this happnes git aborts failing to perform perfectly valid\n> > command because unrelated worktree is not yet fully initialized. \n> \n> s/happnes/happens/\n> \n> > Rather than testing if the file exists before reading it handle ENOENT\n> > and ENOTDIR. \n> \n> One more comment below...\n> \n> > Signed-off-by: Michal Suchanek \n> > ---\n> > diff --git a/setup.c b/setup.c\n> > @@ -270,12 +270,20 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)\n> > {\n> > strbuf_addf(&path, \"%s/commondir\", gitdir);\n> > - if (file_exists(path.buf)) {\n> > - if (strbuf_read_file(&data, path.buf, 0) <= 0)\n> > + ret = strbuf_read_file(&data, path.buf, 0);\n> > + if (ret <= 0) {\n> > + /*\n> > + * if file is missing or zero size (just being written)\n> > + * assume default, bail otherwise\n> > + */\n> > + if (ret && errno != ENOENT && errno != ENOTDIR)\n> > die_errno(_(\"failed to read %s\"), path.buf); \n> \n> It's not clear from the explanation given in the commit message if the\n> new behavior is indeed sensible. The original intent of the code, as I\n> understand it, is to validate \"commondir\", to ensure that it is not\n> somehow corrupt (such as the user editing it and making it empty).\n\nHow is it validated in the code below when it is non-zero size?\n\nThere is *no* validation whatsoever. Yet zero size is somehow totally\nunacceptable and requires that git working in *any* worktree aborts if\ncommondir file in *any* worktree is zero size.\n\n> Following this change, that particular validation no longer takes\n> place. But, more importantly, what does it mean to fall back to\n> \"default\" for this particular worktree? I'm having trouble\n> understanding how the new behavior can be correct or desirable. (Am I\n> missing something obvious?)\n\nIf the file can be missing altogether and it is not an error how it is\nincorrect or undesirable to ignore zero size file?\n\nThanks\n\nMichal\n"},{"id":"369729","messageId":"20190220182922.21693fa7@kitsune.suse.cz","threadId":"50535","inReplyTo":"CAPig+cSdA8XRwCJQD3o6DZLwesBLRTys7OV6u0wy9Ve3Hp6XPA@mail.gmail.com","subject":"Re: [PATCH v3 1/2] worktree: fix worktree add race.","fromName":"Michal Suchánek","fromEmail":"msuchanek@suse.de","sentAt":"2019-02-20T17:29:22Z","receivedAt":"2019-02-20T17:29:32Z","isPatch":true,"sender":{"key":"msuchanek@suse.de","avatar":"https://avatars.githubusercontent.com/u/787652?v=4"},"body":"On Wed, 20 Feb 2019 11:34:54 -0500\nEric Sunshine wrote:\n\n> On Wed, Feb 20, 2019 at 11:17 AM Michal Suchanek wrote:\n> > Git runs a stat loop to find a worktree name that's available and then does\n> > mkdir on the found name. Turn it to mkdir loop to avoid another invocation of\n> > worktree add finding the same free name and creating the directory first.\n> >\n> > Signed-off-by: Michal Suchanek \n> > ---\n> > diff --git a/builtin/worktree.c b/builtin/worktree.c\n> > @@ -295,8 +295,12 @@ static int add_worktree(const char *path, const char *refname,\n> > if (safe_create_leading_directories_const(sb_repo.buf))\n> > die_errno(_(\"could not create leading directories of '%s'\"),\n> > sb_repo.buf);\n> > - while (!stat(sb_repo.buf, &st)) {\n> > + while (mkdir(sb_repo.buf, 0777)) {\n> > counter++;\n> > + if ((errno != EEXIST) || !counter /* overflow */)\n> > + die_errno(_(\"could not create directory of '%s'\"),\n> > + sb_repo.buf);\n> > strbuf_setlen(&sb_repo, len);\n> > strbuf_addf(&sb_repo, \"%d\", counter);\n> > }\n> > @@ -306,8 +310,6 @@ static int add_worktree(const char *path, const char *refname,\n> > atexit(remove_junk);\n> > sigchain_push_common(remove_junk_on_signal);\n> > - if (mkdir(sb_repo.buf, 0777))\n> > - die_errno(_(\"could not create directory of '%s'\"), sb_repo.buf);\n> > junk_git_dir = xstrdup(sb_repo.buf);\n> > is_junk = 1; \n> \n> Did you audit this \"junk\" handling to verify that stuff which ought to\n> be cleaned up still is cleaned up now that the mkdir() and die() have\n> been moved above the atexit(remove_junk) invocation?\n> \n> I did just audit it, and I _think_ that it still works as expected,\n> but it would be good to hear that someone else has come to the same\n> conclusion.\n\nThe die() is executed only when mkdir() fails so there is no junk to\nclean up in that case.\n\nThanks\n\nMichal\n"},{"id":"369731","messageId":"CAPig+cS4vZpyj4Cx=Q89v3xTrCG4WbtX8EhTfOT2RKytjV-HrA@mail.gmail.com","threadId":"50535","inReplyTo":"20190220181605.60bbc28d@kitsune.suse.cz","subject":"Re: [PATCH v3 2/2] setup: don't fail if commondir reference is deleted.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-02-20T18:35:57Z","receivedAt":"2019-02-20T18:36:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 20, 2019 at 12:16 PM Michal Suchánek wrote:\n> On Wed, 20 Feb 2019 11:55:46 -0500\n> Eric Sunshine wrote:\n> > On Wed, Feb 20, 2019 at 11:17 AM Michal Suchanek wrote:\n> > > Apparently it can happen that stat() claims there is a commondir file but when\n> > > trying to open the file it is missing.\n> >\n> > Under what circumstances?\n>\n> I would like to know that as well. The only command tested was worktree\n> add which should not remove the file. Nonetheless running many woktree\n> add commands in parallel can cause the file to go away for some of\n> them.\n\nYou actually encountered this particular error message, correct? Was\nthat before or after you fixed the race in builtin/worktree.c itself\nvia patch 1/2? Did the reported 'errno' indicate that the file did not\nexist or was it some other error?\n\n> For many commands git calls itself recursively so there is\n> probably much more going on than the single function that creates the\n> worktree.\n\n\"git worktree add\" is careful to invoke other Git commands only after\n\"commondir\" exists, so it's not clear how this circumstance arises if\nthe file is indeed missing by the time the other Git command is run.\n\n> > > Another even rarer issue is that the file might be zero size because another\n> > > process initializing a worktree opened the file but has not written is content\n> > > yet.\n> >\n> > Based upon the explanation thus far, I'm having trouble understanding\n> > under what circumstances these race conditions can arise. Are you\n> > trying to invoke Git commands in a particular worktree even as the\n> > worktree itself is being created?\n>\n> It's explained in the following paragraph. If you have multiple\n> worktrees some *other* worktreee may be uninitialized.\n\nI understand that, but setup.c:get_common_dir_noenv() is concerned\nonly with _this_ worktree -- the one in which the Git command is being\nrun -- so it's not clear if or how some other partially-initialized\nworktree could have any impact. (And, I'm having trouble fathoming how\nit could, which is why I'm asking these questions).\n\nIs it possible that when you saw that error message, it actually arose\nfrom some code other than setup.c:get_common_dir_noenv()?\n\n> > > - if (file_exists(path.buf)) {\n> > > - if (strbuf_read_file(&data, path.buf, 0) <= 0)\n> > > + ret = strbuf_read_file(&data, path.buf, 0);\n> > > + if (ret <= 0) {\n> > > + /*\n> > > + * if file is missing or zero size (just being written)\n> > > + * assume default, bail otherwise\n> > > + */\n> > > + if (ret && errno != ENOENT && errno != ENOTDIR)\n> > > die_errno(_(\"failed to read %s\"), path.buf);\n> >\n> > It's not clear from the explanation given in the commit message if the\n> > new behavior is indeed sensible. The original intent of the code, as I\n> > understand it, is to validate \"commondir\", to ensure that it is not\n> > somehow corrupt (such as the user editing it and making it empty).\n>\n> How is it validated in the code below when it is non-zero size?\n\nChecking whether the file has content _is_ a form of validation, even\nif not extensive validation.\n\n> There is *no* validation whatsoever. Yet zero size is somehow totally\n> unacceptable and requires that git working in *any* worktree aborts if\n> commondir file in *any* worktree is zero size.\n\nAs noted above, it's not clear from the commit message how this case\ncan arise given that setup.c:get_common_dir_noenv() is presumably\nconcerned with and only consults _this_ worktree, so I'm having\ntrouble understanding how the state of other worktrees could impact\nit.\n\n> > Following this change, that particular validation no longer takes\n> > place. But, more importantly, what does it mean to fall back to\n> > \"default\" for this particular worktree? I'm having trouble\n> > understanding how the new behavior can be correct or desirable. (Am I\n> > missing something obvious?)\n>\n> If the file can be missing altogether and it is not an error how it is\n> incorrect or undesirable to ignore zero size file?\n\nBecause the _presence_ of that file indicates a linked worktree,\nwhereas it's absence indicates the main worktree. If the file is\npresent but empty, then that is an abnormal condition, i.e. some form\nof corruption.\n\nThe difference is significant, and that's why I'm asking if the new\nbehavior is correct or desirable. If you start interpreting this\nabnormal condition as a non-error, then get_common_dir_noenv() will be\nreporting that this is the main worktree when in fact it is (a somehow\ncorrupted) linked worktree. Such false reporting could trigger\nundesirable and outright wrong behavior in callers.\n"},{"id":"369761","messageId":"CAPig+cT48d9JJyqVx0WvBiFV+BLqAqo5dX3yndNhoJZmKRPgEg@mail.gmail.com","threadId":"50535","inReplyTo":"CAPig+cS4vZpyj4Cx=Q89v3xTrCG4WbtX8EhTfOT2RKytjV-HrA@mail.gmail.com","subject":"Re: [PATCH v3 2/2] setup: don't fail if commondir reference is deleted.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-02-21T09:27:21Z","receivedAt":"2019-02-21T09:27:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 20, 2019 at 1:35 PM Eric Sunshine wrote:\n> On Wed, Feb 20, 2019 at 12:16 PM Michal Suchánek wrote:\n> > On Wed, 20 Feb 2019 11:55:46 -0500\n> > Eric Sunshine wrote:\n> > > On Wed, Feb 20, 2019 at 11:17 AM Michal Suchanek wrote:\n> > > > Another even rarer issue is that the file might be zero size because another\n> > > > process initializing a worktree opened the file but has not written is content\n> > > > yet.\n> > >\n> > > Based upon the explanation thus far, I'm having trouble understanding\n> > > under what circumstances these race conditions can arise. Are you\n> > > trying to invoke Git commands in a particular worktree even as the\n> > > worktree itself is being created?\n> >\n> > It's explained in the following paragraph. If you have multiple\n> > worktrees some *other* worktreee may be uninitialized.\n>\n> I understand that, but setup.c:get_common_dir_noenv() is concerned\n> only with _this_ worktree -- the one in which the Git command is being\n> run -- so it's not clear if or how some other partially-initialized\n> worktree could have any impact. (And, I'm having trouble fathoming how\n> it could, which is why I'm asking these questions).\n\nI still can't see how setup.c:get_common_dir_noenv() could be\nresponsible for the behavior you're describing of _any_ Git command\nerroring out due to _any_ worktree being incompletely-initialized.\nHowever, I can imagine \"git worktree add\" itself being racy and\nfailing due to a missing or empty \"commondir\" file for some other\nworktree since that command _does_ consult other worktree entries when\nvalidating the \"add\" operation via\nbuiltin/worktree.c:validate_worktree_add() which calls\nget_worktrees(). If get_worktrees() is subject to that raciness\nproblem, then \"git worktree add\" will inherit that undesirable\nraciness behavior (as will other \"git worktree\" commands which call\nget_worktrees(), such as \"git worktree list\").\n\n> Is it possible that when you saw that error message, it actually arose\n> from some code other than setup.c:get_common_dir_noenv()?\n\nSo, I'm suspecting get_worktrees() or some function it calls (and so\non) as the racy culprit.\n"},{"id":"369766","messageId":"CACsJy8AWezO7TFq8ne1a2pSAJZoc6oYqnNNxmVW_FkA9--ntbQ@mail.gmail.com","threadId":"50535","inReplyTo":"6f9c8775817117c2b36539eb048e2462a650ab8f.1550508544.git.msuchanek@suse.de","subject":"Re: [PATCH 2/2] setup: don't fail if commondir reference is deleted.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-21T10:50:38Z","receivedAt":"2019-02-21T10:51:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Feb 19, 2019 at 12:05 AM Michal Suchanek wrote:\n>\n> When adding wotktrees git can die in get_common_dir_noenv while\n> examining existing worktrees because the commondir file does not exist.\n> Rather than testing if the file exists before reading it handle ENOENT.\n\nI don't think we could go around fixing every access to incomplete\nworktrees like this. If this is because of racy 'worktree add', then\nperhaps a better solution is make it absolutely clear it's not ready\nfor anybody to access.\n\nFor example, we can suffix the worktree directory name with \".lock\"\nand make sure get_worktrees() ignores entries ending with \".lock\".\nThat should protect other commands while 'worktree add' is still\nrunning. Only when the worktree is complete that 'worktree add' should\nrename the directory to lose \".lock\" and run external commands like\ngit-checkout to populate the worktree.\n\n> Signed-off-by: Michal Suchanek \n> ---\n> v2:\n> - do not test file existence first, just read it and handle ENOENT.\n> - handle zero size file correctly\n> ---\n> setup.c | 16 +++++++++++-----\n> 1 file changed, 11 insertions(+), 5 deletions(-)\n>\n> diff --git a/setup.c b/setup.c\n> index ca9e8a949ed8..dd865f280d34 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -270,12 +270,20 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)\n> {\n> struct strbuf data = STRBUF_INIT;\n> struct strbuf path = STRBUF_INIT;\n> - int ret = 0;\n> + int ret;\n>\n> strbuf_addf(&path, \"%s/commondir\", gitdir);\n> - if (file_exists(path.buf)) {\n> - if (strbuf_read_file(&data, path.buf, 0) <= 0)\n> + ret = strbuf_read_file(&data, path.buf, 0);\n> + if (ret <= 0) {\n> + /*\n> + * if file is missing or zero size (just being written)\n> + * assume default, bail otherwise\n> + */\n> + if (ret && errno != ENOENT)\n> die_errno(_(\"failed to read %s\"), path.buf);\n> + strbuf_addstr(sb, gitdir);\n> + ret = 0;\n> + } else {\n> while (data.len && (data.buf[data.len - 1] == '\\n' ||\n> data.buf[data.len - 1] == '\\r'))\n> data.len--;\n> @@ -286,8 +294,6 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)\n> strbuf_addbuf(&path, &data);\n> strbuf_add_real_path(sb, path.buf);\n> ret = 1;\n> - } else {\n> - strbuf_addstr(sb, gitdir);\n> }\n>\n> strbuf_release(&data);\n> --\n> 2.20.1\n>\n\n\n-- \nDuy\n"},{"id":"369769","messageId":"20190221121350.28015371@naga.suse.cz","threadId":"50535","inReplyTo":"CAPig+cT48d9JJyqVx0WvBiFV+BLqAqo5dX3yndNhoJZmKRPgEg@mail.gmail.com","subject":"Re: [PATCH v3 2/2] setup: don't fail if commondir reference is deleted.","fromName":"Michal Suchánek","fromEmail":"msuchanek@suse.de","sentAt":"2019-02-21T11:13:50Z","receivedAt":"2019-02-21T11:13:55Z","isPatch":true,"sender":{"key":"msuchanek@suse.de","avatar":"https://avatars.githubusercontent.com/u/787652?v=4"},"body":"On Thu, 21 Feb 2019 04:27:21 -0500\nEric Sunshine wrote:\n\n> On Wed, Feb 20, 2019 at 1:35 PM Eric Sunshine wrote:\n> > On Wed, Feb 20, 2019 at 12:16 PM Michal Suchánek wrote: \n> > > On Wed, 20 Feb 2019 11:55:46 -0500\n> > > Eric Sunshine wrote: \n> > > > On Wed, Feb 20, 2019 at 11:17 AM Michal Suchanek wrote: \n> > > > > Another even rarer issue is that the file might be zero size because another\n> > > > > process initializing a worktree opened the file but has not written is content\n> > > > > yet. \n> > > >\n> > > > Based upon the explanation thus far, I'm having trouble understanding\n> > > > under what circumstances these race conditions can arise. Are you\n> > > > trying to invoke Git commands in a particular worktree even as the\n> > > > worktree itself is being created? \n> > >\n> > > It's explained in the following paragraph. If you have multiple\n> > > worktrees some *other* worktreee may be uninitialized. \n> >\n> > I understand that, but setup.c:get_common_dir_noenv() is concerned\n> > only with _this_ worktree -- the one in which the Git command is being\n> > run -- so it's not clear if or how some other partially-initialized\n> > worktree could have any impact. (And, I'm having trouble fathoming how\n> > it could, which is why I'm asking these questions). \n> \n> I still can't see how setup.c:get_common_dir_noenv() could be\n> responsible for the behavior you're describing of _any_ Git command\n> erroring out due to _any_ worktree being incompletely-initialized.\n> However, I can imagine \"git worktree add\" itself being racy and\n> failing due to a missing or empty \"commondir\" file for some other\n> worktree since that command _does_ consult other worktree entries when\n> validating the \"add\" operation via\n> builtin/worktree.c:validate_worktree_add() which calls\n> get_worktrees(). If get_worktrees() is subject to that raciness\n> problem, then \"git worktree add\" will inherit that undesirable\n> raciness behavior (as will other \"git worktree\" commands which call\n> get_worktrees(), such as \"git worktree list\").\n> \n> > Is it possible that when you saw that error message, it actually arose\n> > from some code other than setup.c:get_common_dir_noenv()? \n> \n> So, I'm suspecting get_worktrincludes both itees() or some function it calls (and so\n> on) as the racy culprit.\n\nYes, that's my explanation for the situation as well.\n\nThanks\n\nMichal\n"},{"id":"369792","messageId":"20190221121950.12d24e53@naga.suse.cz","threadId":"50535","inReplyTo":"CAPig+cS4vZpyj4Cx=Q89v3xTrCG4WbtX8EhTfOT2RKytjV-HrA@mail.gmail.com","subject":"Re: [PATCH v3 2/2] setup: don't fail if commondir reference is deleted.","fromName":"Michal Suchánek","fromEmail":"msuchanek@suse.de","sentAt":"2019-02-21T11:19:50Z","receivedAt":"2019-02-21T11:19:54Z","isPatch":true,"sender":{"key":"msuchanek@suse.de","avatar":"https://avatars.githubusercontent.com/u/787652?v=4"},"body":"On Wed, 20 Feb 2019 13:35:57 -0500\nEric Sunshine wrote:\n\n> On Wed, Feb 20, 2019 at 12:16 PM Michal Suchánek wrote:\n> > On Wed, 20 Feb 2019 11:55:46 -0500\n> > Eric Sunshine wrote: \n\n> > > Following this change, that particular validation no longer takes\n> > > place. But, more importantly, what does it mean to fall back to\n> > > \"default\" for this particular worktree? I'm having trouble\n> > > understanding how the new behavior can be correct or desirable. (Am I\n> > > missing something obvious?) \n> >\n> > If the file can be missing altogether and it is not an error how it is\n> > incorrect or undesirable to ignore zero size file? \n> \n> Because the _presence_ of that file indicates a linked worktree,\n> whereas it's absence indicates the main worktree. If the file is\n> present but empty, then that is an abnormal condition, i.e. some form\n> of corruption.\n> \n> The difference is significant, and that's why I'm asking if the new\n> behavior is correct or desirable. If you start interpreting this\n> abnormal condition as a non-error, then get_common_dir_noenv() will be\n> reporting that this is the main worktree when in fact it is (a somehow\n> corrupted) linked worktree. Such false reporting could trigger\n> undesirable and outright wrong behavior in callers.\n\nThis is not an issue introduced with this patch, however. The worktree\nis not initialized atomically. First the worktree directory is created\nand then it is populated with content including the commondir reference.\n\nBecause there is no big repository lock that everyone takes to access\na repository other running git processes can see the wotktree without\nthe commondir file. \n\nThe way this is mitigated in users of get_worktrees() is an assumption\nthat the first worktree is the main worktree.\n\nIf this is sufficient is not something this patchset aims to address.\nIt merely addresses get_worktrees() aborting due to hitting specific\nstage in the initialization of a worktree.\n\nThanks\n\nMichal\n"},{"id":"369810","messageId":"20190221145056.53b98b2a@kitsune.suse.cz","threadId":"50535","inReplyTo":"CACsJy8AWezO7TFq8ne1a2pSAJZoc6oYqnNNxmVW_FkA9--ntbQ@mail.gmail.com","subject":"Re: [PATCH 2/2] setup: don't fail if commondir reference is deleted.","fromName":"Michal Suchánek","fromEmail":"msuchanek@suse.de","sentAt":"2019-02-21T13:50:56Z","receivedAt":"2019-02-21T13:51:01Z","isPatch":true,"sender":{"key":"msuchanek@suse.de","avatar":"https://avatars.githubusercontent.com/u/787652?v=4"},"body":"On Thu, 21 Feb 2019 17:50:38 +0700\nDuy Nguyen wrote:\n\n> On Tue, Feb 19, 2019 at 12:05 AM Michal Suchanek wrote:\n> >\n> > When adding wotktrees git can die in get_common_dir_noenv while\n> > examining existing worktrees because the commondir file does not exist.\n> > Rather than testing if the file exists before reading it handle ENOENT. \n> \n> I don't think we could go around fixing every access to incomplete\n> worktrees like this. If this is because of racy 'worktree add', then\n> perhaps a better solution is make it absolutely clear it's not ready\n> for anybody to access.\n> \n> For example, we can suffix the worktree directory name with \".lock\"\n> and make sure get_worktrees() ignores entries ending with \".lock\".\n> That should protect other commands while 'worktree add' is still\n> running. Only when the worktree is complete that 'worktree add' should\n> rename the directory to lose \".lock\" and run external commands like\n> git-checkout to populate the worktree.\n\nThe problem is we don't forbid worktree names ending with \".lock\".\nWhich means that if we start to forbid them now existing worktrees\nmight become inaccessible.\n\nThanks\n\nMichal\n"},{"id":"369820","messageId":"adc0f7f9-aa41-780e-6fce-94d493fac318@talktalk.net","threadId":"50535","inReplyTo":"20190221145056.53b98b2a@kitsune.suse.cz","subject":"Re: [PATCH 2/2] setup: don't fail if commondir reference is deleted.","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2019-02-21T17:07:50Z","receivedAt":"2019-02-21T17:07:57Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Michal/Duy\n\nOn 21/02/2019 13:50, Michal Suchánek wrote:\n> On Thu, 21 Feb 2019 17:50:38 +0700\n> Duy Nguyen wrote:\n> \n>> On Tue, Feb 19, 2019 at 12:05 AM Michal Suchanek wrote:\n>>>\n>>> When adding wotktrees git can die in get_common_dir_noenv while\n>>> examining existing worktrees because the commondir file does not exist.\n>>> Rather than testing if the file exists before reading it handle ENOENT.\n>>\n>> I don't think we could go around fixing every access to incomplete\n>> worktrees like this. If this is because of racy 'worktree add', then\n>> perhaps a better solution is make it absolutely clear it's not ready\n>> for anybody to access.\n>>\n>> For example, we can suffix the worktree directory name with \".lock\"\n>> and make sure get_worktrees() ignores entries ending with \".lock\".\n>> That should protect other commands while 'worktree add' is still\n>> running. Only when the worktree is complete that 'worktree add' should\n>> rename the directory to lose \".lock\" and run external commands like\n>> git-checkout to populate the worktree.\n> \n> The problem is we don't forbid worktree names ending with \".lock\".\n> Which means that if we start to forbid them now existing worktrees\n> might become inaccessible.\n\nI think it is also racy as the renaming breaks the use of mkdir erroring \nout if the directory already exists. One solution is to have a lock \nentry in $GIT_COMMON_DIR/worktree-locks and make sure the code that \niterates over the entries in $GIT_COMMON_DIR/worktrees skips any that \nhave a corresponding ignores in $GIT_COMMON_DIR/worktree-locks. If the \nworktree-locks/ is created before worktree/ then it should be \nrace free (you will have to remove the lock if the real entry cannot be \ncreated and then increment the counter and try again). Entries could \nalso be locked on removal to prevent a race there.\n\nBest Wishes\n\nPhillip\n\n> Thanks\n> \n> Michal\n> \n\n"},{"id":"369821","messageId":"CAPig+cTQMZFF-oX-SOzB5JR=V1WThBihC+kNm-2wjbpAWf-OHA@mail.gmail.com","threadId":"50535","inReplyTo":"adc0f7f9-aa41-780e-6fce-94d493fac318@talktalk.net","subject":"Re: [PATCH 2/2] setup: don't fail if commondir reference is deleted.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-02-21T17:12:28Z","receivedAt":"2019-02-21T17:12:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Feb 21, 2019 at 12:07 PM Phillip Wood wrote:\n> On 21/02/2019 13:50, Michal Suchánek wrote:\n> >> On Tue, Feb 19, 2019 at 12:05 AM Michal Suchanek wrote:\n> > The problem is we don't forbid worktree names ending with \".lock\".\n> > Which means that if we start to forbid them now existing worktrees\n> > might become inaccessible.\n>\n> I think it is also racy as the renaming breaks the use of mkdir erroring\n> out if the directory already exists. One solution is to have a lock\n> entry in $GIT_COMMON_DIR/worktree-locks and make sure the code that\n> iterates over the entries in $GIT_COMMON_DIR/worktrees skips any that\n> have a corresponding ignores in $GIT_COMMON_DIR/worktree-locks. If the\n> worktree-locks/ is created before worktree/ then it should be\n> race free (you will have to remove the lock if the real entry cannot be\n> created and then increment the counter and try again). Entries could\n> also be locked on removal to prevent a race there.\n\nI wonder, though, how much this helps or hinders the use-case which\nprompted this patch series in the first place; to wit, creating\nhundreds or thousands of worktrees. Doing so serially was too slow, so\nthe many \"git worktree add\" invocations were instead run in parallel\n(which led to \"discovery\" of race conditions). Using a global worktree\nlock would serialize worktree creation, thus slowing it down once\nagain.\n"},{"id":"369822","messageId":"4309841e-2b98-22d4-505e-1b9ea2f5e3bb@talktalk.net","threadId":"50535","inReplyTo":"CAPig+cTQMZFF-oX-SOzB5JR=V1WThBihC+kNm-2wjbpAWf-OHA@mail.gmail.com","subject":"Re: [PATCH 2/2] setup: don't fail if commondir reference is deleted.","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2019-02-21T17:27:04Z","receivedAt":"2019-02-21T17:27:11Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Eric\n\nOn 21/02/2019 17:12, Eric Sunshine wrote:\n> On Thu, Feb 21, 2019 at 12:07 PM Phillip Wood wrote:\n>> On 21/02/2019 13:50, Michal Suchánek wrote:\n>>>> On Tue, Feb 19, 2019 at 12:05 AM Michal Suchanek wrote:\n>>> The problem is we don't forbid worktree names ending with \".lock\".\n>>> Which means that if we start to forbid them now existing worktrees\n>>> might become inaccessible.\n>>\n>> I think it is also racy as the renaming breaks the use of mkdir erroring\n>> out if the directory already exists. One solution is to have a lock\n>> entry in $GIT_COMMON_DIR/worktree-locks and make sure the code that\n>> iterates over the entries in $GIT_COMMON_DIR/worktrees skips any that\n>> have a corresponding ignores in $GIT_COMMON_DIR/worktree-locks. If the\n>> worktree-locks/ is created before worktree/ then it should be\n>> race free (you will have to remove the lock if the real entry cannot be\n>> created and then increment the counter and try again). Entries could\n>> also be locked on removal to prevent a race there.\n> \n> I wonder, though, how much this helps or hinders the use-case which\n> prompted this patch series in the first place; to wit, creating\n> hundreds or thousands of worktrees. Doing so serially was too slow, so\n> the many \"git worktree add\" invocations were instead run in parallel\n> (which led to \"discovery\" of race conditions). Using a global worktree\n> lock would serialize worktree creation, thus slowing it down once\n> again.\n\nThe idea is that there are per-worktree lock stored under worktree-locks \n(hence the plural name). Using a separate directory for the locks gets \nround the problems of name clashes between the lock for a worktree \ncalled foo and one called foo.lock and means we can rely on mkdir \nerroring out if the worktree name already exists as there is no renaming.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"369824","messageId":"20190221183348.30f759c2@kitsune.suse.cz","threadId":"50535","inReplyTo":"CAPig+cTQMZFF-oX-SOzB5JR=V1WThBihC+kNm-2wjbpAWf-OHA@mail.gmail.com","subject":"Re: [PATCH 2/2] setup: don't fail if commondir reference is deleted.","fromName":"Michal Suchánek","fromEmail":"msuchanek@suse.de","sentAt":"2019-02-21T17:33:48Z","receivedAt":"2019-02-21T17:33:51Z","isPatch":true,"sender":{"key":"msuchanek@suse.de","avatar":"https://avatars.githubusercontent.com/u/787652?v=4"},"body":"On Thu, 21 Feb 2019 12:12:28 -0500\nEric Sunshine wrote:\n\n> On Thu, Feb 21, 2019 at 12:07 PM Phillip Wood wrote:\n> > On 21/02/2019 13:50, Michal Suchánek wrote: \n> > >> On Tue, Feb 19, 2019 at 12:05 AM Michal Suchanek wrote: \n> > > The problem is we don't forbid worktree names ending with \".lock\".\n> > > Which means that if we start to forbid them now existing worktrees\n> > > might become inaccessible. \n> >\n> > I think it is also racy as the renaming breaks the use of mkdir erroring\n> > out if the directory already exists. One solution is to have a lock\n> > entry in $GIT_COMMON_DIR/worktree-locks and make sure the code that\n> > iterates over the entries in $GIT_COMMON_DIR/worktrees skips any that\n> > have a corresponding ignores in $GIT_COMMON_DIR/worktree-locks. If the\n> > worktree-locks/ is created before worktree/ then it should be\n> > race free (you will have to remove the lock if the real entry cannot be\n> > created and then increment the counter and try again). Entries could\n> > also be locked on removal to prevent a race there. \n> \n> I wonder, though, how much this helps or hinders the use-case which\n> prompted this patch series in the first place; to wit, creating\n> hundreds or thousands of worktrees. Doing so serially was too slow, so\n> the many \"git worktree add\" invocations were instead run in parallel\n> (which led to \"discovery\" of race conditions). Using a global worktree\n> lock would serialize worktree creation, thus slowing it down once\n> again.\n\nI created thousands of worktrees only for stress-testing. The real\nworkload needs only a dozen of them. That still leads to hitting a\nrace condition occasionally and automation failure.\n\nCreating a separate lock directory will probably work. The question is\nwhen do you need to take the lock. Before adding a worktree, sure.\nBefore deleting it as well. The problem is that deleting a worktree\nsuccessfully without creating some broken state needs to exclude\nprocesses that might add stuff in the worktree directory. How many\noperations then do *not* need to take the lock?\n\nThanks\n\nMichal\n"},{"id":"369893","messageId":"CACsJy8CGtYe-d_0gXg4HLqvE6eR62yXsX-abz8NZ-Ln_+VdUqQ@mail.gmail.com","threadId":"50535","inReplyTo":"20190221145056.53b98b2a@kitsune.suse.cz","subject":"Re: [PATCH 2/2] setup: don't fail if commondir reference is deleted.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-22T09:26:10Z","receivedAt":"2019-02-22T09:26:39Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Feb 21, 2019 at 8:50 PM Michal Suchánek wrote:\n>\n> On Thu, 21 Feb 2019 17:50:38 +0700\n> Duy Nguyen wrote:\n>\n> > On Tue, Feb 19, 2019 at 12:05 AM Michal Suchanek wrote:\n> > >\n> > > When adding wotktrees git can die in get_common_dir_noenv while\n> > > examining existing worktrees because the commondir file does not exist.\n> > > Rather than testing if the file exists before reading it handle ENOENT.\n> >\n> > I don't think we could go around fixing every access to incomplete\n> > worktrees like this. If this is because of racy 'worktree add', then\n> > perhaps a better solution is make it absolutely clear it's not ready\n> > for anybody to access.\n> >\n> > For example, we can suffix the worktree directory name with \".lock\"\n> > and make sure get_worktrees() ignores entries ending with \".lock\".\n> > That should protect other commands while 'worktree add' is still\n> > running. Only when the worktree is complete that 'worktree add' should\n> > rename the directory to lose \".lock\" and run external commands like\n> > git-checkout to populate the worktree.\n>\n> The problem is we don't forbid worktree names ending with \".lock\".\n> Which means that if we start to forbid them now existing worktrees\n> might become inaccessible.\n\nWorktrees ending with .lock will not work well now anyway. While [1]\nreports the problem with worktree names having a whitespace, \".lock\"\nis in the same class (not a valid refname) and will result the same\nerror. So if you have \"*.lock\" worktrees now you're already in\ntrouble.\n\n[1] https://public-inbox.org/git/1550673274.30738.0@yandex.ru/T/#m9d86e0a388fd4961bc102c2c69e8bc3b2db07a42\n-- \nDuy\n"},{"id":"369894","messageId":"CACsJy8B2HRyBKQd+S7hjfU+xGFH+_y0YKcw8397znc2eGUBogQ@mail.gmail.com","threadId":"50535","inReplyTo":"adc0f7f9-aa41-780e-6fce-94d493fac318@talktalk.net","subject":"Re: [PATCH 2/2] setup: don't fail if commondir reference is deleted.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-02-22T09:32:50Z","receivedAt":"2019-02-22T09:33:19Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Feb 22, 2019 at 12:07 AM Phillip Wood wrote:\n>\n> Hi Michal/Duy\n>\n> On 21/02/2019 13:50, Michal Suchánek wrote:\n> > On Thu, 21 Feb 2019 17:50:38 +0700\n> > Duy Nguyen wrote:\n> >\n> >> On Tue, Feb 19, 2019 at 12:05 AM Michal Suchanek wrote:\n> >>>\n> >>> When adding wotktrees git can die in get_common_dir_noenv while\n> >>> examining existing worktrees because the commondir file does not exist.\n> >>> Rather than testing if the file exists before reading it handle ENOENT.\n> >>\n> >> I don't think we could go around fixing every access to incomplete\n> >> worktrees like this. If this is because of racy 'worktree add', then\n> >> perhaps a better solution is make it absolutely clear it's not ready\n> >> for anybody to access.\n> >>\n> >> For example, we can suffix the worktree directory name with \".lock\"\n> >> and make sure get_worktrees() ignores entries ending with \".lock\".\n> >> That should protect other commands while 'worktree add' is still\n> >> running. Only when the worktree is complete that 'worktree add' should\n> >> rename the directory to lose \".lock\" and run external commands like\n> >> git-checkout to populate the worktree.\n> >\n> > The problem is we don't forbid worktree names ending with \".lock\".\n> > Which means that if we start to forbid them now existing worktrees\n> > might become inaccessible.\n>\n> I think it is also racy as the renaming breaks the use of mkdir erroring\n> out if the directory already exists.\n\nYou mean the part where we see \"fred\" exists and decide to try the\nname \"fred1\" instead (i.e. patch 1/2)?\n\nI don't think it's the problem if that's the case. We mkdir\n\"fred.lock\" _then_ check if \"fred\" exists. If it does, remove\nfred.lock and move on to fred1.lock. Then we rename fred1.lock to\nfred1 and error out if rename fails.\n-- \nDuy\n"},{"id":"369900","messageId":"1e0e8a07-f310-adc7-0538-c1b738da0e98@talktalk.net","threadId":"50535","inReplyTo":"CACsJy8B2HRyBKQd+S7hjfU+xGFH+_y0YKcw8397znc2eGUBogQ@mail.gmail.com","subject":"Re: [PATCH 2/2] setup: don't fail if commondir reference is deleted.","fromName":"Phillip Wood","fromEmail":"phillip.wood@talktalk.net","sentAt":"2019-02-22T10:20:27Z","receivedAt":"2019-02-22T10:20:35Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Duy\n\nOn 22/02/2019 09:32, Duy Nguyen wrote:\n> On Fri, Feb 22, 2019 at 12:07 AM Phillip Wood wrote:\n>>\n>> Hi Michal/Duy\n>>\n>> On 21/02/2019 13:50, Michal Suchánek wrote:\n>>> On Thu, 21 Feb 2019 17:50:38 +0700\n>>> Duy Nguyen wrote:\n>>>\n>>>> On Tue, Feb 19, 2019 at 12:05 AM Michal Suchanek wrote:\n>>>>>\n>>>>> When adding wotktrees git can die in get_common_dir_noenv while\n>>>>> examining existing worktrees because the commondir file does not exist.\n>>>>> Rather than testing if the file exists before reading it handle ENOENT.\n>>>>\n>>>> I don't think we could go around fixing every access to incomplete\n>>>> worktrees like this. If this is because of racy 'worktree add', then\n>>>> perhaps a better solution is make it absolutely clear it's not ready\n>>>> for anybody to access.\n>>>>\n>>>> For example, we can suffix the worktree directory name with \".lock\"\n>>>> and make sure get_worktrees() ignores entries ending with \".lock\".\n>>>> That should protect other commands while 'worktree add' is still\n>>>> running. Only when the worktree is complete that 'worktree add' should\n>>>> rename the directory to lose \".lock\" and run external commands like\n>>>> git-checkout to populate the worktree.\n>>>\n>>> The problem is we don't forbid worktree names ending with \".lock\".\n>>> Which means that if we start to forbid them now existing worktrees\n>>> might become inaccessible.\n>>\n>> I think it is also racy as the renaming breaks the use of mkdir erroring\n>> out if the directory already exists.\n> \n> You mean the part where we see \"fred\" exists and decide to try the\n> name \"fred1\" instead (i.e. patch 1/2)?\n> \n> I don't think it's the problem if that's the case. We mkdir\n> \"fred.lock\" _then_ check if \"fred\" exists. If it does, remove\n> fred.lock and move on to fred1.lock. Then we rename fred1.lock to\n> fred1 and error out if rename fails.\n\nAh you're right, if another process tries to create fred.lock as we're \nrenaming it either their mkdir fred.lock will fail or they'll see fred \nonce they've made fred.lock\n\nSorry for the confusion\n\nPhillip\n"},{"id":"370599","messageId":"20190304143002.56a6c4ec@kitsune.suse.cz","threadId":"50535","inReplyTo":"4309841e-2b98-22d4-505e-1b9ea2f5e3bb@talktalk.net","subject":"Re: [PATCH 2/2] setup: don't fail if commondir reference is deleted.","fromName":"Michal Suchánek","fromEmail":"msuchanek@suse.de","sentAt":"2019-03-04T13:30:02Z","receivedAt":"2019-03-04T13:30:10Z","isPatch":true,"sender":{"key":"msuchanek@suse.de","avatar":"https://avatars.githubusercontent.com/u/787652?v=4"},"body":"Hello,\n\nOn Thu, 21 Feb 2019 17:27:04 +0000\nPhillip Wood wrote:\n\n> Hi Eric\n> \n> On 21/02/2019 17:12, Eric Sunshine wrote:\n> > On Thu, Feb 21, 2019 at 12:07 PM Phillip Wood wrote: \n> >> On 21/02/2019 13:50, Michal Suchánek wrote: \n> >>>> On Tue, Feb 19, 2019 at 12:05 AM Michal Suchanek wrote: \n> >>> The problem is we don't forbid worktree names ending with \".lock\".\n> >>> Which means that if we start to forbid them now existing worktrees\n> >>> might become inaccessible. \n> >>\n> >> I think it is also racy as the renaming breaks the use of mkdir erroring\n> >> out if the directory already exists. One solution is to have a lock\n> >> entry in $GIT_COMMON_DIR/worktree-locks and make sure the code that\n> >> iterates over the entries in $GIT_COMMON_DIR/worktrees skips any that\n> >> have a corresponding ignores in $GIT_COMMON_DIR/worktree-locks. If the\n> >> worktree-locks/ is created before worktree/ then it should be\n> >> race free (you will have to remove the lock if the real entry cannot be\n> >> created and then increment the counter and try again). Entries could\n> >> also be locked on removal to prevent a race there. \n> > \n> > I wonder, though, how much this helps or hinders the use-case which\n> > prompted this patch series in the first place; to wit, creating\n> > hundreds or thousands of worktrees. Doing so serially was too slow, so\n> > the many \"git worktree add\" invocations were instead run in parallel\n> > (which led to \"discovery\" of race conditions). Using a global worktree\n> > lock would serialize worktree creation, thus slowing it down once\n> > again. \n> \n> The idea is that there are per-worktree lock stored under worktree-locks \n> (hence the plural name). Using a separate directory for the locks gets \n> round the problems of name clashes between the lock for a worktree \n> called foo and one called foo.lock and means we can rely on mkdir \n> erroring out if the worktree name already exists as there is no renaming.\n\nI suppose this separate directory would work. When are you supposed to\ntake the lock, though?\n\nWhen adding worktree, sure.\n\nWhen managing worktrees, sure. Otherwise you would see the incomplete\nworktrees.\n\nWhen doing anything in git? Probably. Because otherwise you could\naccidentally use the incomplete worktree. Or somebody deleting worktree\nwould fail removing it because you would keep adding files to it.\n\nIsn't git supposed to allow parallel access to the repository?\n\nAs things stand if you wanted to implement worktree locking you would\nneed to lock the worktree for *every* operation that touches it, and\nfor many operations you would have to lock/unlock *all* worktrees one by\none to find the worktree you are supposed to work on.\n\nI don't feel like adding locking to all of git to fix this problem.\n\nSure, adding enough locking to ensure repository consistency at all\ntimes would be nice but it also needs to be granular enough to not harm\nperformance. I can't say I understand the git repository layout and\nusage well enough to design that.\n\nThanks\n\nMichal\n"},{"id":"370944","messageId":"CACsJy8D_ahM_7mLaAijJsZ0e8BF6PBfr3pPisOnYmRH7U8kmqA@mail.gmail.com","threadId":"50535","inReplyTo":"e134801d570d0a0c85424eb80b41893f4d8383ca.1550679076.git.msuchanek@suse.de","subject":"Re: [PATCH v3 1/2] worktree: fix worktree add race.","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-03-08T09:20:13Z","receivedAt":"2019-03-08T09:20:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Junio, it seems 2/2 is stuck in an endless discussion. But 1/2 is good\nregardless, maybe pick it up now and let 2/2 come later whenever it's\nready?\n\nOn Wed, Feb 20, 2019 at 11:16 PM Michal Suchanek wrote:\n>\n> Git runs a stat loop to find a worktree name that's available and then does\n> mkdir on the found name. Turn it to mkdir loop to avoid another invocation of\n> worktree add finding the same free name and creating the directory first.\n>\n> Signed-off-by: Michal Suchanek \n> ---\n> v2:\n> - simplify loop exit condition\n> - exit early if the mkdir fails for reason other than already present\n> worktree\n> - make counter unsigned\n> ---\n> builtin/worktree.c | 12 +++++++-----\n> 1 file changed, 7 insertions(+), 5 deletions(-)\n>\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index 3f9907fcc994..85a604cfe98c 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -268,10 +268,10 @@ static int add_worktree(const char *path, const char *refname,\n> struct strbuf sb_git = STRBUF_INIT, sb_repo = STRBUF_INIT;\n> struct strbuf sb = STRBUF_INIT;\n> const char *name;\n> - struct stat st;\n> struct child_process cp = CHILD_PROCESS_INIT;\n> struct argv_array child_env = ARGV_ARRAY_INIT;\n> - int counter = 0, len, ret;\n> + unsigned int counter = 0;\n> + int len, ret;\n> struct strbuf symref = STRBUF_INIT;\n> struct commit *commit = NULL;\n> int is_branch = 0;\n> @@ -295,8 +295,12 @@ static int add_worktree(const char *path, const char *refname,\n> if (safe_create_leading_directories_const(sb_repo.buf))\n> die_errno(_(\"could not create leading directories of '%s'\"),\n> sb_repo.buf);\n> - while (!stat(sb_repo.buf, &st)) {\n> +\n> + while (mkdir(sb_repo.buf, 0777)) {\n> counter++;\n> + if ((errno != EEXIST) || !counter /* overflow */)\n> + die_errno(_(\"could not create directory of '%s'\"),\n> + sb_repo.buf);\n> strbuf_setlen(&sb_repo, len);\n> strbuf_addf(&sb_repo, \"%d\", counter);\n> }\n> @@ -306,8 +310,6 @@ static int add_worktree(const char *path, const char *refname,\n> atexit(remove_junk);\n> sigchain_push_common(remove_junk_on_signal);\n>\n> - if (mkdir(sb_repo.buf, 0777))\n> - die_errno(_(\"could not create directory of '%s'\"), sb_repo.buf);\n> junk_git_dir = xstrdup(sb_repo.buf);\n> is_junk = 1;\n>\n> --\n> 2.20.1\n>\n\n\n-- \nDuy\n"},{"id":"370948","messageId":"CAPig+cRCUZApx36ZdmWAY2UbvgF6phjVNb5XU7YN=5LryU5trw@mail.gmail.com","threadId":"50535","inReplyTo":"CACsJy8D_ahM_7mLaAijJsZ0e8BF6PBfr3pPisOnYmRH7U8kmqA@mail.gmail.com","subject":"Re: [PATCH v3 1/2] worktree: fix worktree add race.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-03-08T09:37:59Z","receivedAt":"2019-03-08T09:38:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 8, 2019 at 4:20 AM Duy Nguyen wrote:\n> Junio, it seems 2/2 is stuck in an endless discussion. But 1/2 is good\n> regardless, maybe pick it up now and let 2/2 come later whenever it's\n> ready?\n\nYep, 1/2 seems a good idea and has not been controversial. It may not\nsolve all the race conditions, but it is a good step forward.\n"},{"id":"371096","messageId":"xmqq5zsqmaxp.fsf@gitster-ct.c.googlers.com","threadId":"50535","inReplyTo":"CACsJy8D_ahM_7mLaAijJsZ0e8BF6PBfr3pPisOnYmRH7U8kmqA@mail.gmail.com","subject":"Re: [PATCH v3 1/2] worktree: fix worktree add race.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-03-11T01:55:46Z","receivedAt":"2019-03-11T01:55:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> Junio, it seems 2/2 is stuck in an endless discussion. But 1/2 is good\n> regardless, maybe pick it up now and let 2/2 come later whenever it's\n> ready?\n\nThanks for poking, and I think it is a good idea.\n"}]}