* [PATCH] clone: initialize atexit cleanup handler earlier
2015-03-18 18:03 git clone doesn't cleanup on failure when getcwd fails Spencer Nelson
@ 2015-03-18 18:55 ` Jeff King
2015-03-18 19:02 ` [PATCH] clone: drop period from end of die_errno message Jeff King
0 siblings, 1 reply; 3+ messages in thread
From: Jeff King @ 2015-03-18 18:55 UTC (permalink / raw)
To: Spencer Nelson; +Cc: git
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
^ permalink raw reply related [flat|nested] 3+ messages in thread