git clone doesn't cleanup on failure when getcwd fails

 
If 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.  

If you invoke git clone with the git clone <repo> <dir> syntax, then <dir> will be created, but it will be empty.

This was unexpected - I would think the failure case shouldn’t leave this empty directory, but should instead clean up after itself.

I’m not alone: golang’s go get command for installing third-party packages performs a `git clone <repo> <dir>` only if <dir> 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.

Concrete example:

Make a dummy repo in /tmp/somerepo: tty1: $ cd /tmp $ git init somerepo

In another tty, make a /tmp/poof and enter it tty2: $ mkdir /tmp/poof $ cd /tmp/poof

In the first tty, delete /tmp/poof tty1: $ rmdir /tmp/poof

Finally, in the second tty, clone: tty2: $ git clone /tmp/somerepo /tmp/newcopy fatal: Could not get current working directory: No such file or directory $ ls /tmp/newcopy && echo "yes, it exists" yes, it exists

My version details:

$ git version git version 2.3.2

$ uname -a Darwin 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

On Wed, Mar 18, 2015 at 11:03:30AM -0700, Spencer Nelson wrote:

Yeah, we fail in set_git_work_tree, which calls real_path() on the new directory, which fails because it cannot get the current working directory. In the example you gave, we already have an absolute path to the new directory, and it is outside the "disappeared" directory. So I would think we could get by without having a cwd. It looks like we do so because we have to chdir() away from the cwd as part of real_path, and then need to be able to chdir back. So I'm not sure there's an easy way to fix it.

Anyway, that's tangential to your actual problem...

I think the original code just didn't expect set_git_work_tree to fail, and doesn't install the cleanup code until after it is called. This patch fixes it.

-- >8 --
Subject: clone: initialize atexit cleanup handler earlier

If clone fails, we generally try to clean up any directories we've created. We do this by installing an atexit handler, so that we don't have to manually trigger cleanup. However, since we install this after touching the filesystem, any errors between our initial mkdir() and our atexit() call will result in us leaving a crufty directory around.

We can fix this by moving our atexit() call earlier. It's OK to do it before the junk_work_tree variable is set, because remove_junk makes sure the variable is initialized. This means we "activate" the handler by assigning to the junk_work_tree variable, which we now bump down to just after we call mkdir(). We probably do not want to do it before, because a plausible reason for mkdir() to fail is EEXIST (i.e., we are racing with another "git init"), and we would not want to remove their work.

OTOH, this is probably not that big a deal; we will allow cloning into an empty directory (and skip the mkdir), which is already racy (i.e., one clone may see the other's empty dir and start writing into it). Still, it does not hurt to err on the side of caution here.

Note that writing into junk_work_tree and junk_git_dir after installing the handler is also technically racy, as we call our handler on an async signal. Depending on the platform, we could see a sheared write to the variables. Traditionally we have not worried about this, and indeed we already do this later in the function. If we want to address that, it can come as a separate topic.

Signed-off-by: Jeff King <peff@peff.net>
---
Sheesh, for such a little change, there are a lot of racy things to
think about. Note that even if we did want to make two racing clone
processes atomic in creating the working tree, the whole git_dir
initialization is still not (and explicitly ignores EEXIST). I think if
somebody wants atomicity here, they should do the mkdir themselves, and
then have git fill in the rest.

No test. I seem to recall that Windows is tricky with making the cwd go away (can you even do it there while a process is still in it?), and I don't think such a minor thing is worth the portability headcaches in the test suite.

 builtin/clone.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

We do not usually end our errors with a full stop, but it looks especially bad when you use die_errno, which adds a colon, like:

  fatal: could not create work tree dir 'foo'.: No such file or directory

Signed-off-by: Jeff King <peff@peff.net>
---
Not strictly related to the other patch, but I noticed it while playing
around.

 builtin/clone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)