From a4c4efd25159a8b308f9e204bc87e37b963bc2ae Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 2 Jan 2018 16:08:33 -0500 Subject: 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 by temporarily removing its objects, which means we need to have some objects to move. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh index 4435693..f23f92e 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 -- cgit v0.10.2-6-g49f6 From 8486b84f0ea4d7f943fdbe81050553d69d198742 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 2 Jan 2018 16:09:00 -0500 Subject: 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 Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh index f23f92e..7b2a805 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 -- cgit v0.10.2-6-g49f6 From f9e377adc0b1ed06e35d2c77a6c9f2687c5b950b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 2 Jan 2018 16:10:14 -0500 Subject: 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 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 Signed-off-by: Junio C Hamano diff --git a/builtin/clone.c b/builtin/clone.c index dbddd98..3080640 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); } -- cgit v0.10.2-6-g49f6 From d45420c1c8612f085f1901c33ff6f0ccfbb72d3b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 2 Jan 2018 16:11:39 -0500 Subject: 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, 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 Signed-off-by: Junio C Hamano diff --git a/builtin/clone.c b/builtin/clone.c index 3080640..d9b3d1a 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); } @@ -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 7b2a805..4a1a912 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 -- cgit v0.10.2-6-g49f6