{"thread":{"id":"53781","subject":"[PATCH 0/2] git clone with --separate-git-dir destroys existing directory content","startedAt":"2020-07-01T09:46:16Z","lastAt":"2020-07-10T08:47:59Z","messageCount":14,"participants":["Ben Wijen","Eric Sunshine","Ben","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"400873","messageId":"20200701093653.3706-1-ben@wijen.net","threadId":"53781","inReplyTo":null,"subject":"[PATCH 0/2] git clone with --separate-git-dir destroys existing directory content","fromName":"Ben Wijen","fromEmail":"ben@wijen.net","sentAt":"2020-07-01T09:36:51Z","receivedAt":"2020-07-01T09:46:16Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"Hi,\n\nI found an issue with git clone, when using --separate-git-dir and that directory already exists,\nit's content is destroyed.\n\nI have created a patch set on origin/next, but (as I have learned this the hard way) please let me know\nif this needs to be backported...\n\n\nBen...\n\n\nBen Wijen (2):\n git clone: check for non-empty directory\n git clone: don't clone into non-empty directory\n\n builtin/clone.c | 12 ++++++++++--\n t/t5601-clone.sh | 3 ++-\n 2 files changed, 12 insertions(+), 3 deletions(-)\n\n-- \n2.27.0\n\n"},{"id":"400874","messageId":"20200701093653.3706-2-ben@wijen.net","threadId":"53781","inReplyTo":"20200701093653.3706-1-ben@wijen.net","subject":"[PATCH 1/2] git clone: check for non-empty directory","fromName":"Ben Wijen","fromEmail":"ben@wijen.net","sentAt":"2020-07-01T09:36:52Z","receivedAt":"2020-07-01T09:46:18Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"When using git clone with --separate-git-dir realgitdir and\nrealgitdir already exists, it's content is destroyed.\n\nExtend test to make sure content is left intact\n\nSigned-off-by: Ben Wijen \n---\n t/t5601-clone.sh | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 84ea2a3eb7..03901c55f2 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -271,7 +271,8 @@ test_expect_success 'fetch from gitfile parent' '\n \n test_expect_success 'clone separate gitdir where target already exists' '\n \trm -rf dst &&\n-\ttest_must_fail git clone --separate-git-dir realgitdir src dst\n+\ttest_must_fail git clone --separate-git-dir realgitdir src dst &&\n+\ttest -f realgitdir/config\n '\n \n test_expect_success 'clone --reference from original' '\n-- \n2.27.0\n\n"},{"id":"400875","messageId":"20200701093653.3706-3-ben@wijen.net","threadId":"53781","inReplyTo":"20200701093653.3706-1-ben@wijen.net","subject":"[PATCH 2/2] git clone: don't clone into non-empty directory","fromName":"Ben Wijen","fromEmail":"ben@wijen.net","sentAt":"2020-07-01T09:36:53Z","receivedAt":"2020-07-01T09:46:24Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"When using git clone with --separate-git-dir realgitdir and\nrealgitdir already exists, it's content is destroyed.\n\nMake sure we don't clone into an existing non-empty directory.\n\nSigned-off-by: Ben Wijen \n---\n builtin/clone.c | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex bef70745c0..54a91acf06 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -946,7 +946,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tint is_bundle = 0, is_local;\n \tconst char *repo_name, *repo, *work_tree, *git_dir;\n \tchar *path, *dir, *display_repo = NULL;\n-\tint dest_exists;\n+\tint dest_exists, real_dest_exists = 0;\n \tconst struct ref *refs, *remote_head;\n \tconst struct ref *remote_head_points_at;\n \tconst struct ref *our_head_points_at;\n@@ -1021,6 +1021,14 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"destination path '%s' already exists and is not \"\n \t\t\t\"an empty directory.\"), dir);\n \n+\tif (real_git_dir) {\n+\t\treal_dest_exists = path_exists(real_git_dir);\n+\t\tif (real_dest_exists && !is_empty_dir(real_git_dir))\n+\t\t\tdie(_(\"destination path '%s' already exists and is not \"\n+\t\t\t\t\"an empty directory.\"), real_git_dir);\n+\t}\n+\n+\n \tstrbuf_addf(&reflog_msg, \"clone: from %s\",\n \t\t display_repo ? display_repo : repo);\n \tfree(display_repo);\n@@ -1057,7 +1065,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (real_git_dir) {\n-\t\tif (path_exists(real_git_dir))\n+\t\tif (real_dest_exists)\n \t\t\tjunk_git_dir_flags |= REMOVE_DIR_KEEP_TOPLEVEL;\n \t\tjunk_git_dir = real_git_dir;\n \t} else {\n-- \n2.27.0\n\n"},{"id":"400927","messageId":"CAPig+cS6XnUK9MQL68_yoR9Gve4_itmawsd_0-yJr3S5=3-zPQ@mail.gmail.com","threadId":"53781","inReplyTo":"20200701093653.3706-1-ben@wijen.net","subject":"Re: [PATCH 0/2] git clone with --separate-git-dir destroys existing directory content","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-07-01T15:39:46Z","receivedAt":"2020-07-01T15:40:01Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jul 1, 2020 at 5:46 AM Ben Wijen wrote:\n> I found an issue with git clone, when using --separate-git-dir and that directory already exists,\n> it's content is destroyed.\n\nThanks for fixing this.\n\n> I have created a patch set on origin/next, but (as I have learned this the hard way) please let me know\n> if this needs to be backported...\n\nDoes this patch series depend on certain topics in 'next'? If not,\nplease base it on 'master' instead. This will allow it to graduate\nfrom 'seen' to 'next' and eventually 'master' without being held back\nby any other topics in flight. On the other hand, if it does depend\nupon some topic in 'next', then it is a good idea to specify which\nspecific topics it needs. That way, your patches will be able to\ngraduate after those specific topics graduate rather than having to\nwait until _all_ of 'next' graduates (which could take forever).\n"},{"id":"400929","messageId":"CAPig+cT+cXuM3Asu6+Z25pGV2uRm6K1iAeYVhic9kqk1mS84-g@mail.gmail.com","threadId":"53781","inReplyTo":"20200701093653.3706-2-ben@wijen.net","subject":"Re: [PATCH 1/2] git clone: check for non-empty directory","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-07-01T16:00:45Z","receivedAt":"2020-07-01T16:01:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jul 1, 2020 at 5:46 AM Ben Wijen wrote:\n> When using git clone with --separate-git-dir realgitdir and\n> realgitdir already exists, it's content is destroyed.\n>\n> Extend test to make sure content is left intact\n>\n> Signed-off-by: Ben Wijen \n> ---\n> diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\n> @@ -271,7 +271,8 @@ test_expect_success 'fetch from gitfile parent' '\n> test_expect_success 'clone separate gitdir where target already exists' '\n> rm -rf dst &&\n> - test_must_fail git clone --separate-git-dir realgitdir src dst\n> + test_must_fail git clone --separate-git-dir realgitdir src dst &&\n> + test -f realgitdir/config\n> '\n\nThis addresses the immediate problem of the directory content being\ndestroyed, which is good. But, we can future-proof it even more by\nalso verifying (to some degree) that the existing content has not been\ndisturbed. Doing so would give us greater confidence against some\nfuture change breaking \"realgitdir\" in some fashion other than merely\nemptying it. For instance, we might do this:\n\n rm -rf dst &&\n echo foo=bar >realgitdir/config &&\n test_must_fail git clone --separate-git-dir realgitdir src dst &&\n grep foo=bar realgitdir/config\n\nOne other observation: To preserve bisectability (git-bisect) of\ngit.git itself, we want to ensure that the entire test suite still\npasses at each point in a patch series. Making this change to the test\nin its own patch introduces a failure into the test suite, which is\nundesirable. One way to address this shortcoming would be to\ntemporarily change this test from 'test_must_succeed' to\n'test_must_fail', and then flip it back to 'test_must_succeed' in\npatch 2/2. A cleaner approach is simply to combine the two patches so\nthat the fix and updated test are bundled together (which makes sense\nanyhow in this case since this patch is so short and need not stand on\nits own).\n"},{"id":"400931","messageId":"CAPig+cTEpajuD98RH5xepJ+rK2f75SROrTvgjzKnyfjxbuA1AQ@mail.gmail.com","threadId":"53781","inReplyTo":"20200701093653.3706-3-ben@wijen.net","subject":"Re: [PATCH 2/2] git clone: don't clone into non-empty directory","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-07-01T16:10:11Z","receivedAt":"2020-07-01T16:10:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jul 1, 2020 at 5:46 AM Ben Wijen wrote:\n> When using git clone with --separate-git-dir realgitdir and\n> realgitdir already exists, it's content is destroyed.\n>\n> Make sure we don't clone into an existing non-empty directory.\n>\n> Signed-off-by: Ben Wijen \n> ---\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> @@ -1021,6 +1021,14 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> die(_(\"destination path '%s' already exists and is not \"\n> \"an empty directory.\"), dir);\n>\n> + if (real_git_dir) {\n> + real_dest_exists = path_exists(real_git_dir);\n> + if (real_dest_exists && !is_empty_dir(real_git_dir))\n\n\"git init --separate-git-dir=\" checks only whether the path exists,\nand errors out if it does; it doesn't care whether the directory is\nempty or not. I'm wondering, therefore, if this check should be\ntightened to more closely align the behavior of the two commands.\n(Using the tighter semantic now doesn't prohibit loosening it in the\nfuture, whereas it's harder to tighten behavior which started out\nloose.)\n\n> + die(_(\"destination path '%s' already exists and is not \"\n> + \"an empty directory.\"), real_git_dir);\n\nThis message might be a bit clearer if \"destination\" is changed to\n\"repository\". (And drop the bit about not being empty -- if we go that\nroute.)\n"},{"id":"400936","messageId":"CAPig+cRj6p4tdDhQOeoXpoqp+Wb94rKaVim-Yddj_Gz13phQpg@mail.gmail.com","threadId":"53781","inReplyTo":"CAPig+cT+cXuM3Asu6+Z25pGV2uRm6K1iAeYVhic9kqk1mS84-g@mail.gmail.com","subject":"Re: [PATCH 1/2] git clone: check for non-empty directory","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-07-01T17:40:49Z","receivedAt":"2020-07-01T17:41:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jul 1, 2020 at 12:00 PM Eric Sunshine wrote:\n> [...] For instance, we might do this:\n> rm -rf dst &&\n> echo foo=bar >realgitdir/config &&\n\nI meant to use '>>' here, not '>'.\n"},{"id":"400960","messageId":"CAPig+cQwJKSVMP8G7E62bESERYsAFTUN2PBX32YWMhECjU=qTQ@mail.gmail.com","threadId":"53781","inReplyTo":"CAPig+cT+cXuM3Asu6+Z25pGV2uRm6K1iAeYVhic9kqk1mS84-g@mail.gmail.com","subject":"Re: [PATCH 1/2] git clone: check for non-empty directory","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-07-02T05:32:15Z","receivedAt":"2020-07-02T05:32:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Jul 1, 2020 at 12:00 PM Eric Sunshine wrote:\n> One other observation: To preserve bisectability (git-bisect) of\n> git.git itself, we want to ensure that the entire test suite still\n> passes at each point in a patch series. Making this change to the test\n> in its own patch introduces a failure into the test suite, which is\n> undesirable. One way to address this shortcoming would be to\n> temporarily change this test from 'test_must_succeed' to\n> 'test_must_fail', and then flip it back to 'test_must_succeed' in\n> patch 2/2. [...]\n\nAnd another correction: I meant 'test_expect_success' and\n'test_expect_failure', respectively.\n"},{"id":"400962","messageId":"fe053a22-16a1-a071-520c-9bb5be7dc09f@wijen.net","threadId":"53781","inReplyTo":"CAPig+cTEpajuD98RH5xepJ+rK2f75SROrTvgjzKnyfjxbuA1AQ@mail.gmail.com","subject":"Re: [PATCH 2/2] git clone: don't clone into non-empty directory","fromName":"Ben","fromEmail":"ben@wijen.net","sentAt":"2020-07-02T07:50:47Z","receivedAt":"2020-07-02T07:50:59Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"On 01-07-2020 18:10, Eric Sunshine wrote:\n> On Wed, Jul 1, 2020 at 5:46 AM Ben Wijen wrote:\n> \n> \"git init --separate-git-dir=\" checks only whether the path exists,\n> and errors out if it does; it doesn't care whether the directory is\n> empty or not. I'm wondering, therefore, if this check should be\n> tightened to more closely align the behavior of the two commands.\n> (Using the tighter semantic now doesn't prohibit loosening it in the\n> future, whereas it's harder to tighten behavior which started out\n> loose.)\n> \n\nAbout `git init`, I did take a look at it, but saw 'exist_ok' is always\ntrue when called from 'cmd_init_db' (see builtin/init_db.c:654)\nThis means `git init` always allows when git_dir/real_git_dir already exists,\nwhich I understand for the worktree, but I doubt if one wants this for the\nrepository. (when --separate-git-dir is used)\n\nAlso, because 'exist_ok' is always true for `git init` and - with this patch -\n`git clone` checks the git_dir/real_git_dir before the 'init_db' call I'm wondering if\nthe code in 'init_db' (builtin/init-db.c:394-400) can be removed. \n\nThen, even if we do take that route (no 'is_empty_dir', only 'path_exists') we must\nalso address junk_git_dir_flags, as REMOVE_DIR_KEEP_TOPLEVEL would invalidate\na second git clone.\n\nSo, as this patch only fixes the problem at hand, IMHO separate patch-sets should\nbe created for mentioned issues.\n\nBen...\n"},{"id":"400963","messageId":"20200702081335.16786-1-ben@wijen.net","threadId":"53781","inReplyTo":"CAPig+cS6XnUK9MQL68_yoR9Gve4_itmawsd_0-yJr3S5=3-zPQ@mail.gmail.com","subject":"[PATCH v2 0/1] git clone: don't clone into non-empty directory","fromName":"Ben Wijen","fromEmail":"ben@wijen.net","sentAt":"2020-07-02T08:13:34Z","receivedAt":"2020-07-02T08:13:49Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"Eric, thank you for your review.\n\nWith this update I have:\n* rebased (and squashed) on master\n* included your proposals for the test\n* changed \"destination path...\" to \"repository path...\"\n\n\nBen...\n\nBen Wijen (1):\n git clone: don't clone into non-empty directory\n\n builtin/clone.c | 12 ++++++++++--\n t/t5601-clone.sh | 4 +++-\n 2 files changed, 13 insertions(+), 3 deletions(-)\n\n-- \n2.27.0\n\n"},{"id":"400964","messageId":"20200702081335.16786-2-ben@wijen.net","threadId":"53781","inReplyTo":"CAPig+cS6XnUK9MQL68_yoR9Gve4_itmawsd_0-yJr3S5=3-zPQ@mail.gmail.com","subject":"[PATCH v2 1/1] git clone: don't clone into non-empty directory","fromName":"Ben Wijen","fromEmail":"ben@wijen.net","sentAt":"2020-07-02T08:13:35Z","receivedAt":"2020-07-02T08:14:01Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"When using git clone with --separate-git-dir realgitdir and\nrealgitdir already exists, it's content is destroyed.\n\nSo, make sure we don't clone into an existing non-empty directory.\n\nSigned-off-by: Ben Wijen \n---\n builtin/clone.c | 12 ++++++++++--\n t/t5601-clone.sh | 4 +++-\n 2 files changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 2a8e3aaaed..e470193bb8 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -946,7 +946,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tint is_bundle = 0, is_local;\n \tconst char *repo_name, *repo, *work_tree, *git_dir;\n \tchar *path, *dir, *display_repo = NULL;\n-\tint dest_exists;\n+\tint dest_exists, real_dest_exists = 0;\n \tconst struct ref *refs, *remote_head;\n \tconst struct ref *remote_head_points_at;\n \tconst struct ref *our_head_points_at;\n@@ -1021,6 +1021,14 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"destination path '%s' already exists and is not \"\n \t\t\t\"an empty directory.\"), dir);\n \n+\tif (real_git_dir) {\n+\t\treal_dest_exists = path_exists(real_git_dir);\n+\t\tif (real_dest_exists && !is_empty_dir(real_git_dir))\n+\t\t\tdie(_(\"repository path '%s' already exists and is not \"\n+\t\t\t\t\"an empty directory.\"), real_git_dir);\n+\t}\n+\n+\n \tstrbuf_addf(&reflog_msg, \"clone: from %s\",\n \t\t display_repo ? display_repo : repo);\n \tfree(display_repo);\n@@ -1057,7 +1065,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (real_git_dir) {\n-\t\tif (path_exists(real_git_dir))\n+\t\tif (real_dest_exists)\n \t\t\tjunk_git_dir_flags |= REMOVE_DIR_KEEP_TOPLEVEL;\n \t\tjunk_git_dir = real_git_dir;\n \t} else {\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 84ea2a3eb7..eb9a093e25 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -271,7 +271,9 @@ test_expect_success 'fetch from gitfile parent' '\n \n test_expect_success 'clone separate gitdir where target already exists' '\n \trm -rf dst &&\n-\ttest_must_fail git clone --separate-git-dir realgitdir src dst\n+\techo foo=bar >>realgitdir/config &&\n+\ttest_must_fail git clone --separate-git-dir realgitdir src dst &&\n+\tgrep foo=bar realgitdir/config\n '\n \n test_expect_success 'clone --reference from original' '\n-- \n2.27.0\n\n"},{"id":"401057","messageId":"xmqqv9j08dxo.fsf@gitster.c.googlers.com","threadId":"53781","inReplyTo":"20200702081335.16786-2-ben@wijen.net","subject":"Re: [PATCH v2 1/1] git clone: don't clone into non-empty directory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-06T22:40:51Z","receivedAt":"2020-07-06T22:40:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ben Wijen writes:\n\n> When using git clone with --separate-git-dir realgitdir and\n> realgitdir already exists, it's content is destroyed.\n>\n> So, make sure we don't clone into an existing non-empty directory.\n\nThis came from d45420c1 (clone: do not clean up directories we\ndidn't create, 2018-01-02), where we claimed\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\nThe assumed use case is that somebody created an empty directory for\nour use in a grandparent directory where we have no write permission\nand gave it to us, and we want to do a \"git clone\" into it while\nkeeping that directory alive. But we didn't make sure the directory\nwas not empty ourselves---we just assumed it to be empty.\n\nAnd the originally envisioned use case does not have anything to do\nwith the use of --separate-git-dir at all. So from that point of\nview, this change does not break the originally envisioned use case,\nso this probably is not regressing any existing and valid use case\nthat may not involve \"--separate-git-dir\", but I'd rather see that\nthe commit message explain these things to its readers to justify\nitself.\n\nThanks.\n\n"},{"id":"401335","messageId":"20200710084732.5152-2-ben@wijen.net","threadId":"53781","inReplyTo":"xmqqv9j08dxo.fsf@gitster.c.googlers.com","subject":"[PATCH v3 1/1] git clone: don't clone into non-empty directory","fromName":"Ben Wijen","fromEmail":"ben@wijen.net","sentAt":"2020-07-10T08:47:32Z","receivedAt":"2020-07-10T08:47:48Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"When using git clone with --separate-git-dir realgitdir and\nrealgitdir already exists, it's content is destroyed.\n\nSo, make sure we don't clone into an existing non-empty directory.\n\nNote: #d45420c1: \"clone: do not clean up directories we didn't create\"\nassumed we always write into an empty directory, but missed the check\nfor real_git_dir, this commit fixed it by adding the check.\n\nSigned-off-by: Ben Wijen \n---\n builtin/clone.c | 12 ++++++++++--\n t/t5601-clone.sh | 4 +++-\n 2 files changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex bef70745c0..a9f3312a54 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -946,7 +946,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \tint is_bundle = 0, is_local;\n \tconst char *repo_name, *repo, *work_tree, *git_dir;\n \tchar *path, *dir, *display_repo = NULL;\n-\tint dest_exists;\n+\tint dest_exists, real_dest_exists = 0;\n \tconst struct ref *refs, *remote_head;\n \tconst struct ref *remote_head_points_at;\n \tconst struct ref *our_head_points_at;\n@@ -1021,6 +1021,14 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"destination path '%s' already exists and is not \"\n \t\t\t\"an empty directory.\"), dir);\n \n+\tif (real_git_dir) {\n+\t\treal_dest_exists = path_exists(real_git_dir);\n+\t\tif (real_dest_exists && !is_empty_dir(real_git_dir))\n+\t\t\tdie(_(\"repository path '%s' already exists and is not \"\n+\t\t\t\t\"an empty directory.\"), real_git_dir);\n+\t}\n+\n+\n \tstrbuf_addf(&reflog_msg, \"clone: from %s\",\n \t\t display_repo ? display_repo : repo);\n \tfree(display_repo);\n@@ -1057,7 +1065,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (real_git_dir) {\n-\t\tif (path_exists(real_git_dir))\n+\t\tif (real_dest_exists)\n \t\t\tjunk_git_dir_flags |= REMOVE_DIR_KEEP_TOPLEVEL;\n \t\tjunk_git_dir = real_git_dir;\n \t} else {\ndiff --git a/t/t5601-clone.sh b/t/t5601-clone.sh\nindex 84ea2a3eb7..eb9a093e25 100755\n--- a/t/t5601-clone.sh\n+++ b/t/t5601-clone.sh\n@@ -271,7 +271,9 @@ test_expect_success 'fetch from gitfile parent' '\n \n test_expect_success 'clone separate gitdir where target already exists' '\n \trm -rf dst &&\n-\ttest_must_fail git clone --separate-git-dir realgitdir src dst\n+\techo foo=bar >>realgitdir/config &&\n+\ttest_must_fail git clone --separate-git-dir realgitdir src dst &&\n+\tgrep foo=bar realgitdir/config\n '\n \n test_expect_success 'clone --reference from original' '\n-- \n2.27.0\n\n"},{"id":"401336","messageId":"20200710084732.5152-1-ben@wijen.net","threadId":"53781","inReplyTo":"xmqqv9j08dxo.fsf@gitster.c.googlers.com","subject":"[PATCH v3 0/1] git clone: don't clone into non-empty directory","fromName":"Ben Wijen","fromEmail":"ben@wijen.net","sentAt":"2020-07-10T08:47:31Z","receivedAt":"2020-07-10T08:47:59Z","isPatch":true,"sender":{"key":"ben@wijen.net","avatar":"https://avatars.githubusercontent.com/u/1721241?v=4"},"body":"Hi Junio,\n\nI added a small note to the commit message.\nPlease let me know if you require more.\n\nThank you,\n\n\nBen Wijen (1):\n git clone: don't clone into non-empty directory\n\n builtin/clone.c | 12 ++++++++++--\n t/t5601-clone.sh | 4 +++-\n 2 files changed, 13 insertions(+), 3 deletions(-)\n\n-- \n2.27.0\n\n"}]}