[v2,1/1] git clone: don't clone into non-empty directory
diff mbox series

Message ID 20200702081335.16786-2-ben@wijen.net
State New
Headers show
Series
  • git clone: don't clone into non-empty directory
Related show

Commit Message

Ben Wijen July 2, 2020, 8:13 a.m. UTC
When using git clone with --separate-git-dir realgitdir and
realgitdir already exists, it's content is destroyed.

So, make sure we don't clone into an existing non-empty directory.

Signed-off-by: Ben Wijen <ben@wijen.net>
---
 builtin/clone.c  | 12 ++++++++++--
 t/t5601-clone.sh |  4 +++-
 2 files changed, 13 insertions(+), 3 deletions(-)

Comments

Junio C Hamano July 6, 2020, 10:40 p.m. UTC | #1
Ben Wijen <ben@wijen.net> writes:

> When using git clone with --separate-git-dir realgitdir and
> realgitdir already exists, it's content is destroyed.
>
> So, make sure we don't clone into an existing non-empty directory.

This came from d45420c1 (clone: do not clean up directories we
didn't create, 2018-01-02), where we claimed

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

The assumed use case is that somebody created an empty directory for
our use in a grandparent directory where we have no write permission
and gave it to us, and we want to do a "git clone" into it while
keeping that directory alive.  But we didn't make sure the directory
was not empty ourselves---we just assumed it to be empty.

And the originally envisioned use case does not have anything to do
with the use of --separate-git-dir at all.  So from that point of
view, this change does not break the originally envisioned use case,
so this probably is not regressing any existing and valid use case
that may not involve "--separate-git-dir", but I'd rather see that
the commit message explain these things to its readers to justify
itself.

Thanks.

Patch
diff mbox series

diff --git a/builtin/clone.c b/builtin/clone.c
index 2a8e3aaaed..e470193bb8 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -946,7 +946,7 @@  int cmd_clone(int argc, const char **argv, const char *prefix)
 	int is_bundle = 0, is_local;
 	const char *repo_name, *repo, *work_tree, *git_dir;
 	char *path, *dir, *display_repo = NULL;
-	int dest_exists;
+	int dest_exists, real_dest_exists = 0;
 	const struct ref *refs, *remote_head;
 	const struct ref *remote_head_points_at;
 	const struct ref *our_head_points_at;
@@ -1021,6 +1021,14 @@  int cmd_clone(int argc, const char **argv, const char *prefix)
 		die(_("destination path '%s' already exists and is not "
 			"an empty directory."), dir);
 
+	if (real_git_dir) {
+		real_dest_exists = path_exists(real_git_dir);
+		if (real_dest_exists && !is_empty_dir(real_git_dir))
+			die(_("repository path '%s' already exists and is not "
+				"an empty directory."), real_git_dir);
+	}
+
+
 	strbuf_addf(&reflog_msg, "clone: from %s",
 		    display_repo ? display_repo : repo);
 	free(display_repo);
@@ -1057,7 +1065,7 @@  int cmd_clone(int argc, const char **argv, const char *prefix)
 	}
 
 	if (real_git_dir) {
-		if (path_exists(real_git_dir))
+		if (real_dest_exists)
 			junk_git_dir_flags |= REMOVE_DIR_KEEP_TOPLEVEL;
 		junk_git_dir = real_git_dir;
 	} else {
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 84ea2a3eb7..eb9a093e25 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -271,7 +271,9 @@  test_expect_success 'fetch from gitfile parent' '
 
 test_expect_success 'clone separate gitdir where target already exists' '
 	rm -rf dst &&
-	test_must_fail git clone --separate-git-dir realgitdir src dst
+	echo foo=bar >>realgitdir/config &&
+	test_must_fail git clone --separate-git-dir realgitdir src dst &&
+	grep foo=bar realgitdir/config
 '
 
 test_expect_success 'clone --reference from original' '