Git removes existing folder when cancelling clone

Git removes existing folder when cancelling clone

[Stephan Janssen]

Hi,

I hope this mailing list is the right way to communicate this.

I just noticed the following behaviour on macOS 10.13.2 running Git v2.15.0:

1. `mkdir new-folder`
2. `ls` - shows new-folder
3. `git clone [repo] new-folder`
4. Git asks for password
5. I cancel by pressing ctrl+c

Result:
`ls` no longer shows new-folder.

Expected result:
`ls` shows new-folder

I’m not sure whether this might be a case of ‘works as intended’, but it’s not what I’d expect.

Kind regards,
Stephan Janssen

Re: Git removes existing folder when cancelling clone

[Robert P. J. Day]

[-- Attachment #1: Type: text/plain, Size: 1347 bytes --]

On Tue, 2 Jan 2018, Stephan Janssen wrote:

> Hi,
>
> I hope this mailing list is the right way to communicate this.
>
> I just noticed the following behaviour on macOS 10.13.2 running Git v2.15.0:
>
> 1. `mkdir new-folder`
> 2. `ls` - shows new-folder
> 3. `git clone [repo] new-folder`
> 4. Git asks for password
> 5. I cancel by pressing ctrl+c
>
> Result:
> `ls` no longer shows new-folder.
>
> Expected result:
> `ls` shows new-folder
>
> I’m not sure whether this might be a case of ‘works as intended’,
> but it’s not what I’d expect.

  i'm *pretty* sure that the optional directory name you supply is
meant to represent a directory you want git to *create* for you, not
one that already exists. that means the behaviour you see makes sense
-- if git assumes it was supposed to create the directory, and you
cancel the clone, it reasonably assumes it should get rid of it.

  i am willing to be corrected.

rday

-- 

========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                        http://crashcourse.ca

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================

Re: Git removes existing folder when cancelling clone

[Jeff King]

On Tue, Jan 02, 2018 at 06:12:35AM -0500, Robert P. J. Day wrote:

> > I just noticed the following behaviour on macOS 10.13.2 running Git v2.15.0:
> >
> > 1. `mkdir new-folder`
> > 2. `ls` - shows new-folder
> > 3. `git clone [repo] new-folder`
> > 4. Git asks for password
> > 5. I cancel by pressing ctrl+c
> >
> > Result:
> > `ls` no longer shows new-folder.
> >
> > Expected result:
> > `ls` shows new-folder
> >
> > I’m not sure whether this might be a case of ‘works as intended’,
> > but it’s not what I’d expect.
> 
>   i'm *pretty* sure that the optional directory name you supply is
> meant to represent a directory you want git to *create* for you, not
> one that already exists. that means the behaviour you see makes sense
> -- if git assumes it was supposed to create the directory, and you
> cancel the clone, it reasonably assumes it should get rid of it.

Correct. In the early days we required that the "new-folder" directory
not exist, and the initial "git clone" would have bailed in that case.
Any directory we removed would have been one we created.

That changed in 55892d2398 (Allow cloning to an existing empty
directory, 2009-01-11), and we continue to allow only empty directories.

So I don't think there's an urgent data-loss bug here; we will only ever
destroy an empty directory. However, the original intent was to leave
the filesystem as we found it on a failed or aborted clone, and we
obviously don't do that in this case. So it might be nice if we could
restore it to an empty directory.

Restoring the original contents in the general case is hard, since other
parts of the clone may have created arbitrary files. But if we know the
original is just empty, we can simply delete everything in it, but not
the outer directory.

-Peff

[PATCH 0/4] Git removes existing folder when cancelling clone

[Jeff King]

On Tue, Jan 02, 2018 at 03:04:43PM -0500, Jeff King wrote:

> So I don't think there's an urgent data-loss bug here; we will only ever
> destroy an empty directory. However, the original intent was to leave
> the filesystem as we found it on a failed or aborted clone, and we
> obviously don't do that in this case. So it might be nice if we could
> restore it to an empty directory.

Here's a patch series to do that. The first three are just preparatory
cleanups; the last one is the interesting bit.

  [1/4]: t5600: fix outdated comment about unborn HEAD
  [2/4]: t5600: modernize style
  [3/4]: clone: factor out dir_exists() helper
  [4/4]: clone: do not clean up directories we didn't create

 builtin/clone.c               |  31 ++++++++++---
 t/t5600-clone-fail-cleanup.sh | 100 +++++++++++++++++++++++++++++++-----------
 2 files changed, 98 insertions(+), 33 deletions(-)

-Peff

[PATCH 1/4] t5600: fix outdated comment about unborn HEAD

[Jeff King]

Back when this test was written, git-clone could not handle
a repository without any commits. These days it works fine,
and this comment is out of date.

At first glance it seems like we could just drop this code
entirely now, but it's necessary for the final test, which
was added later. That test corrupts the repository by
temporarily removing its objects, which means we need to
have some objects to move.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5600-clone-fail-cleanup.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh
index 4435693bb2..f23f92e5a7 100755
--- a/t/t5600-clone-fail-cleanup.sh
+++ b/t/t5600-clone-fail-cleanup.sh
@@ -22,7 +22,7 @@ test_expect_success \
 # Need a repo to clone
 test_create_repo foo
 
-# clone doesn't like it if there is no HEAD. Is that a bug?
+# create some objects so that we can corrupt the repo later
 (cd foo && touch file && git add file && git commit -m 'add file' >/dev/null 2>&1)
 
 # source repository given to git clone should be relative to the
-- 
2.16.0.rc0.384.gc477e89267

[PATCH 2/4] t5600: modernize style

[Jeff King]

This is an old script which could use some updating before
we add to it:

  - use the standard line-breaking:

      test_expect_success 'title' '
              body
      '

  - run all code inside test_expect blocks to catch
    unexpected failures in setup steps

  - use "test_commit -C" instead of manually entering
    sub-repo

  - use test_when_finished for cleanup steps

  - test_path_is_* as appropriate

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5600-clone-fail-cleanup.sh | 48 ++++++++++++++++++++++---------------------
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh
index f23f92e5a7..7b2a8052f8 100755
--- a/t/t5600-clone-fail-cleanup.sh
+++ b/t/t5600-clone-fail-cleanup.sh
@@ -11,42 +11,44 @@ remove the directory before attempting a clone again.'
 
 . ./test-lib.sh
 
-test_expect_success \
-    'clone of non-existent source should fail' \
-    'test_must_fail git clone foo bar'
+test_expect_success 'clone of non-existent source should fail' '
+	test_must_fail git clone foo bar
+'
 
-test_expect_success \
-    'failed clone should not leave a directory' \
-    '! test -d bar'
+test_expect_success 'failed clone should not leave a directory' '
+	test_path_is_missing bar
+'
 
-# Need a repo to clone
-test_create_repo foo
+test_expect_success 'create a repo to clone' '
+	test_create_repo foo
+'
 
-# create some objects so that we can corrupt the repo later
-(cd foo && touch file && git add file && git commit -m 'add file' >/dev/null 2>&1)
+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' '
+	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_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_done
-- 
2.16.0.rc0.384.gc477e89267

[PATCH 3/4] clone: factor out dir_exists() helper

[Jeff King]

Two parts of git-clone's setup logic check whether a
directory exists, and they both call stat directly with the
same scratch "struct stat" buffer. Let's pull that into a
helper, which has a few advantages:

  - it makes the purpose of the stat calls more obvious

  - it makes it clear that we don't care about the
    information in "buf" remaining valid

  - if we later decide to make the check more robust (e.g.,
    complaining about non-directories), we can do it in one
    place

Note that we could just use file_exists() for this, which
has identical code. But we specifically care about
directories, so this future-proofs us against that function
later getting more picky about seeing actual files.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/clone.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 2da71db107..04b0d7283f 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -863,10 +863,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 +943,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 +954,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);
 	}
 
