Re: [PATCH 4/4] clone: do not clean up directories we didn't create

[Junio C Hamano]

Jeff King  writes:

> Oops. The second one should be "wt" (since the idea was to flip the
> logic from the previous). Like so:
>
> diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh
> index 5cd94d5558..4a1a912e03 100755
> --- a/t/t5600-clone-fail-cleanup.sh
> +++ b/t/t5600-clone-fail-cleanup.sh
> @@ -89,7 +89,7 @@ test_expect_success 'failed clone into empty leaves 
> directory (separate, git)' '
>   test_path_is_missing no-wt
>  '
>  
> -test_expect_success 'failed clone into empty leaves directory (separate, 
> git)' '
> +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 &&

Squashed; thanks.

Re: [PATCH 4/4] clone: do not clean up directories we didn't create

[Junio C Hamano]

Jeff King  writes:

> diff --git a/builtin/clone.c b/builtin/clone.c
> index 04b0d7283f..284651797e 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -502,12 +504,12 @@ static void remove_junk(void)
>  
>   if (junk_git_dir) {
>   strbuf_addstr(, junk_git_dir);
> - remove_dir_recursively(, 0);
> + remove_dir_recursively(, junk_git_dir_flags);
>   strbuf_reset();
>   }
>   if (junk_work_tree) {
>   strbuf_addstr(, junk_work_tree);
> - remove_dir_recursively(, 0);
> + remove_dir_recursively(, junk_work_tree_flags);
>   }
>   strbuf_release();
>  }
> @@ -972,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);

The changes all look reasonable.

Thanks.

Re: [PATCH 4/4] clone: do not clean up directories we didn't create

[Jeff King]

On Tue, Jan 02, 2018 at 05:49:45PM -0500, Eric Sunshine wrote:

> > diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh
> > @@ -42,13 +53,48 @@ test_expect_success 'successful clone must leave the 
> > directory' '
> > +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, 
> > git)' '
> > +   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
> > +'
> 
> The final two tests seem to have the same title...

Oops. The second one should be "wt" (since the idea was to flip the
logic from the previous). Like so:

diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh
index 5cd94d5558..4a1a912e03 100755
--- a/t/t5600-clone-fail-cleanup.sh
+++ b/t/t5600-clone-fail-cleanup.sh
@@ -89,7 +89,7 @@ test_expect_success 'failed clone into empty leaves directory 
(separate, git)' '
test_path_is_missing no-wt
 '
 
-test_expect_success 'failed clone into empty leaves directory (separate, git)' 
'
+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 &&

Re: [PATCH 4/4] clone: do not clean up directories we didn't create

[Eric Sunshine]

On Tue, Jan 2, 2018 at 4:11 PM, Jeff King  wrote:
> [...]
> 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 
> Signed-off-by: Jeff King 
> ---
> diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh
> @@ -42,13 +53,48 @@ test_expect_success 'successful clone must leave the 
> directory' '
> +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, 
> git)' '
> +   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
> +'

The final two tests seem to have the same title...

[PATCH 4/4] clone: do not clean up directories we didn't create

[Jeff King]

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 
Signed-off-by: Jeff King 
---
 builtin/clone.c   | 20 
 t/t5600-clone-fail-cleanup.sh | 56 +++
 2 files changed, 67 insertions(+), 9 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 04b0d7283f..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(, junk_git_dir);
-   remove_dir_recursively(, 0);
+   remove_dir_recursively(, junk_git_dir_flags);
strbuf_reset();
}
if (junk_work_tree) {
strbuf_addstr(, junk_work_tree);
-   remove_dir_recursively(, 0);
+   remove_dir_recursively(, junk_work_tree_flags);
}
strbuf_release();
 }
@@ -972,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 7b2a8052f8..5cd94d5558 100755
--- a/t/t5600-clone-fail-cleanup.sh
+++ b/t/t5600-clone-fail-cleanup.sh
@@ -7,10 +7,21 @@ 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
 
+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 'clone of non-existent source should fail' '
test_must_fail git clone foo bar
 '
@@ -42,13 +53,48 @@ test_expect_success 'successful clone must leave the 
directory' '
 '
 
 test_expect_success 'failed clone --separate-git-dir should not leave any 
directories' '
-   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/ &&
+   corrupt_repo &&
test_must_fail git clone --separate-git-dir gitdir foo