Merge branch 'jk/abort-clone-with-existing-dest' into maint

"git clone $there $here" is allowed even when here directory exists
as long as it is an empty directory, but the command incorrectly
removed it upon a failure of the operation.

* jk/abort-clone-with-existing-dest:
  clone: do not clean up directories we didn't create
  clone: factor out dir_exists() helper
  t5600: modernize style
  t5600: fix outdated comment about unborn HEAD

2 files changed, 98 insertions, 33 deletions

diff --git a/builtin/clone.c b/builtin/clone.c
index 2da71db107..284651797e 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -473,7 +473,9 @@ static void clone_local(const char *src_repo, const char *dest_repo)
 }
 
 static const char *junk_work_tree;
+static int junk_work_tree_flags;
 static const char *junk_git_dir;
+static int junk_git_dir_flags;
 static enum {
 	JUNK_LEAVE_NONE,
 	JUNK_LEAVE_REPO,
@@ -502,12 +504,12 @@ static void remove_junk(void)
 
 	if (junk_git_dir) {
 		strbuf_addstr(&sb, junk_git_dir);
-		remove_dir_recursively(&sb, 0);
+		remove_dir_recursively(&sb, junk_git_dir_flags);
 		strbuf_reset(&sb);
 	}
 	if (junk_work_tree) {
 		strbuf_addstr(&sb, junk_work_tree);
-		remove_dir_recursively(&sb, 0);
+		remove_dir_recursively(&sb, junk_work_tree_flags);
 	}
 	strbuf_release(&sb);
 }
@@ -863,10 +865,15 @@ static void dissociate_from_references(void)
 	free(alternates);
 }
 