-- 
2.16.0.rc0.384.gc477e89267

[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 <sjanssen@you-get.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 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(&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);
 }
@@ -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 worktree &&
 	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, 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
+'
+
 test_done
-- 
2.16.0.rc0.384.gc477e89267

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 <peff@peff.net> 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 <sjanssen@you-get.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> 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...

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 3/4] clone: factor out dir_exists() helper

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Two parts of git-clone's setup logic check whether a
> directory exists, and they both call stat directly with the
> same scratch "struct stat" buffer. Let's pull that into a
> helper, which has a few advantages:
>
>   - it makes the purpose of the stat calls more obvious
>
>   - it makes it clear that we don't care about the
>     information in "buf" remaining valid
>
>   - if we later decide to make the check more robust (e.g.,
>     complaining about non-directories), we can do it in one
>     place
>
> Note that we could just use file_exists() for this, which
> has identical code. But we specifically care about
> directories, so this future-proofs us against that function
> later getting more picky about seeing actual files.

It leaves funny taste in my mouth to see that dir_exists() does call
stat() but does not check st.st_mode to see if it is a directory,
but for this particular caller, we want dest_exists() to be true
even when the thing is a non-directory, so that !is_empty_dir(dir)
call is made on the next line to trigger "exists but not an empty
dir" error.  After all, what this caller really wants to ask is "is
something sitting there?" and the answer it expects under normal
condition is "no, there is nothing there".

