Git removes existing folder when cancelling clone

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

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

> 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);
>  	}

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

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

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

> [...]
> 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
> +'

> > 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 &&

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