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

Message ID 20200701093653.3706-3-ben@wijen.net
State New
Headers show
Series
  • git clone with --separate-git-dir destroys existing directory content
Related show

Commit Message

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

Make sure we don't clone into an existing non-empty directory.

Signed-off-by: Ben Wijen <ben@wijen.net>
---
 builtin/clone.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

Comments

Eric Sunshine July 1, 2020, 4:10 p.m. UTC | #1
On Wed, Jul 1, 2020 at 5:46 AM Ben Wijen <ben@wijen.net> wrote:
> When using git clone with --separate-git-dir realgitdir and
> realgitdir already exists, it's content is destroyed.
>
> Make sure we don't clone into an existing non-empty directory.
>
> Signed-off-by: Ben Wijen <ben@wijen.net>
> ---
> diff --git a/builtin/clone.c b/builtin/clone.c
> @@ -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))

"git init --separate-git-dir=" checks only whether the path exists,
and errors out if it does; it doesn't care whether the directory is
empty or not. I'm wondering, therefore, if this check should be
tightened to more closely align the behavior of the two commands.
(Using the tighter semantic now doesn't prohibit loosening it in the
future, whereas it's harder to tighten behavior which started out
loose.)

> +                       die(_("destination path '%s' already exists and is not "
> +                               "an empty directory."), real_git_dir);

This message might be a bit clearer if "destination" is changed to
"repository". (And drop the bit about not being empty -- if we go that
route.)
Ben Wijen July 2, 2020, 7:50 a.m. UTC | #2
On 01-07-2020 18:10, Eric Sunshine wrote:
> On Wed, Jul 1, 2020 at 5:46 AM Ben Wijen <ben@wijen.net> wrote:
> 
> "git init --separate-git-dir=" checks only whether the path exists,
> and errors out if it does; it doesn't care whether the directory is
> empty or not. I'm wondering, therefore, if this check should be
> tightened to more closely align the behavior of the two commands.
> (Using the tighter semantic now doesn't prohibit loosening it in the
> future, whereas it's harder to tighten behavior which started out
> loose.)
> 

About `git init`, I did take a look at it, but saw 'exist_ok' is always
true when called from 'cmd_init_db' (see builtin/init_db.c:654)
This means `git init` always allows when git_dir/real_git_dir already exists,
which I understand for the worktree, but I doubt if one wants this for the
repository. (when --separate-git-dir is used)

Also, because 'exist_ok' is always true for `git init` and - with this patch -
`git clone` checks the git_dir/real_git_dir before the 'init_db' call I'm wondering if
the code in 'init_db' (builtin/init-db.c:394-400) can be removed. 

Then, even if we do take that route (no 'is_empty_dir', only 'path_exists') we must
also address junk_git_dir_flags, as REMOVE_DIR_KEEP_TOPLEVEL would invalidate
a second git clone.

So, as this patch only fixes the problem at hand, IMHO separate patch-sets should
be created for mentioned issues.

Ben...

Patch
diff mbox series

diff --git a/builtin/clone.c b/builtin/clone.c
index bef70745c0..54a91acf06 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(_("destination 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 {