{"thread":{"id":"47522","subject":"Git removes existing folder when cancelling clone","startedAt":"2018-01-02T11:10:36Z","lastAt":"2018-01-05T19:19:26Z","messageCount":16,"participants":["Stephan Janssen","Robert P. J. Day","Jeff King","Eric Sunshine","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"335612","messageId":"FE0E0877-B77D-4BD6-A513-435C251D920A@you-get.com","threadId":"47522","inReplyTo":null,"subject":"Git removes existing folder when cancelling clone","fromName":"Stephan Janssen","fromEmail":"sjanssen@you-get.com","sentAt":"2018-01-02T11:10:24Z","receivedAt":"2018-01-02T11:10:36Z","isPatch":false,"sender":{"key":"sjanssen@you-get.com","avatar":null},"body":"Hi,\n\nI hope this mailing list is the right way to communicate this.\n\nI just noticed the following behaviour on macOS 10.13.2 running Git v2.15.0:\n\n1. `mkdir new-folder`\n2. `ls` - shows new-folder\n3. `git clone [repo] new-folder`\n4. Git asks for password\n5. I cancel by pressing ctrl+c\n\nResult:\n`ls` no longer shows new-folder.\n\nExpected result:\n`ls` shows new-folder\n\nI’m not sure whether this might be a case of ‘works as intended’, but it’s not what I’d expect.\n\nKind regards,\nStephan Janssen"},{"id":"335614","messageId":"alpine.LFD.2.21.1801020610400.14363@localhost.localdomain","threadId":"47522","inReplyTo":"FE0E0877-B77D-4BD6-A513-435C251D920A@you-get.com","subject":"Re: Git removes existing folder when cancelling clone","fromName":"Robert P. J. Day","fromEmail":"rpjday@crashcourse.ca","sentAt":"2018-01-02T11:12:35Z","receivedAt":"2018-01-02T11:56:03Z","isPatch":false,"sender":{"key":"rpjday@crashcourse.ca","avatar":"https://avatars.githubusercontent.com/u/226084077?v=4"},"body":"On Tue, 2 Jan 2018, Stephan Janssen wrote:\n\n> Hi,\n>\n> I hope this mailing list is the right way to communicate this.\n>\n> I just noticed the following behaviour on macOS 10.13.2 running Git v2.15.0:\n>\n> 1. `mkdir new-folder`\n> 2. `ls` - shows new-folder\n> 3. `git clone [repo] new-folder`\n> 4. Git asks for password\n> 5. I cancel by pressing ctrl+c\n>\n> Result:\n> `ls` no longer shows new-folder.\n>\n> Expected result:\n> `ls` shows new-folder\n>\n> I’m not sure whether this might be a case of ‘works as intended’,\n> but it’s not what I’d expect.\n\n  i'm *pretty* sure that the optional directory name you supply is\nmeant to represent a directory you want git to *create* for you, not\none that already exists. that means the behaviour you see makes sense\n-- if git assumes it was supposed to create the directory, and you\ncancel the clone, it reasonably assumes it should get rid of it.\n\n  i am willing to be corrected.\n\nrday\n\n-- \n\n========================================================================\nRobert P. J. Day                                 Ottawa, Ontario, CANADA\n                        http://crashcourse.ca\n\nTwitter:                                       http://twitter.com/rpjday\nLinkedIn:                               http://ca.linkedin.com/in/rpjday\n========================================================================"},{"id":"335627","messageId":"20180102200443.GA1703@sigill.intra.peff.net","threadId":"47522","inReplyTo":"alpine.LFD.2.21.1801020610400.14363@localhost.localdomain","subject":"Re: Git removes existing folder when cancelling clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-02T20:04:43Z","receivedAt":"2018-01-02T20:04:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 02, 2018 at 06:12:35AM -0500, Robert P. J. Day wrote:\n\n> > I just noticed the following behaviour on macOS 10.13.2 running Git v2.15.0:\n> >\n> > 1. `mkdir new-folder`\n> > 2. `ls` - shows new-folder\n> > 3. `git clone [repo] new-folder`\n> > 4. Git asks for password\n> > 5. I cancel by pressing ctrl+c\n> >\n> > Result:\n> > `ls` no longer shows new-folder.\n> >\n> > Expected result:\n> > `ls` shows new-folder\n> >\n> > I’m not sure whether this might be a case of ‘works as intended’,\n> > but it’s not what I’d expect.\n> \n>   i'm *pretty* sure that the optional directory name you supply is\n> meant to represent a directory you want git to *create* for you, not\n> one that already exists. that means the behaviour you see makes sense\n> -- if git assumes it was supposed to create the directory, and you\n> cancel the clone, it reasonably assumes it should get rid of it.\n\nCorrect. In the early days we required that the \"new-folder\" directory\nnot exist, and the initial \"git clone\" would have bailed in that case.\nAny directory we removed would have been one we created.\n\nThat changed in 55892d2398 (Allow cloning to an existing empty\ndirectory, 2009-01-11), and we continue to allow only empty directories.\n\nSo I don't think there's an urgent data-loss bug here; we will only ever\ndestroy an empty directory. However, the original intent was to leave\nthe filesystem as we found it on a failed or aborted clone, and we\nobviously don't do that in this case. So it might be nice if we could\nrestore it to an empty directory.\n\nRestoring the original contents in the general case is hard, since other\nparts of the clone may have created arbitrary files. But if we know the\noriginal is just empty, we can simply delete everything in it, but not\nthe outer directory.\n\n-Peff\n"},{"id":"335629","messageId":"20180102210753.GA10430@sigill.intra.peff.net","threadId":"47522","inReplyTo":"20180102200443.GA1703@sigill.intra.peff.net","subject":"[PATCH 0/4] Git removes existing folder when cancelling clone","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-02T21:07:53Z","receivedAt":"2018-01-02T21:08:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 02, 2018 at 03:04:43PM -0500, Jeff King wrote:\n\n> So I don't think there's an urgent data-loss bug here; we will only ever\n> destroy an empty directory. However, the original intent was to leave\n> the filesystem as we found it on a failed or aborted clone, and we\n> obviously don't do that in this case. So it might be nice if we could\n> restore it to an empty directory.\n\nHere's a patch series to do that. The first three are just preparatory\ncleanups; the last one is the interesting bit.\n\n  [1/4]: t5600: fix outdated comment about unborn HEAD\n  [2/4]: t5600: modernize style\n  [3/4]: clone: factor out dir_exists() helper\n  [4/4]: clone: do not clean up directories we didn't create\n\n builtin/clone.c               |  31 ++++++++++---\n t/t5600-clone-fail-cleanup.sh | 100 +++++++++++++++++++++++++++++++-----------\n 2 files changed, 98 insertions(+), 33 deletions(-)\n\n-Peff\n"},{"id":"335630","messageId":"20180102210832.GA22556@sigill.intra.peff.net","threadId":"47522","inReplyTo":"20180102210753.GA10430@sigill.intra.peff.net","subject":"[PATCH 1/4] t5600: fix outdated comment about unborn HEAD","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-02T21:08:33Z","receivedAt":"2018-01-02T21:08:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Back when this test was written, git-clone could not handle\na repository without any commits. These days it works fine,\nand this comment is out of date.\n\nAt first glance it seems like we could just drop this code\nentirely now, but it's necessary for the final test, which\nwas added later. That test corrupts the repository by\ntemporarily removing its objects, which means we need to\nhave some objects to move.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5600-clone-fail-cleanup.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh\nindex 4435693bb2..f23f92e5a7 100755\n--- a/t/t5600-clone-fail-cleanup.sh\n+++ b/t/t5600-clone-fail-cleanup.sh\n@@ -22,7 +22,7 @@ test_expect_success \\\n # Need a repo to clone\n test_create_repo foo\n \n-# clone doesn't like it if there is no HEAD. Is that a bug?\n+# create some objects so that we can corrupt the repo later\n (cd foo && touch file && git add file && git commit -m 'add file' >/dev/null 2>&1)\n \n # source repository given to git clone should be relative to the\n-- \n2.16.0.rc0.384.gc477e89267\n\n"},{"id":"335631","messageId":"20180102210859.GB22556@sigill.intra.peff.net","threadId":"47522","inReplyTo":"20180102210753.GA10430@sigill.intra.peff.net","subject":"[PATCH 2/4] t5600: modernize style","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-02T21:09:00Z","receivedAt":"2018-01-02T21:09:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is an old script which could use some updating before\nwe add to it:\n\n  - use the standard line-breaking:\n\n      test_expect_success 'title' '\n              body\n      '\n\n  - run all code inside test_expect blocks to catch\n    unexpected failures in setup steps\n\n  - use \"test_commit -C\" instead of manually entering\n    sub-repo\n\n  - use test_when_finished for cleanup steps\n\n  - test_path_is_* as appropriate\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5600-clone-fail-cleanup.sh | 48 ++++++++++++++++++++++---------------------\n 1 file changed, 25 insertions(+), 23 deletions(-)\n\ndiff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh\nindex f23f92e5a7..7b2a8052f8 100755\n--- a/t/t5600-clone-fail-cleanup.sh\n+++ b/t/t5600-clone-fail-cleanup.sh\n@@ -11,42 +11,44 @@ remove the directory before attempting a clone again.'\n \n . ./test-lib.sh\n \n-test_expect_success \\\n-    'clone of non-existent source should fail' \\\n-    'test_must_fail git clone foo bar'\n+test_expect_success 'clone of non-existent source should fail' '\n+\ttest_must_fail git clone foo bar\n+'\n \n-test_expect_success \\\n-    'failed clone should not leave a directory' \\\n-    '! test -d bar'\n+test_expect_success 'failed clone should not leave a directory' '\n+\ttest_path_is_missing bar\n+'\n \n-# Need a repo to clone\n-test_create_repo foo\n+test_expect_success 'create a repo to clone' '\n+\ttest_create_repo foo\n+'\n \n-# create some objects so that we can corrupt the repo later\n-(cd foo && touch file && git add file && git commit -m 'add file' >/dev/null 2>&1)\n+test_expect_success 'create objects in repo for later corruption' '\n+\ttest_commit -C foo file\n+'\n \n # source repository given to git clone should be relative to the\n # current path not to the target dir\n-test_expect_success \\\n-    'clone of non-existent (relative to $PWD) source should fail' \\\n-    'test_must_fail git clone ../foo baz'\n+test_expect_success 'clone of non-existent (relative to $PWD) source should fail' '\n+\ttest_must_fail git clone ../foo baz\n+'\n \n-test_expect_success \\\n-    'clone should work now that source exists' \\\n-    'git clone foo bar'\n+test_expect_success 'clone should work now that source exists' '\n+\tgit clone foo bar\n+'\n \n-test_expect_success \\\n-    'successful clone must leave the directory' \\\n-    'test -d bar'\n+test_expect_success 'successful clone must leave the directory' '\n+\ttest_path_is_dir bar\n+'\n \n test_expect_success 'failed clone --separate-git-dir should not leave any directories' '\n+\ttest_when_finished \"rmdir foo/.git/objects.bak\" &&\n \tmkdir foo/.git/objects.bak/ &&\n+\ttest_when_finished \"mv foo/.git/objects.bak/* foo/.git/objects/\" &&\n \tmv foo/.git/objects/* foo/.git/objects.bak/ &&\n \ttest_must_fail git clone --separate-git-dir gitdir foo worktree &&\n-\ttest_must_fail test -e gitdir &&\n-\ttest_must_fail test -e worktree &&\n-\tmv foo/.git/objects.bak/* foo/.git/objects/ &&\n-\trmdir foo/.git/objects.bak\n+\ttest_path_is_missing gitdir &&\n+\ttest_path_is_missing worktree\n '\n \n test_done\n-- \n2.16.0.rc0.384.gc477e89267\n\n"},{"id":"335632","messageId":"20180102211014.GC22556@sigill.intra.peff.net","threadId":"47522","inReplyTo":"20180102210753.GA10430@sigill.intra.peff.net","subject":"[PATCH 3/4] clone: factor out dir_exists() helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-02T21:10:14Z","receivedAt":"2018-01-02T21:10:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Two parts of git-clone's setup logic check whether a\ndirectory exists, and they both call stat directly with the\nsame scratch \"struct stat\" buffer. Let's pull that into a\nhelper, which has a few advantages:\n\n  - it makes the purpose of the stat calls more obvious\n\n  - it makes it clear that we don't care about the\n    information in \"buf\" remaining valid\n\n  - if we later decide to make the check more robust (e.g.,\n    complaining about non-directories), we can do it in one\n    place\n\nNote that we could just use file_exists() for this, which\nhas identical code. But we specifically care about\ndirectories, so this future-proofs us against that function\nlater getting more picky about seeing actual files.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/clone.c | 11 ++++++++---\n 1 file changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 2da71db107..04b0d7283f 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -863,10 +863,15 @@ static void dissociate_from_references(void)\n \tfree(alternates);\n }\n \n+static int dir_exists(const char *path)\n+{\n+\tstruct stat sb;\n+\treturn !stat(path, &sb);\n+}\n+\n int cmd_clone(int argc, const char **argv, const char *prefix)\n {\n \tint is_bundle = 0, is_local;\n-\tstruct stat buf;\n \tconst char *repo_name, *repo, *work_tree, *git_dir;\n \tchar *path, *dir;\n \tint dest_exists;\n@@ -938,7 +943,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tdir = guess_dir_name(repo_name, is_bundle, option_bare);\n \tstrip_trailing_slashes(dir);\n \n-\tdest_exists = !stat(dir, &buf);\n+\tdest_exists = dir_exists(dir);\n \tif (dest_exists && !is_empty_dir(dir))\n \t\tdie(_(\"destination path '%s' already exists and is not \"\n \t\t\t\"an empty directory.\"), dir);\n@@ -949,7 +954,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\twork_tree = NULL;\n \telse {\n \t\twork_tree = getenv(\"GIT_WORK_TREE\");\n-\t\tif (work_tree && !stat(work_tree, &buf))\n+\t\tif (work_tree && dir_exists(work_tree))\n \t\t\tdie(_(\"working tree '%s' already exists.\"), work_tree);\n \t}\n \n-- \n2.16.0.rc0.384.gc477e89267\n\n"},{"id":"335633","messageId":"20180102211139.GD22556@sigill.intra.peff.net","threadId":"47522","inReplyTo":"20180102210753.GA10430@sigill.intra.peff.net","subject":"[PATCH 4/4] clone: do not clean up directories we didn't create","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-02T21:11:39Z","receivedAt":"2018-01-02T21:11:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Once upon a time, git-clone would refuse to write into a\ndirectory that it did not itself create. The cleanup\nroutines for a failed clone could therefore just remove the\ngit and worktree dirs completely.\n\nIn 55892d2398 (Allow cloning to an existing empty directory,\n2009-01-11), we learned to write into an existing directory.\nWhich means that doing:\n\n  mkdir foo\n  git clone will-fail foo\n\nends up deleting foo. This isn't a huge catastrophe, since\nby definition foo must be empty. But it's somewhat\nconfusing; we should leave the filesystem as we found it.\n\nBecause we know that the only directory we'll write into is\nan empty one, we can handle this case by just passing the\nKEEP_TOPLEVEL flag to our recursive delete (if we could\nwrite into populated directories, we'd have to keep track of\nwhat we wrote and what we did not, which would be much\nharder).\n\nNote that we need to handle the work-tree and git-dir\nseparately, though, as only one might exist (and the new\ntests in t5600 cover all cases).\n\nReported-by: Stephan Janssen <sjanssen@you-get.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/clone.c               | 20 ++++++++++++----\n t/t5600-clone-fail-cleanup.sh | 56 +++++++++++++++++++++++++++++++++++++++----\n 2 files changed, 67 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 04b0d7283f..284651797e 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -473,7 +473,9 @@ static void clone_local(const char *src_repo, const char *dest_repo)\n }\n \n static const char *junk_work_tree;\n+static int junk_work_tree_flags;\n static const char *junk_git_dir;\n+static int junk_git_dir_flags;\n static enum {\n \tJUNK_LEAVE_NONE,\n \tJUNK_LEAVE_REPO,\n@@ -502,12 +504,12 @@ static void remove_junk(void)\n \n \tif (junk_git_dir) {\n \t\tstrbuf_addstr(&sb, junk_git_dir);\n-\t\tremove_dir_recursively(&sb, 0);\n+\t\tremove_dir_recursively(&sb, junk_git_dir_flags);\n \t\tstrbuf_reset(&sb);\n \t}\n \tif (junk_work_tree) {\n \t\tstrbuf_addstr(&sb, junk_work_tree);\n-\t\tremove_dir_recursively(&sb, 0);\n+\t\tremove_dir_recursively(&sb, junk_work_tree_flags);\n \t}\n \tstrbuf_release(&sb);\n }\n@@ -972,14 +974,24 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tif (safe_create_leading_directories_const(work_tree) < 0)\n \t\t\tdie_errno(_(\"could not create leading directories of '%s'\"),\n \t\t\t\t  work_tree);\n-\t\tif (!dest_exists && mkdir(work_tree, 0777))\n+\t\tif (dest_exists)\n+\t\t\tjunk_work_tree_flags |= REMOVE_DIR_KEEP_TOPLEVEL;\n+\t\telse if (mkdir(work_tree, 0777))\n \t\t\tdie_errno(_(\"could not create work tree dir '%s'\"),\n \t\t\t\t  work_tree);\n \t\tjunk_work_tree = work_tree;\n \t\tset_git_work_tree(work_tree);\n \t}\n \n-\tjunk_git_dir = real_git_dir ? real_git_dir : git_dir;\n+\tif (real_git_dir) {\n+\t\tif (dir_exists(real_git_dir))\n+\t\t\tjunk_git_dir_flags |= REMOVE_DIR_KEEP_TOPLEVEL;\n+\t\tjunk_git_dir = real_git_dir;\n+\t} else {\n+\t\tif (dest_exists)\n+\t\t\tjunk_git_dir_flags |= REMOVE_DIR_KEEP_TOPLEVEL;\n+\t\tjunk_git_dir = git_dir;\n+\t}\n \tif (safe_create_leading_directories_const(git_dir) < 0)\n \t\tdie(_(\"could not create leading directories of '%s'\"), git_dir);\n \ndiff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh\nindex 7b2a8052f8..5cd94d5558 100755\n--- a/t/t5600-clone-fail-cleanup.sh\n+++ b/t/t5600-clone-fail-cleanup.sh\n@@ -7,10 +7,21 @@ test_description='test git clone to cleanup after failure\n \n This test covers the fact that if git clone fails, it should remove\n the directory it created, to avoid the user having to manually\n-remove the directory before attempting a clone again.'\n+remove the directory before attempting a clone again.\n+\n+Unless the directory already exists, in which case we clean up only what we\n+wrote.\n+'\n \n . ./test-lib.sh\n \n+corrupt_repo () {\n+\ttest_when_finished \"rmdir foo/.git/objects.bak\" &&\n+\tmkdir foo/.git/objects.bak/ &&\n+\ttest_when_finished \"mv foo/.git/objects.bak/* foo/.git/objects/\" &&\n+\tmv foo/.git/objects/* foo/.git/objects.bak/\n+}\n+\n test_expect_success 'clone of non-existent source should fail' '\n \ttest_must_fail git clone foo bar\n '\n@@ -42,13 +53,48 @@ test_expect_success 'successful clone must leave the directory' '\n '\n \n test_expect_success 'failed clone --separate-git-dir should not leave any directories' '\n-\ttest_when_finished \"rmdir foo/.git/objects.bak\" &&\n-\tmkdir foo/.git/objects.bak/ &&\n-\ttest_when_finished \"mv foo/.git/objects.bak/* foo/.git/objects/\" &&\n-\tmv foo/.git/objects/* foo/.git/objects.bak/ &&\n+\tcorrupt_repo &&\n \ttest_must_fail git clone --separate-git-dir gitdir foo worktree &&\n \ttest_path_is_missing gitdir &&\n \ttest_path_is_missing worktree\n '\n \n+test_expect_success 'failed clone into empty leaves directory (vanilla)' '\n+\tmkdir -p empty &&\n+\tcorrupt_repo &&\n+\ttest_must_fail git clone foo empty &&\n+\ttest_dir_is_empty empty\n+'\n+\n+test_expect_success 'failed clone into empty leaves directory (bare)' '\n+\tmkdir -p empty &&\n+\tcorrupt_repo &&\n+\ttest_must_fail git clone --bare foo empty &&\n+\ttest_dir_is_empty empty\n+'\n+\n+test_expect_success 'failed clone into empty leaves directory (separate)' '\n+\tmkdir -p empty-git empty-wt &&\n+\tcorrupt_repo &&\n+\ttest_must_fail git clone --separate-git-dir empty-git foo empty-wt &&\n+\ttest_dir_is_empty empty-git &&\n+\ttest_dir_is_empty empty-wt\n+'\n+\n+test_expect_success 'failed clone into empty leaves directory (separate, git)' '\n+\tmkdir -p empty-git &&\n+\tcorrupt_repo &&\n+\ttest_must_fail git clone --separate-git-dir empty-git foo no-wt &&\n+\ttest_dir_is_empty empty-git &&\n+\ttest_path_is_missing no-wt\n+'\n+\n+test_expect_success 'failed clone into empty leaves directory (separate, git)' '\n+\tmkdir -p empty-wt &&\n+\tcorrupt_repo &&\n+\ttest_must_fail git clone --separate-git-dir no-git foo empty-wt &&\n+\ttest_path_is_missing no-git &&\n+\ttest_dir_is_empty empty-wt\n+'\n+\n test_done\n-- \n2.16.0.rc0.384.gc477e89267\n"},{"id":"335640","messageId":"CAPig+cQOU7d21kDELtFAkM0qJh8pvj7F56oWVYw+kYiUftQEuA@mail.gmail.com","threadId":"47522","inReplyTo":"20180102211139.GD22556@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] clone: do not clean up directories we didn't create","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-01-02T22:49:45Z","receivedAt":"2018-01-02T22:49:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jan 2, 2018 at 4:11 PM, Jeff King <peff@peff.net> wrote:\n> [...]\n> Because we know that the only directory we'll write into is\n> an empty one, we can handle this case by just passing the\n> KEEP_TOPLEVEL flag to our recursive delete (if we could\n> write into populated directories, we'd have to keep track of\n> what we wrote and what we did not, which would be much\n> harder).\n>\n> Note that we need to handle the work-tree and git-dir\n> separately, though, as only one might exist (and the new\n> tests in t5600 cover all cases).\n>\n> Reported-by: Stephan Janssen <sjanssen@you-get.com>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh\n> @@ -42,13 +53,48 @@ test_expect_success 'successful clone must leave the directory' '\n> +test_expect_success 'failed clone into empty leaves directory (separate, git)' '\n> +       mkdir -p empty-git &&\n> +       corrupt_repo &&\n> +       test_must_fail git clone --separate-git-dir empty-git foo no-wt &&\n> +       test_dir_is_empty empty-git &&\n> +       test_path_is_missing no-wt\n> +'\n> +\n> +test_expect_success 'failed clone into empty leaves directory (separate, git)' '\n> +       mkdir -p empty-wt &&\n> +       corrupt_repo &&\n> +       test_must_fail git clone --separate-git-dir no-git foo empty-wt &&\n> +       test_path_is_missing no-git &&\n> +       test_dir_is_empty empty-wt\n> +'\n\nThe final two tests seem to have the same title...\n"},{"id":"335644","messageId":"20180102233932.GA27884@sigill.intra.peff.net","threadId":"47522","inReplyTo":"CAPig+cQOU7d21kDELtFAkM0qJh8pvj7F56oWVYw+kYiUftQEuA@mail.gmail.com","subject":"Re: [PATCH 4/4] clone: do not clean up directories we didn't create","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-02T23:39:32Z","receivedAt":"2018-01-02T23:39:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 02, 2018 at 05:49:45PM -0500, Eric Sunshine wrote:\n\n> > diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh\n> > @@ -42,13 +53,48 @@ test_expect_success 'successful clone must leave the directory' '\n> > +test_expect_success 'failed clone into empty leaves directory (separate, git)' '\n> > +       mkdir -p empty-git &&\n> > +       corrupt_repo &&\n> > +       test_must_fail git clone --separate-git-dir empty-git foo no-wt &&\n> > +       test_dir_is_empty empty-git &&\n> > +       test_path_is_missing no-wt\n> > +'\n> > +\n> > +test_expect_success 'failed clone into empty leaves directory (separate, git)' '\n> > +       mkdir -p empty-wt &&\n> > +       corrupt_repo &&\n> > +       test_must_fail git clone --separate-git-dir no-git foo empty-wt &&\n> > +       test_path_is_missing no-git &&\n> > +       test_dir_is_empty empty-wt\n> > +'\n> \n> The final two tests seem to have the same title...\n\nOops. The second one should be \"wt\" (since the idea was to flip the\nlogic from the previous). Like so:\n\ndiff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh\nindex 5cd94d5558..4a1a912e03 100755\n--- a/t/t5600-clone-fail-cleanup.sh\n+++ b/t/t5600-clone-fail-cleanup.sh\n@@ -89,7 +89,7 @@ test_expect_success 'failed clone into empty leaves directory (separate, git)' '\n \ttest_path_is_missing no-wt\n '\n \n-test_expect_success 'failed clone into empty leaves directory (separate, git)' '\n+test_expect_success 'failed clone into empty leaves directory (separate, wt)' '\n \tmkdir -p empty-wt &&\n \tcorrupt_repo &&\n \ttest_must_fail git clone --separate-git-dir no-git foo empty-wt &&\n"},{"id":"335894","messageId":"xmqqbmi9dw55.fsf@gitster.mtv.corp.google.com","threadId":"47522","inReplyTo":"20180102211014.GC22556@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] clone: factor out dir_exists() helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-04T23:47:18Z","receivedAt":"2018-01-04T23:47:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Two parts of git-clone's setup logic check whether a\n> directory exists, and they both call stat directly with the\n> same scratch \"struct stat\" buffer. Let's pull that into a\n> helper, which has a few advantages:\n>\n>   - it makes the purpose of the stat calls more obvious\n>\n>   - it makes it clear that we don't care about the\n>     information in \"buf\" remaining valid\n>\n>   - if we later decide to make the check more robust (e.g.,\n>     complaining about non-directories), we can do it in one\n>     place\n>\n> Note that we could just use file_exists() for this, which\n> has identical code. But we specifically care about\n> directories, so this future-proofs us against that function\n> later getting more picky about seeing actual files.\n\nIt leaves funny taste in my mouth to see that dir_exists() does call\nstat() but does not check st.st_mode to see if it is a directory,\nbut for this particular caller, we want dest_exists() to be true\neven when the thing is a non-directory, so that !is_empty_dir(dir)\ncall is made on the next line to trigger \"exists but not an empty\ndir\" error.  After all, what this caller really wants to ask is \"is\nsomething sitting there?\" and the answer it expects under normal\ncondition is \"no, there is nothing there\".\n\nIf we really want to be anal, perhaps a new helper path_exists()\nthat cares only about existence of paths (i.e. the implementation of\nthese two helpers they currently have), together with update to\ncheck the st.st_mode for file_exists() and dir_exists(), may help\nmaking the API set more rational, but I do not think it is worth it.\n\nThanks.\n\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/clone.c | 11 ++++++++---\n>  1 file changed, 8 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 2da71db107..04b0d7283f 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -863,10 +863,15 @@ static void dissociate_from_references(void)\n>  \tfree(alternates);\n>  }\n>  \n> +static int dir_exists(const char *path)\n> +{\n> +\tstruct stat sb;\n> +\treturn !stat(path, &sb);\n> +}\n> +\n>  int cmd_clone(int argc, const char **argv, const char *prefix)\n>  {\n>  \tint is_bundle = 0, is_local;\n> -\tstruct stat buf;\n>  \tconst char *repo_name, *repo, *work_tree, *git_dir;\n>  \tchar *path, *dir;\n>  \tint dest_exists;\n> @@ -938,7 +943,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \t\tdir = guess_dir_name(repo_name, is_bundle, option_bare);\n>  \tstrip_trailing_slashes(dir);\n>  \n> -\tdest_exists = !stat(dir, &buf);\n> +\tdest_exists = dir_exists(dir);\n>  \tif (dest_exists && !is_empty_dir(dir))\n>  \t\tdie(_(\"destination path '%s' already exists and is not \"\n>  \t\t\t\"an empty directory.\"), dir);\n> @@ -949,7 +954,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \t\twork_tree = NULL;\n>  \telse {\n>  \t\twork_tree = getenv(\"GIT_WORK_TREE\");\n> -\t\tif (work_tree && !stat(work_tree, &buf))\n> +\t\tif (work_tree && dir_exists(work_tree))\n>  \t\t\tdie(_(\"working tree '%s' already exists.\"), work_tree);\n>  \t}\n"},{"id":"335895","messageId":"xmqq7esxdw2x.fsf@gitster.mtv.corp.google.com","threadId":"47522","inReplyTo":"20180102211139.GD22556@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] clone: do not clean up directories we didn't create","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-04T23:48:38Z","receivedAt":"2018-01-04T23:48:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 04b0d7283f..284651797e 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -502,12 +504,12 @@ static void remove_junk(void)\n>  \n>  \tif (junk_git_dir) {\n>  \t\tstrbuf_addstr(&sb, junk_git_dir);\n> -\t\tremove_dir_recursively(&sb, 0);\n> +\t\tremove_dir_recursively(&sb, junk_git_dir_flags);\n>  \t\tstrbuf_reset(&sb);\n>  \t}\n>  \tif (junk_work_tree) {\n>  \t\tstrbuf_addstr(&sb, junk_work_tree);\n> -\t\tremove_dir_recursively(&sb, 0);\n> +\t\tremove_dir_recursively(&sb, junk_work_tree_flags);\n>  \t}\n>  \tstrbuf_release(&sb);\n>  }\n> @@ -972,14 +974,24 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n>  \t\tif (safe_create_leading_directories_const(work_tree) < 0)\n>  \t\t\tdie_errno(_(\"could not create leading directories of '%s'\"),\n>  \t\t\t\t  work_tree);\n> -\t\tif (!dest_exists && mkdir(work_tree, 0777))\n> +\t\tif (dest_exists)\n> +\t\t\tjunk_work_tree_flags |= REMOVE_DIR_KEEP_TOPLEVEL;\n> +\t\telse if (mkdir(work_tree, 0777))\n>  \t\t\tdie_errno(_(\"could not create work tree dir '%s'\"),\n>  \t\t\t\t  work_tree);\n>  \t\tjunk_work_tree = work_tree;\n>  \t\tset_git_work_tree(work_tree);\n>  \t}\n>  \n> -\tjunk_git_dir = real_git_dir ? real_git_dir : git_dir;\n> +\tif (real_git_dir) {\n> +\t\tif (dir_exists(real_git_dir))\n> +\t\t\tjunk_git_dir_flags |= REMOVE_DIR_KEEP_TOPLEVEL;\n> +\t\tjunk_git_dir = real_git_dir;\n> +\t} else {\n> +\t\tif (dest_exists)\n> +\t\t\tjunk_git_dir_flags |= REMOVE_DIR_KEEP_TOPLEVEL;\n> +\t\tjunk_git_dir = git_dir;\n> +\t}\n>  \tif (safe_create_leading_directories_const(git_dir) < 0)\n>  \t\tdie(_(\"could not create leading directories of '%s'\"), git_dir);\n\nThe changes all look reasonable.\n\nThanks.\n"},{"id":"335896","messageId":"20180104235412.GA3474@sigill.intra.peff.net","threadId":"47522","inReplyTo":"xmqqbmi9dw55.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/4] clone: factor out dir_exists() helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-04T23:54:12Z","receivedAt":"2018-01-04T23:54:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 04, 2018 at 03:47:18PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Two parts of git-clone's setup logic check whether a\n> > directory exists, and they both call stat directly with the\n> > same scratch \"struct stat\" buffer. Let's pull that into a\n> > helper, which has a few advantages:\n> >\n> >   - it makes the purpose of the stat calls more obvious\n> >\n> >   - it makes it clear that we don't care about the\n> >     information in \"buf\" remaining valid\n> >\n> >   - if we later decide to make the check more robust (e.g.,\n> >     complaining about non-directories), we can do it in one\n> >     place\n> >\n> > Note that we could just use file_exists() for this, which\n> > has identical code. But we specifically care about\n> > directories, so this future-proofs us against that function\n> > later getting more picky about seeing actual files.\n> \n> It leaves funny taste in my mouth to see that dir_exists() does call\n> stat() but does not check st.st_mode to see if it is a directory,\n> but for this particular caller, we want dest_exists() to be true\n> even when the thing is a non-directory, so that !is_empty_dir(dir)\n> call is made on the next line to trigger \"exists but not an empty\n> dir\" error.  After all, what this caller really wants to ask is \"is\n> something sitting there?\" and the answer it expects under normal\n> condition is \"no, there is nothing there\".\n\nYeah, that was part of the reason I left this as file-local instead of\nadding it globally.\n\n> If we really want to be anal, perhaps a new helper path_exists()\n> that cares only about existence of paths (i.e. the implementation of\n> these two helpers they currently have), together with update to\n> check the st.st_mode for file_exists() and dir_exists(), may help\n> making the API set more rational, but I do not think it is worth it.\n\nYep, I also considered that file_exists() probably wants to be\npath_exists() with its current implementation. We'd probably want to\nreview all of the callers.\n\nAnyway, I tried to do the minimal refactoring here, with no change in\nbehavior. I'm not opposed to calling this dir_exists() as path_exists()\nand making it globally available (as you note, I don't think we'd want\nto use a true dir_exists() here).\n\n-Peff\n"},{"id":"335898","messageId":"20180105002206.GB3474@sigill.intra.peff.net","threadId":"47522","inReplyTo":"20180104235412.GA3474@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] clone: factor out dir_exists() helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-05T00:22:06Z","receivedAt":"2018-01-05T00:22:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 04, 2018 at 06:54:12PM -0500, Jeff King wrote:\n\n> > If we really want to be anal, perhaps a new helper path_exists()\n> > that cares only about existence of paths (i.e. the implementation of\n> > these two helpers they currently have), together with update to\n> > check the st.st_mode for file_exists() and dir_exists(), may help\n> > making the API set more rational, but I do not think it is worth it.\n> \n> Yep, I also considered that file_exists() probably wants to be\n> path_exists() with its current implementation. We'd probably want to\n> review all of the callers.\n> \n> Anyway, I tried to do the minimal refactoring here, with no change in\n> behavior. I'm not opposed to calling this dir_exists() as path_exists()\n> and making it globally available (as you note, I don't think we'd want\n> to use a true dir_exists() here).\n\nSo I actually started down this road just now, but I'm not sure if it's\nworth it.  If we were to transition to an endgame with path_exists(),\ndir_exists(), and file_exists(), we'd probably want to do something\nlike:\n\n  1. introduce path_exists(), but leave existing file_exists() callers\n     in place\n\n  2. introduce file_exists_as_file(), which checks S_IFREG\n\n  3. audit each file_exists() call to see if it ought to be\n     path_exists() or file_exists_as_file() and convert as needed\n\n  4. When there are no more file_exists() calls left, all\n     file_exists_as_file() instances can be renamed to file_exists().\n\nBut as with any \"audit each...\" plan, that leaves topics in flight out\nof luck. If we want to be kind to those, we'd have to wait a long while\nto shake out any file_exists() callers.\n\nAt which point is there much value in having path_exists() as a wrapper?\nIt's a better name, perhaps, but I think my future-proofing against\n\"file_exists() may become file-specific\" was probably overly paranoid. I\ndon't think we could sanely do that conversion without risking breakage.\n\nMaybe we should just document its behavior and use it here, rather than\nintroducing this new dir_exists().\n\n-Peff\n"},{"id":"335938","messageId":"xmqq373kdtrv.fsf@gitster.mtv.corp.google.com","threadId":"47522","inReplyTo":"20180102233932.GA27884@sigill.intra.peff.net","subject":"Re: [PATCH 4/4] clone: do not clean up directories we didn't create","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-05T18:50:44Z","receivedAt":"2018-01-05T18:50:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Oops. The second one should be \"wt\" (since the idea was to flip the\n> logic from the previous). Like so:\n>\n> diff --git a/t/t5600-clone-fail-cleanup.sh b/t/t5600-clone-fail-cleanup.sh\n> index 5cd94d5558..4a1a912e03 100755\n> --- a/t/t5600-clone-fail-cleanup.sh\n> +++ b/t/t5600-clone-fail-cleanup.sh\n> @@ -89,7 +89,7 @@ test_expect_success 'failed clone into empty leaves directory (separate, git)' '\n>  \ttest_path_is_missing no-wt\n>  '\n>  \n> -test_expect_success 'failed clone into empty leaves directory (separate, git)' '\n> +test_expect_success 'failed clone into empty leaves directory (separate, wt)' '\n>  \tmkdir -p empty-wt &&\n>  \tcorrupt_repo &&\n>  \ttest_must_fail git clone --separate-git-dir no-git foo empty-wt &&\n\nSquashed; thanks.\n"},{"id":"335945","messageId":"xmqqo9m8cdvs.fsf@gitster.mtv.corp.google.com","threadId":"47522","inReplyTo":"20180105002206.GB3474@sigill.intra.peff.net","subject":"Re: [PATCH 3/4] clone: factor out dir_exists() helper","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-05T19:19:19Z","receivedAt":"2018-01-05T19:19:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Jan 04, 2018 at 06:54:12PM -0500, Jeff King wrote:\n>\n>> > If we really want to be anal, perhaps a new helper path_exists()\n>> > that cares only about existence of paths (i.e. the implementation of\n>> > these two helpers they currently have), together with update to\n>> > check the st.st_mode for file_exists() and dir_exists(), may help\n>> > making the API set more rational, but I do not think it is worth it.\n>> \n>> Yep, I also considered that file_exists() probably wants to be\n>> path_exists() with its current implementation. We'd probably want to\n>> review all of the callers.\n>> \n>> Anyway, I tried to do the minimal refactoring here, with no change in\n>> behavior. I'm not opposed to calling this dir_exists() as path_exists()\n>> and making it globally available (as you note, I don't think we'd want\n>> to use a true dir_exists() here).\n>\n> So I actually started down this road just now, but I'm not sure if it's\n> worth it.\n\nYeah, although I said it already without starting down this road,\nyou actually thought about it more and your insight is more valuable\n;-)\n\n> If we were to transition to an endgame with path_exists(),\n> dir_exists(), and file_exists(), we'd probably want to do something\n> like:\n>\n>   1. introduce path_exists(), but leave existing file_exists() callers\n>      in place\n>\n>   2. introduce file_exists_as_file(), which checks S_IFREG\n>\n>   3. audit each file_exists() call to see if it ought to be\n>      path_exists() or file_exists_as_file() and convert as needed\n>\n>   4. When there are no more file_exists() calls left, all\n>      file_exists_as_file() instances can be renamed to file_exists().\n>\n> But as with any \"audit each...\" plan, that leaves topics in flight out\n> of luck. If we want to be kind to those, we'd have to wait a long while\n> to shake out any file_exists() callers.\n>\n> At which point is there much value in having path_exists() as a wrapper?\n> It's a better name, perhaps, but I think my future-proofing against\n> \"file_exists() may become file-specific\" was probably overly paranoid. I\n> don't think we could sanely do that conversion without risking breakage.\n\nIf we want to, the endgame can be to have a single path_exists_as()\nhelper that could be used like so:\n\n\t#define PATH_TYPE_IFREG\t(1<<0)\n\t#define PATH_TYPE_IFLNK (1<<1)\n\t#define PATH_TYPE_IFDIR (1<<2)\n\t#define PATY_TYPE_IFANY ((1<<3)-1)\n\n\t#define path_exists(path) path_exists_as(path, PATH_TYPE_ANY)\n\t#define path_exists_as_file(path) path_exists_as(path, PATH_TYPE_IFREG)\n\n\t/* backward compatibility */\n\t#define file_exists(path) path_exists_as(path, PATH_TYPE_ANY)\n\nWe can avoid \"file-exists used to mean this thing but in a distant\nfuture it means completely different thing\" that way.\n\n> Maybe we should just document its behavior and use it here, rather than\n> introducing this new dir_exists().\n\nSounds good enough to me ;-)\n"}]}