If we really want to be anal, perhaps a new helper path_exists()
that cares only about existence of paths (i.e. the implementation of
these two helpers they currently have), together with update to
check the st.st_mode for file_exists() and dir_exists(), may help
making the API set more rational, but I do not think it is worth it.

Thanks.

>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/clone.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 2da71db107..04b0d7283f 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -863,10 +863,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 +943,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 +954,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);
>  	}

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

[Junio C Hamano]

Jeff King <peff@peff.net> 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(&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);
>  }
> @@ -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 3/4] clone: factor out dir_exists() helper

[Jeff King]

On Thu, Jan 04, 2018 at 03:47:18PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Two parts of git-clone's setup logic check whether a
> > directory exists, and they both call stat directly with the
> > same scratch "struct stat" buffer. Let's pull that into a
> > helper, which has a few advantages:
> >
> >   - it makes the purpose of the stat calls more obvious
> >
> >   - it makes it clear that we don't care about the
> >     information in "buf" remaining valid
> >
> >   - if we later decide to make the check more robust (e.g.,
> >     complaining about non-directories), we can do it in one
> >     place
> >
> > Note that we could just use file_exists() for this, which
> > has identical code. But we specifically care about
> > directories, so this future-proofs us against that function
> > later getting more picky about seeing actual files.
> 
> It leaves funny taste in my mouth to see that dir_exists() does call
> stat() but does not check st.st_mode to see if it is a directory,
> but for this particular caller, we want dest_exists() to be true
> even when the thing is a non-directory, so that !is_empty_dir(dir)
> call is made on the next line to trigger "exists but not an empty
> dir" error.  After all, what this caller really wants to ask is "is
> something sitting there?" and the answer it expects under normal
> condition is "no, there is nothing there".

Yeah, that was part of the reason I left this as file-local instead of
adding it globally.

> If we really want to be anal, perhaps a new helper path_exists()
> that cares only about existence of paths (i.e. the implementation of
> these two helpers they currently have), together with update to
> check the st.st_mode for file_exists() and dir_exists(), may help
> making the API set more rational, but I do not think it is worth it.

Yep, I also considered that file_exists() probably wants to be
path_exists() with its current implementation. We'd probably want to
review all of the callers.

Anyway, I tried to do the minimal refactoring here, with no change in
behavior. I'm not opposed to calling this dir_exists() as path_exists()
and making it globally available (as you note, I don't think we'd want
to use a true dir_exists() here).

-Peff

Re: [PATCH 3/4] clone: factor out dir_exists() helper

[Jeff King]

On Thu, Jan 04, 2018 at 06:54:12PM -0500, Jeff King wrote:

> > If we really want to be anal, perhaps a new helper path_exists()
> > that cares only about existence of paths (i.e. the implementation of
> > these two helpers they currently have), together with update to
> > check the st.st_mode for file_exists() and dir_exists(), may help
> > making the API set more rational, but I do not think it is worth it.
> 
> Yep, I also considered that file_exists() probably wants to be
> path_exists() with its current implementation. We'd probably want to
> review all of the callers.
> 
> Anyway, I tried to do the minimal refactoring here, with no change in
> behavior. I'm not opposed to calling this dir_exists() as path_exists()
> and making it globally available (as you note, I don't think we'd want
> to use a true dir_exists() here).

