{"thread":{"id":"38849","subject":"git clone doesn't cleanup on failure when getcwd fails","startedAt":"2015-03-18T18:03:30Z","lastAt":"2015-03-18T19:02:01Z","messageCount":3,"participants":["Spencer Nelson","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"257968","messageId":"etPan.5509bdf2.7a1ae87d.1766@ttvadmins-MacBook-Pro.local","threadId":"38849","inReplyTo":null,"subject":"git clone doesn't cleanup on failure when getcwd fails","fromName":"Spencer Nelson","fromEmail":"s@spenczar.com","sentAt":"2015-03-18T18:03:30Z","receivedAt":"2015-03-18T18:03:30Z","isPatch":false,"sender":{"key":"s@spenczar.com","avatar":null},"body":" \nIf you’re in a shell in a directory which no longer exists (because, say, another terminal removed it), then getcwd() will fail, at least on OS X Yosemite 10.10.2. In this case, git clone will fail. That’s totally reasonable. \n\nIf you invoke git clone with the git clone syntax, then will be created, but it will be empty.\n\nThis was unexpected - I would think the failure case shouldn’t leave this empty directory, but should instead clean up after itself.\n\nI’m not alone: golang’s go get command for installing third-party packages performs a `git clone ` only if does not already exist - if it does exist, then go believes that the package is already installed, and so does nothing. So, if you call go get from within a directory which no longer exists, git will create the empty directory, putting go get into a bad state.\n\n\nConcrete example:\n\nMake a dummy repo in /tmp/somerepo:\ntty1:\n$ cd /tmp\n$ git init somerepo\n\nIn another tty, make a /tmp/poof and enter it\ntty2:\n$ mkdir /tmp/poof\n$ cd /tmp/poof\n\nIn the first tty, delete /tmp/poof\ntty1:\n$ rmdir /tmp/poof\n\nFinally, in the second tty, clone:\ntty2:\n$ git clone /tmp/somerepo /tmp/newcopy\nfatal: Could not get current working directory: No such file or directory\n$ ls /tmp/newcopy && echo \"yes, it exists\"\nyes, it exists\n\n\nMy version details:\n\n$ git version\ngit version 2.3.2\n\n$ uname -a\nDarwin mbp.local 14.1.0 Darwin Kernel Version 14.1.0: Thu Feb 26 19:26:47 PST 2015; root:xnu-2782.10.73~1/RELEASE_X86_64 x86_64\n"},{"id":"257970","messageId":"20150318185531.GA650@peff.net","threadId":"38849","inReplyTo":"etPan.5509bdf2.7a1ae87d.1766@ttvadmins-MacBook-Pro.local","subject":"[PATCH] clone: initialize atexit cleanup handler earlier","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-18T18:55:32Z","receivedAt":"2015-03-18T18:55:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 18, 2015 at 11:03:30AM -0700, Spencer Nelson wrote:\n\n> If you’re in a shell in a directory which no longer exists (because,\n> say, another terminal removed it), then getcwd() will fail, at least\n> on OS X Yosemite 10.10.2. In this case, git clone will fail. That’s\n> totally reasonable.\n\nYeah, we fail in set_git_work_tree, which calls real_path() on the new\ndirectory, which fails because it cannot get the current working\ndirectory. In the example you gave, we already have an absolute path to\nthe new directory, and it is outside the \"disappeared\" directory. So I\nwould think we could get by without having a cwd. It looks like we do so\nbecause we have to chdir() away from the cwd as part of real_path, and\nthen need to be able to chdir back. So I'm not sure there's an easy way\nto fix it.\n\nAnyway, that's tangential to your actual problem...\n\n> If you invoke git clone with the git clone syntax, then\n> will be created, but it will be empty.\n\nI think the original code just didn't expect set_git_work_tree to fail,\nand doesn't install the cleanup code until after it is called. This\npatch fixes it.\n\n-- >8 --\nSubject: clone: initialize atexit cleanup handler earlier\n\nIf clone fails, we generally try to clean up any directories\nwe've created. We do this by installing an atexit handler,\nso that we don't have to manually trigger cleanup. However,\nsince we install this after touching the filesystem, any\nerrors between our initial mkdir() and our atexit() call\nwill result in us leaving a crufty directory around.\n\nWe can fix this by moving our atexit() call earlier. It's OK\nto do it before the junk_work_tree variable is set, because\nremove_junk makes sure the variable is initialized. This\nmeans we \"activate\" the handler by assigning to the\njunk_work_tree variable, which we now bump down to just\nafter we call mkdir(). We probably do not want to do it\nbefore, because a plausible reason for mkdir() to fail is\nEEXIST (i.e., we are racing with another \"git init\"), and we\nwould not want to remove their work.\n\nOTOH, this is probably not that big a deal; we will allow\ncloning into an empty directory (and skip the mkdir), which\nis already racy (i.e., one clone may see the other's empty\ndir and start writing into it). Still, it does not hurt to\nerr on the side of caution here.\n\nNote that writing into junk_work_tree and junk_git_dir after\ninstalling the handler is also technically racy, as we call\nour handler on an async signal. Depending on the platform,\nwe could see a sheared write to the variables. Traditionally\nwe have not worried about this, and indeed we already do\nthis later in the function. If we want to address that, it\ncan come as a separate topic.\n\nSigned-off-by: Jeff King \n---\nSheesh, for such a little change, there are a lot of racy things to\nthink about. Note that even if we did want to make two racing clone\nprocesses atomic in creating the working tree, the whole git_dir\ninitialization is still not (and explicitly ignores EEXIST). I think if\nsomebody wants atomicity here, they should do the mkdir themselves, and\nthen have git fill in the rest.\n\nNo test. I seem to recall that Windows is tricky with making the cwd go\naway (can you even do it there while a process is still in it?), and I\ndon't think such a minor thing is worth the portability headcaches in\nthe test suite.\n\n builtin/clone.c | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex aa01437..53a2e5a 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -842,20 +842,21 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tgit_dir = mkpathdup(\"%s/.git\", dir);\n \t}\n \n+\tatexit(remove_junk);\n+\tsigchain_push_common(remove_junk_on_signal);\n+\n \tif (!option_bare) {\n-\t\tjunk_work_tree = work_tree;\n \t\tif (safe_create_leading_directories_const(work_tree) < 0)\n \t\t\tdie_errno(_(\"could not create leading directories of '%s'\"),\n \t\t\t\t work_tree);\n \t\tif (!dest_exists && mkdir(work_tree, 0777))\n \t\t\tdie_errno(_(\"could not create work tree dir '%s'\"),\n \t\t\t\t work_tree);\n+\t\tjunk_work_tree = work_tree;\n \t\tset_git_work_tree(work_tree);\n \t}\n-\tjunk_git_dir = git_dir;\n-\tatexit(remove_junk);\n-\tsigchain_push_common(remove_junk_on_signal);\n \n+\tjunk_git_dir = git_dir;\n \tif (safe_create_leading_directories_const(git_dir) < 0)\n \t\tdie(_(\"could not create leading directories of '%s'\"), git_dir);\n \n-- \n2.3.3.520.g3cfbb5d\n"},{"id":"257972","messageId":"20150318190200.GC650@peff.net","threadId":"38849","inReplyTo":"20150318185531.GA650@peff.net","subject":"[PATCH] clone: drop period from end of die_errno message","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-18T19:02:01Z","receivedAt":"2015-03-18T19:02:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We do not usually end our errors with a full stop, but it\nlooks especially bad when you use die_errno, which adds a\ncolon, like:\n\n fatal: could not create work tree dir 'foo'.: No such file or directory\n\nSigned-off-by: Jeff King \n---\nNot strictly related to the other patch, but I noticed it while playing\naround.\n\n builtin/clone.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 9572467..aa01437 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -848,7 +848,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\t\tdie_errno(_(\"could not create leading directories of '%s'\"),\n \t\t\t\t work_tree);\n \t\tif (!dest_exists && mkdir(work_tree, 0777))\n-\t\t\tdie_errno(_(\"could not create work tree dir '%s'.\"),\n+\t\t\tdie_errno(_(\"could not create work tree dir '%s'\"),\n \t\t\t\t work_tree);\n \t\tset_git_work_tree(work_tree);\n \t}\n-- \n2.3.3.520.g3cfbb5d\n"}]}