From 399f774109b2d493db011a4c34845e39ecbaa5d5 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 2 Jan 2018 15:12:14 -0500 Subject: [PATCH 1/4] t5600: fix outdated comment about unborn HEAD 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 my temporarily removing its objects, which means we need to have some objects to move. --- 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 4435693bb2ca48..f23f92e5a731d0 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 From 153b19ee7786bb70b957fe5147c83a1e85e64660 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 2 Jan 2018 15:19:22 -0500 Subject: [PATCH 2/4] t5600: modernize style 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 --- 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 f23f92e5a731d0..7b2a8052f893fa 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 From 3dd2f0da11103ae0177253b3f9a92f7dde3f61de Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 2 Jan 2018 15:55:42 -0500 Subject: [PATCH 3/4] clone: factor out dir_exists() helper Two parts of git-clone's setup logic check whether a directory exists, and they both call stat directly 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 call 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 actual files. --- builtin/clone.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index 2c38df221b6911..d3b21e3876a183 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; @@ -940,7 +945,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); @@ -951,7 +956,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); } From b50a2181f38958a3a9559c9f2560d2e03da5295e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 2 Jan 2018 16:00:51 -0500 Subject: [PATCH 4/4] clone: do not clean up directories we didn't create 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). 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 --- 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 d3b21e3876a183..600fbe8aa3f1c4 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -472,7 +472,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, @@ -501,12 +503,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); } @@ -974,14 +976,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 7b2a8052f893fa..4a1a912e032983 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, wt)' ' + 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