So I actually started down this road just now, but I'm not sure if it's
worth it.  If we were to transition to an endgame with path_exists(),
dir_exists(), and file_exists(), we'd probably want to do something
like:

  1. introduce path_exists(), but leave existing file_exists() callers
     in place

  2. introduce file_exists_as_file(), which checks S_IFREG

  3. audit each file_exists() call to see if it ought to be
     path_exists() or file_exists_as_file() and convert as needed

  4. When there are no more file_exists() calls left, all
     file_exists_as_file() instances can be renamed to file_exists().

But as with any "audit each..." plan, that leaves topics in flight out
of luck. If we want to be kind to those, we'd have to wait a long while
to shake out any file_exists() callers.

At which point is there much value in having path_exists() as a wrapper?
It's a better name, perhaps, but I think my future-proofing against
"file_exists() may become file-specific" was probably overly paranoid. I
don't think we could sanely do that conversion without risking breakage.

Maybe we should just document its behavior and use it here, rather than
introducing this new dir_exists().

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> 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 3/4] clone: factor out dir_exists() helper

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Jan 04, 2018 at 06:54:12PM -0500, Jeff King wrote:
>
>> > If we really want to be anal, perhaps a new helper path_exists()
>> > that cares only about existence of paths (i.e. the implementation of
>> > these two helpers they currently have), together with update to
>> > check the st.st_mode for file_exists() and dir_exists(), may help
>> > making the API set more rational, but I do not think it is worth it.
>> 
>> Yep, I also considered that file_exists() probably wants to be
>> path_exists() with its current implementation. We'd probably want to
>> review all of the callers.
>> 
>> Anyway, I tried to do the minimal refactoring here, with no change in
>> behavior. I'm not opposed to calling this dir_exists() as path_exists()
>> and making it globally available (as you note, I don't think we'd want
>> to use a true dir_exists() here).
>
> So I actually started down this road just now, but I'm not sure if it's
> worth it.

Yeah, although I said it already without starting down this road,
you actually thought about it more and your insight is more valuable
;-)

> If we were to transition to an endgame with path_exists(),
> dir_exists(), and file_exists(), we'd probably want to do something
> like:
>
>   1. introduce path_exists(), but leave existing file_exists() callers
>      in place
>
>   2. introduce file_exists_as_file(), which checks S_IFREG
>
>   3. audit each file_exists() call to see if it ought to be
>      path_exists() or file_exists_as_file() and convert as needed
>
>   4. When there are no more file_exists() calls left, all
>      file_exists_as_file() instances can be renamed to file_exists().
>
> But as with any "audit each..." plan, that leaves topics in flight out
> of luck. If we want to be kind to those, we'd have to wait a long while
> to shake out any file_exists() callers.
>
> At which point is there much value in having path_exists() as a wrapper?
> It's a better name, perhaps, but I think my future-proofing against
> "file_exists() may become file-specific" was probably overly paranoid. I
> don't think we could sanely do that conversion without risking breakage.

If we want to, the endgame can be to have a single path_exists_as()
helper that could be used like so:

	#define PATH_TYPE_IFREG	(1<<0)
	#define PATH_TYPE_IFLNK (1<<1)
	#define PATH_TYPE_IFDIR (1<<2)
	#define PATY_TYPE_IFANY ((1<<3)-1)

	#define path_exists(path) path_exists_as(path, PATH_TYPE_ANY)
	#define path_exists_as_file(path) path_exists_as(path, PATH_TYPE_IFREG)

	/* backward compatibility */
	#define file_exists(path) path_exists_as(path, PATH_TYPE_ANY)

We can avoid "file-exists used to mean this thing but in a distant
future it means completely different thing" that way.

> Maybe we should just document its behavior and use it here, rather than
> introducing this new dir_exists().

Sounds good enough to me ;-)