clone: do not clean up directories we didn't create

Once upon a time, git-clone would refuse to write into a directory that it did not itself create. The cleanup routines for a failed clone could therefore just remove the git and worktree dirs completely. In 55892d2398 (Allow cloning to an existing empty directory, 2009-01-11), we learned to write into an existing directory. Which means that doing: mkdir foo git clone will-fail foo ends up deleting foo. This isn't a huge catastrophe, since by definition foo must be empty. But it's somewhat confusing; we should leave the filesystem as we found it. Because we know that the only directory we'll write into is an empty one, we can handle this case by just passing the KEEP_TOPLEVEL flag to our recursive delete (if we could write into populated directories, we'd have to keep track of what we wrote and what we did not, which would be much harder). Note that we need to handle the work-tree and git-dir separately, though, as only one might exist (and the new tests in t5600 cover all cases). Reported-by: Stephan Janssen <sjanssen@you-get.com> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Jan 2, 2018 at 16:11 UTC d45420c1c8612f085f1901c33ff6f0ccfbb72d3b
2 files changed +67 -9
builtin/clone.c
+16 -4
@@ -472,7 +472,9 @@ static void clone_local(const char *src_repo, const char *dest_repo)
472 }
473
474 static const char *junk_work_tree;
475 +static int junk_work_tree_flags;
476 static const char *junk_git_dir;
477 +static int junk_git_dir_flags;
478 static enum {
479 JUNK_LEAVE_NONE,
480 JUNK_LEAVE_REPO,
@@ -501,12 +503,12 @@ static void remove_junk(void)
503
504 if (junk_git_dir) {
505 strbuf_addstr(&sb, junk_git_dir);
504 - remove_dir_recursively(&sb, 0);
506 + remove_dir_recursively(&sb, junk_git_dir_flags);
507 strbuf_reset(&sb);
508 }
509 if (junk_work_tree) {
510 strbuf_addstr(&sb, junk_work_tree);
509 - remove_dir_recursively(&sb, 0);
511 + remove_dir_recursively(&sb, junk_work_tree_flags);
512 }
513 strbuf_release(&sb);
514 }
@@ -972,14 +974,24 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
974 if (safe_create_leading_directories_const(work_tree) < 0)
975 die_errno(_("could not create leading directories of '%s'"),
976 work_tree);
975 - if (!dest_exists && mkdir(work_tree, 0777))
977 + if (dest_exists)
978 + junk_work_tree_flags |= REMOVE_DIR_KEEP_TOPLEVEL;
979 + else if (mkdir(work_tree, 0777))
980 die_errno(_("could not create work tree dir '%s'"),
981 work_tree);
982 junk_work_tree = work_tree;
983 set_git_work_tree(work_tree);
984 }
985
982 - junk_git_dir = real_git_dir ? real_git_dir : git_dir;
986 + if (real_git_dir) {
987 + if (dir_exists(real_git_dir))
988 + junk_git_dir_flags |= REMOVE_DIR_KEEP_TOPLEVEL;
989 + junk_git_dir = real_git_dir;
990 + } else {
991 + if (dest_exists)
992 + junk_git_dir_flags |= REMOVE_DIR_KEEP_TOPLEVEL;
993 + junk_git_dir = git_dir;
994 + }
995 if (safe_create_leading_directories_const(git_dir) < 0)
996 die(_("could not create leading directories of '%s'"), git_dir);
997
t/t5600-clone-fail-cleanup.sh
+51 -5
@@ -7,10 +7,21 @@ test_description='test git clone to cleanup after failure
7
8 This test covers the fact that if git clone fails, it should remove
9 the directory it created, to avoid the user having to manually
10 -remove the directory before attempting a clone again.'
10 +remove the directory before attempting a clone again.
11 +
12 +Unless the directory already exists, in which case we clean up only what we
13 +wrote.
14 +'
15
16 . ./test-lib.sh
17
18 +corrupt_repo () {
19 + test_when_finished "rmdir foo/.git/objects.bak" &&
20 + mkdir foo/.git/objects.bak/ &&
21 + test_when_finished "mv foo/.git/objects.bak/* foo/.git/objects/" &&
22 + mv foo/.git/objects/* foo/.git/objects.bak/
23 +}
24 +
25 test_expect_success 'clone of non-existent source should fail' '
26 test_must_fail git clone foo bar
27 '
@@ -42,13 +53,48 @@ test_expect_success 'successful clone must leave the directory' '
53 '
54
55 test_expect_success 'failed clone --separate-git-dir should not leave any directories' '
45 - test_when_finished "rmdir foo/.git/objects.bak" &&
46 - mkdir foo/.git/objects.bak/ &&
47 - test_when_finished "mv foo/.git/objects.bak/* foo/.git/objects/" &&
48 - mv foo/.git/objects/* foo/.git/objects.bak/ &&
56 + corrupt_repo &&
57 test_must_fail git clone --separate-git-dir gitdir foo worktree &&
58 test_path_is_missing gitdir &&
59 test_path_is_missing worktree
60 '
61
62 +test_expect_success 'failed clone into empty leaves directory (vanilla)' '
63 + mkdir -p empty &&
64 + corrupt_repo &&
65 + test_must_fail git clone foo empty &&
66 + test_dir_is_empty empty
67 +'
68 +
69 +test_expect_success 'failed clone into empty leaves directory (bare)' '
70 + mkdir -p empty &&
71 + corrupt_repo &&
72 + test_must_fail git clone --bare foo empty &&
73 + test_dir_is_empty empty
74 +'
75 +
76 +test_expect_success 'failed clone into empty leaves directory (separate)' '
77 + mkdir -p empty-git empty-wt &&
78 + corrupt_repo &&
79 + test_must_fail git clone --separate-git-dir empty-git foo empty-wt &&
80 + test_dir_is_empty empty-git &&
81 + test_dir_is_empty empty-wt
82 +'
83 +
84 +test_expect_success 'failed clone into empty leaves directory (separate, git)' '
85 + mkdir -p empty-git &&
86 + corrupt_repo &&
87 + test_must_fail git clone --separate-git-dir empty-git foo no-wt &&
88 + test_dir_is_empty empty-git &&
89 + test_path_is_missing no-wt
90 +'
91 +
92 +test_expect_success 'failed clone into empty leaves directory (separate, wt)' '
93 + mkdir -p empty-wt &&
94 + corrupt_repo &&
95 + test_must_fail git clone --separate-git-dir no-git foo empty-wt &&
96 + test_path_is_missing no-git &&
97 + test_dir_is_empty empty-wt
98 +'
99 +
100 test_done