git clone doesn't cleanup on failure when getcwd fails

git clone doesn't cleanup on failure when getcwd fails

[Spencer Nelson]

 
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

[PATCH] clone: initialize atexit cleanup handler earlier

[Jeff King]

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

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

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

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

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(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index aa01437..53a2e5a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -842,20 +842,21 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		git_dir = mkpathdup("%s/.git", dir);
 	}
 
+	atexit(remove_junk);
+	sigchain_push_common(remove_junk_on_signal);
+
 	if (!option_bare) {
-		junk_work_tree = work_tree;
 		if (safe_create_leading_directories_const(work_tree) < 0)
 			die_errno(_("could not create leading directories of '%s'"),
 				  work_tree);
 		if (!dest_exists && mkdir(work_tree, 0777))
 			die_errno(_("could not create work tree dir '%s'"),
 				  work_tree);
+		junk_work_tree = work_tree;
 		set_git_work_tree(work_tree);
 	}
-	junk_git_dir = git_dir;
-	atexit(remove_junk);
-	sigchain_push_common(remove_junk_on_signal);
 
+	junk_git_dir = git_dir;
 	if (safe_create_leading_directories_const(git_dir) < 0)
 		die(_("could not create leading directories of '%s'"), git_dir);
 
-- 
2.3.3.520.g3cfbb5d

[PATCH] clone: drop period from end of die_errno message

[Jeff King]

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(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 9572467..aa01437 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -848,7 +848,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			die_errno(_("could not create leading directories of '%s'"),
 				  work_tree);
 		if (!dest_exists && mkdir(work_tree, 0777))
-			die_errno(_("could not create work tree dir '%s'."),
+			die_errno(_("could not create work tree dir '%s'"),
 				  work_tree);
 		set_git_work_tree(work_tree);
 	}
-- 
2.3.3.520.g3cfbb5d