+static int dir_exists(const char *path)
+{
+	struct stat sb;
+	return !stat(path, &sb);
+}
+
 int cmd_clone(int argc, const char **argv, const char *prefix)
 {
 	int is_bundle = 0, is_local;
-	struct stat buf;
 	const char *repo_name, *repo, *work_tree, *git_dir;
 	char *path, *dir;
 	int dest_exists;
@@ -938,7 +945,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		dir = guess_dir_name(repo_name, is_bundle, option_bare);
 	strip_trailing_slashes(dir);
 
-	dest_exists = !stat(dir, &buf);
+	dest_exists = dir_exists(dir);
 	if (dest_exists && !is_empty_dir(dir))
 		die(_("destination path '%s' already exists and is not "
 			"an empty directory."), dir);
@@ -949,7 +956,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		work_tree = NULL;
 	else {
 		work_tree = getenv("GIT_WORK_TREE");
-		if (work_tree && !stat(work_tree, &buf))
+		if (work_tree && dir_exists(work_tree))
 			die(_("working tree '%s' already exists."), work_tree);
 	}
 
@@ -967,14 +974,24 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		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))
+		if (dest_exists)
+			junk_work_tree_flags |= REMOVE_DIR_KEEP_TOPLEVEL;
+		else if (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 = real_git_dir ? real_git_dir : git_dir;
+	if (real_git_dir) {
+		if (dir_exists(real_git_dir))
+			junk_git_dir_flags |= REMOVE_DIR_KEEP_TOPLEVEL;
+		junk_git_dir = real_git_dir;
+	} else {
+		if (dest_exists)
+			junk_git_dir_flags |= REMOVE_DIR_KEEP_TOPLEVEL;
+		junk_git_dir = git_dir;
+	}
 	if (safe_create_leading_directories_const(git_dir) < 0)
 		die(_("could not create leading directories of '%s'"), git_dir);
 
diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh
index 4435693bb2..4a1a912e03 100755
--- a/t/t5600-clone-fail-cleanup.sh
+++ b/t/t5600-clone-fail-cleanup.sh
@@ -7,46 +7,94 @@ test_description='test git clone to cleanup after failure
 
 This test covers the fact that if git clone fails, it should remove
 the directory it created, to avoid the user having to manually
-remove the directory before attempting a clone again.'
+remove the directory before attempting a clone again.
+
+Unless the directory already exists, in which case we clean up only what we
+wrote.
+'
 
 . ./test-lib.sh
 
-test_expect_success \
-    'clone of non-existent source should fail' \
-    'test_must_fail git clone foo bar'
+corrupt_repo () {
+	test_when_finished "rmdir foo/.git/objects.bak" &&
+	mkdir foo/.git/objects.bak/ &&
+	test_when_finished "mv foo/.git/objects.bak/* foo/.git/objects/" &&
+	mv foo/.git/objects/* foo/.git/objects.bak/
+}
 
-test_expect_success \
-    'failed clone should not leave a directory' \
-    '! test -d bar'
+test_expect_success 'clone of non-existent source should fail' '
+	test_must_fail git clone foo bar
+'
 
-# Need a repo to clone
-test_create_repo foo
+test_expect_success 'failed clone should not leave a directory' '
+	test_path_is_missing bar
+'
 
-# clone doesn't like it if there is no HEAD. Is that a bug?
-(cd foo && touch file && git add file && git commit -m 'add file' >/dev/null 2>&1)
+test_expect_success 'create a repo to clone' '
+	test_create_repo foo
+'
+
+test_expect_success 'create objects in repo for later corruption' '
+	test_commit -C foo file
+'
 
 # source repository given to git clone should be relative to the
 # current path not to the target dir
-test_expect_success \
-    'clone of non-existent (relative to $PWD) source should fail' \
-    'test_must_fail git clone ../foo baz'
+test_expect_success 'clone of non-existent (relative to $PWD) source should fail' '
+	test_must_fail git clone ../foo baz
+'
 
-test_expect_success \
-    'clone should work now that source exists' \
-    'git clone foo bar'
+test_expect_success 'clone should work now that source exists' '
+	git clone foo bar
+'
 
-test_expect_success \
-    'successful clone must leave the directory' \
-    'test -d bar'
+test_expect_success 'successful clone must leave the directory' '
+	test_path_is_dir bar
+'
 
 test_expect_success 'failed clone --separate-git-dir should not leave any directories' '
-	mkdir foo/.git/objects.bak/ &&
-	mv foo/.git/objects/* foo/.git/objects.bak/ &&
+	corrupt_repo &&
 	test_must_fail git clone --separate-git-dir gitdir foo worktree &&
-	test_must_fail test -e gitdir &&
-	test_must_fail test -e worktree &&
-	mv foo/.git/objects.bak/* foo/.git/objects/ &&
-	rmdir foo/.git/objects.bak
+	test_path_is_missing gitdir &&
+	test_path_is_missing worktree
+'
+
+test_expect_success 'failed clone into empty leaves directory (vanilla)' '
+	mkdir -p empty &&
+	corrupt_repo &&
+	test_must_fail git clone foo empty &&
+	test_dir_is_empty empty
+'
+
+test_expect_success 'failed clone into empty leaves directory (bare)' '
+	mkdir -p empty &&
+	corrupt_repo &&
+	test_must_fail git clone --bare foo empty &&
+	test_dir_is_empty empty
+'
+
+test_expect_success 'failed clone into empty leaves directory (separate)' '
+	mkdir -p empty-git empty-wt &&
+	corrupt_repo &&
+	test_must_fail git clone --separate-git-dir empty-git foo empty-wt &&
+	test_dir_is_empty empty-git &&
+	test_dir_is_empty empty-wt
+'
+
+test_expect_success 'failed clone into empty leaves directory (separate, git)' '
+	mkdir -p empty-git &&
+	corrupt_repo &&
+	test_must_fail git clone --separate-git-dir empty-git foo no-wt &&
+	test_dir_is_empty empty-git &&
+	test_path_is_missing no-wt
+'
+
+test_expect_success 'failed clone into empty leaves directory (separate, wt)' '
+	mkdir -p empty-wt &&
+	corrupt_repo &&
+	test_must_fail git clone --separate-git-dir no-git foo empty-wt &&
+	test_path_is_missing no-git &&
+	test_dir_is_empty empty-wt
 '
 
 test_done