{"thread":{"id":"40443","subject":"[PATCH] clone --dissociate: avoid locking pack files","startedAt":"2015-09-28T19:44:57Z","lastAt":"2015-10-11T10:45:51Z","messageCount":23,"participants":["Johannes Schindelin","Max Kirillov","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"270834","messageId":"682991036f1e8e974ed8ecd7d20dbcc6fb86c344.1443469464.git.johannes.schindelin@gmx.de","threadId":"40443","inReplyTo":null,"subject":"[PATCH] clone --dissociate: avoid locking pack files","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-09-28T19:44:57Z","receivedAt":"2015-09-28T19:44:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When `git clone` is asked to dissociate the repository from the\nreference repository whose objects were used, it is quite possible that\nthe pack files need to be repacked. In that case, the pack files need to\nbe deleted that were originally hard-links to the reference repository's\npack files.\n\nOn platforms where a file cannot be deleted if another process still\nholds a handle on it, we therefore need to take pains to release all\npack files and indexes before dissociating.\n\nThis fixes https://github.com/git-for-windows/git/issues/446\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/clone.c | 9 ++++++++-\n t/t5700-clone-reference.sh | 21 +++++++++++++++++++++\n 2 files changed, 29 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 578da85..223adc4 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1064,8 +1064,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \ttransport_unlock_pack(transport);\n \ttransport_disconnect(transport);\n \n-\tif (option_dissociate)\n+\tif (option_dissociate) {\n+\t\tstruct packed_git *p;\n+\n+\t\tfor (p = packed_git; p; p = p->next) {\n+\t\t\tclose_pack_windows(p);\n+\t\t\tclose_pack_index(p);\n+\t\t}\n \t\tdissociate_from_references();\n+\t}\n \n \tjunk_mode = JUNK_LEAVE_REPO;\n \terr = checkout();\ndiff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh\nindex ef1779f..2250ef4 100755\n--- a/t/t5700-clone-reference.sh\n+++ b/t/t5700-clone-reference.sh\n@@ -188,5 +188,26 @@ test_expect_success 'clone and dissociate from reference' '\n \ttest_must_fail git -C R fsck &&\n \tgit -C S fsck\n '\n+test_expect_success 'clone, dissociate from partial reference and repack' '\n+\trm -fr P Q R &&\n+\tgit init P &&\n+\t(\n+\t\tcd P &&\n+\t\ttest_commit one &&\n+\t\tgit repack &&\n+\t\ttest_commit two &&\n+\t\tgit repack\n+\t) &&\n+\tgit clone --bare P Q &&\n+\t(\n+\t\tcd P &&\n+\t\tgit checkout -b second &&\n+\t\ttest_commit three &&\n+\t\tgit repack\n+\t) &&\n+\tgit clone --bare --dissociate --reference=P Q R &&\n+\tls R/objects/pack/*.pack >packs.txt &&\n+\ttest_line_count = 1 packs.txt\n+'\n \n test_done\n-- \n2.5.3.windows.1.3.gc322723\n"},{"id":"270946","messageId":"20150930192814.GA26154@wheezy.local","threadId":"40443","inReplyTo":"682991036f1e8e974ed8ecd7d20dbcc6fb86c344.1443469464.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] clone --dissociate: avoid locking pack files","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-09-30T19:28:14Z","receivedAt":"2015-09-30T19:28:14Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Mon, Sep 28, 2015 at 09:44:57PM +0200, Johannes Schindelin wrote:\n> When `git clone` is asked to dissociate the repository from the\n> reference repository whose objects were used, it is quite possible that\n> the pack files need to be repacked. In that case, the pack files need to\n> be deleted that were originally hard-links to the reference repository's\n> pack files.\n\nHello. For 1.9.* I used to have some hack for closing files\nalso. The case was to allow scheduled git gc to remove packs\neven if I forgot to quit some less in some console.\n\n> On platforms where a file cannot be deleted if another process still\n> holds a handle on it, we therefore need to take pains to release all\n> pack files and indexes before dissociating.\n> \n> This fixes https://github.com/git-for-windows/git/issues/446\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> builtin/clone.c | 9 ++++++++-\n> t/t5700-clone-reference.sh | 21 +++++++++++++++++++++\n> 2 files changed, 29 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 578da85..223adc4 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -1064,8 +1064,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \ttransport_unlock_pack(transport);\n> \ttransport_disconnect(transport);\n> \n> -\tif (option_dissociate)\n> +\tif (option_dissociate) {\n> +\t\tstruct packed_git *p;\n> +\n> +\t\tfor (p = packed_git; p; p = p->next) {\n> +\t\t\tclose_pack_windows(p);\n> +\t\t\tclose_pack_index(p);\n> +\t\t}\n> \t\tdissociate_from_references();\n> +\t}\n\nThis does not seem to close handles to the pack files\nthemseves, does Windows still allow removing the files? I\nprobably did not tried that, because I started from handles,\nand discovered mapped files only later.\n\n> \tjunk_mode = JUNK_LEAVE_REPO;\n> \terr = checkout();\n> diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh\n> index ef1779f..2250ef4 100755\n> --- a/t/t5700-clone-reference.sh\n> +++ b/t/t5700-clone-reference.sh\n> @@ -188,5 +188,26 @@ test_expect_success 'clone and dissociate from reference' '\n> \ttest_must_fail git -C R fsck &&\n> \tgit -C S fsck\n> '\n> +test_expect_success 'clone, dissociate from partial reference and repack' '\n> +\trm -fr P Q R &&\n> +\tgit init P &&\n> +\t(\n> +\t\tcd P &&\n> +\t\ttest_commit one &&\n> +\t\tgit repack &&\n> +\t\ttest_commit two &&\n> +\t\tgit repack\n> +\t) &&\n> +\tgit clone --bare P Q &&\n> +\t(\n> +\t\tcd P &&\n> +\t\tgit checkout -b second &&\n> +\t\ttest_commit three &&\n> +\t\tgit repack\n> +\t) &&\n> +\tgit clone --bare --dissociate --reference=P Q R &&\n> +\tls R/objects/pack/*.pack >packs.txt &&\n> +\ttest_line_count = 1 packs.txt\n> +'\n\nUnless it goes very lowlevel like running lsof of readin\nproc testing this should always pass on Linux, even if the\nissue is not fixed, maybe should be a conditional for\nWindows only?\n\n> test_done\n> -- \n> 2.5.3.windows.1.3.gc322723\n> \n> \n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"270948","messageId":"xmqqlhbn7ky1.fsf@gitster.mtv.corp.google.com","threadId":"40443","inReplyTo":"20150930192814.GA26154@wheezy.local","subject":"Re: [PATCH] clone --dissociate: avoid locking pack files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-30T19:42:46Z","receivedAt":"2015-09-30T19:42:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Max Kirillov writes:\n\n>> +\tif (option_dissociate) {\n>> +\t\tstruct packed_git *p;\n>> +\n>> +\t\tfor (p = packed_git; p; p = p->next) {\n>> +\t\t\tclose_pack_windows(p);\n>> +\t\t\tclose_pack_index(p);\n>> +\t\t}\n>> \t\tdissociate_from_references();\n>> +\t}\n>\n> This does not seem to close handles to the pack files\n> themseves, does Windows still allow removing the files? I\n> probably did not tried that, because I started from handles,\n> and discovered mapped files only later.\n\nThis is trying to (incompletely) mimic what free_pack_by_name() in\nsha1_file.c is doing but for all packs, I think. I wonder why we do\nnot have to do other things here that are done over there, like\nclearing delta-base-cache, closing pack_fd and decrementing\npack_open_fds, etc.\n\nThe right approach may to have a helper in sha1_file.c that closes\nand cleans up _all_ packs, and call it from here, instead of having\nbuiltin/clone.c even know about implementation details such as\npacked_git is a linked list, etc.\n\n>\n>> \tjunk_mode = JUNK_LEAVE_REPO;\n>> \terr = checkout();\n>> diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh\n>> index ef1779f..2250ef4 100755\n>> --- a/t/t5700-clone-reference.sh\n>> +++ b/t/t5700-clone-reference.sh\n>> @@ -188,5 +188,26 @@ test_expect_success 'clone and dissociate from reference' '\n>> \ttest_must_fail git -C R fsck &&\n>> \tgit -C S fsck\n>> '\n>> +test_expect_success 'clone, dissociate from partial reference and repack' '\n>> +\trm -fr P Q R &&\n>> +\tgit init P &&\n>> +\t(\n>> +\t\tcd P &&\n>> +\t\ttest_commit one &&\n>> +\t\tgit repack &&\n>> +\t\ttest_commit two &&\n>> +\t\tgit repack\n>> +\t) &&\n>> +\tgit clone --bare P Q &&\n>> +\t(\n>> +\t\tcd P &&\n>> +\t\tgit checkout -b second &&\n>> +\t\ttest_commit three &&\n>> +\t\tgit repack\n>> +\t) &&\n>> +\tgit clone --bare --dissociate --reference=P Q R &&\n>> +\tls R/objects/pack/*.pack >packs.txt &&\n>> +\ttest_line_count = 1 packs.txt\n>> +'\n>\n> Unless it goes very lowlevel like running lsof of readin\n> proc testing this should always pass on Linux, even if the\n> issue is not fixed, maybe should be a conditional for\n> Windows only?\n>\n>> test_done\n>> -- \n>> 2.5.3.windows.1.3.gc322723\n>> \n>> \n>> --\n>> To unsubscribe from this list: send the line \"unsubscribe git\" in\n>> the body of a message to majordomo@vger.kernel.org\n>> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"270973","messageId":"20151001043936.GB26154@wheezy.local","threadId":"40443","inReplyTo":"20150930192814.GA26154@wheezy.local","subject":"Re: [PATCH] clone --dissociate: avoid locking pack files","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-01T04:39:36Z","receivedAt":"2015-10-01T04:39:36Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Wed, Sep 30, 2015 at 10:28:14PM +0300, Max Kirillov wrote:\n> On Mon, Sep 28, 2015 at 09:44:57PM +0200, Johannes Schindelin wrote:\n>> -\tif (option_dissociate)\n>> +\tif (option_dissociate) {\n>> +\t\tstruct packed_git *p;\n>> +\n>> +\t\tfor (p = packed_git; p; p = p->next) {\n>> +\t\t\tclose_pack_windows(p);\n>> +\t\t\tclose_pack_index(p);\n>> +\t\t}\n>> \t\tdissociate_from_references();\n>> +\t}\n\n> This does not seem to close handles to the pack files\n> themseves, does Windows still allow removing the files? I\n> probably did not tried that, because I started from handles,\n> and discovered mapped files only later.\n\nApparently, pack file is closed just after mapping if it's\nsmaller than core.packedGitWindowSize. Could it be the\nreason that this patch worked in you test case?\n\n-- \nMax\n"},{"id":"271149","messageId":"4033cd50d604c7088633e6b99f0f9b6a@dscho.org","threadId":"40443","inReplyTo":"20151001043936.GB26154@wheezy.local","subject":"Re: [PATCH] clone --dissociate: avoid locking pack files","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-05T18:32:35Z","receivedAt":"2015-10-05T18:32:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Max,\n\nOn 2015-10-01 06:39, Max Kirillov wrote:\n> On Wed, Sep 30, 2015 at 10:28:14PM +0300, Max Kirillov wrote:\n>> On Mon, Sep 28, 2015 at 09:44:57PM +0200, Johannes Schindelin wrote:\n>>> -\tif (option_dissociate)\n>>> +\tif (option_dissociate) {\n>>> +\t\tstruct packed_git *p;\n>>> +\n>>> +\t\tfor (p = packed_git; p; p = p->next) {\n>>> +\t\t\tclose_pack_windows(p);\n>>> +\t\t\tclose_pack_index(p);\n>>> +\t\t}\n>>> \t\tdissociate_from_references();\n>>> +\t}\n> \n>> This does not seem to close handles to the pack files\n>> themseves, does Windows still allow removing the files? I\n>> probably did not tried that, because I started from handles,\n>> and discovered mapped files only later.\n> \n> Apparently, pack file is closed just after mapping if it's\n> smaller than core.packedGitWindowSize. Could it be the\n> reason that this patch worked in you test case?\n\nThat must be the reason, thanks!\n\nCiao,\nDscho\n"},{"id":"271164","messageId":"cover.1444076827.git.johannes.schindelin@gmx.de","threadId":"40443","inReplyTo":"682991036f1e8e974ed8ecd7d20dbcc6fb86c344.1443469464.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 0/4] Fix locking issues on Windows with `git clone --dissociate`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-05T20:29:44Z","receivedAt":"2015-10-05T20:29:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This is version 2, split into multiple commits for easier digestion.\n\nMax, I hope that this helps also your use case!\n\nJohannes Schindelin (4):\n Demonstrate a Windows file locking issue with `git clone --dissociate`\n Consolidate code to close a pack's file descriptor\n Add a function to release all packs\n clone --dissociate: avoid locking pack files\n\n builtin/clone.c | 4 +++-\n cache.h | 1 +\n sha1_file.c | 56 ++++++++++++++++++++++++++++------------------\n t/t5700-clone-reference.sh | 21 +++++++++++++++++\n 4 files changed, 59 insertions(+), 23 deletions(-)\n\n-- \n2.5.3.windows.1.3.gc322723\n"},{"id":"271165","messageId":"499e9d3bdd4e975eb6339e6469966f1e0cb2079f.1444076827.git.johannes.schindelin@gmx.de","threadId":"40443","inReplyTo":"cover.1444076827.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 1/4] Demonstrate a Windows file locking issue with `git clone --dissociate`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-05T20:29:59Z","receivedAt":"2015-10-05T20:29:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"On Windows, dissociating from a reference can fail very easily due to\npack files that are still in use when they want to be removed.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5700-clone-reference.sh | 21 +++++++++++++++++++++\n 1 file changed, 21 insertions(+)\n\ndiff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh\nindex ef1779f..f7cec85 100755\n--- a/t/t5700-clone-reference.sh\n+++ b/t/t5700-clone-reference.sh\n@@ -188,5 +188,26 @@ test_expect_success 'clone and dissociate from reference' '\n \ttest_must_fail git -C R fsck &&\n \tgit -C S fsck\n '\n+test_expect_failure MINGW 'clone, dissociate from partial reference and repack' '\n+\trm -fr P Q R &&\n+\tgit init P &&\n+\t(\n+\t\tcd P &&\n+\t\ttest_commit one &&\n+\t\tgit repack &&\n+\t\ttest_commit two &&\n+\t\tgit repack\n+\t) &&\n+\tgit clone --bare P Q &&\n+\t(\n+\t\tcd P &&\n+\t\tgit checkout -b second &&\n+\t\ttest_commit three &&\n+\t\tgit repack\n+\t) &&\n+\tgit clone --bare --dissociate --reference=P Q R &&\n+\tls R/objects/pack/*.pack >packs.txt &&\n+\ttest_line_count = 1 packs.txt\n+'\n \n test_done\n-- \n2.5.3.windows.1.3.gc322723\n"},{"id":"271166","messageId":"a1f0d9339a9de95ba3f5eaaaf2e1a712629ad5d4.1444076827.git.johannes.schindelin@gmx.de","threadId":"40443","inReplyTo":"cover.1444076827.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 2/4] Consolidate code to close a pack's file descriptor","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-05T20:30:24Z","receivedAt":"2015-10-05T20:30:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"There was a lot of repeated code to close the file descriptor of\na given pack. Let's just refactor this code into a single function.\n\nSigned-off-by: Johannes Schindelin \n---\n sha1_file.c | 38 ++++++++++++++++++--------------------\n 1 file changed, 18 insertions(+), 20 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex d295a32..8c3c913 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -786,6 +786,18 @@ void close_pack_windows(struct packed_git *p)\n \t}\n }\n \n+static int close_pack_fd(struct packed_git *p)\n+{\n+\tif (p->pack_fd < 0)\n+\t\treturn 0;\n+\n+\tclose(p->pack_fd);\n+\tpack_open_fds--;\n+\tp->pack_fd = -1;\n+\n+\treturn 1;\n+}\n+\n /*\n * The LRU pack is the one with the oldest MRU window, preferring packs\n * with no used windows, or the oldest mtime if it has no windows allocated.\n@@ -853,12 +865,8 @@ static int close_one_pack(void)\n \t\tfind_lru_pack(p, &lru_p, &mru_w, &accept_windows_inuse);\n \t}\n \n-\tif (lru_p) {\n-\t\tclose(lru_p->pack_fd);\n-\t\tpack_open_fds--;\n-\t\tlru_p->pack_fd = -1;\n-\t\treturn 1;\n-\t}\n+\tif (lru_p)\n+\t\treturn close_pack_fd(lru_p);\n \n \treturn 0;\n }\n@@ -899,10 +907,7 @@ void free_pack_by_name(const char *pack_name)\n \t\tif (strcmp(pack_name, p->pack_name) == 0) {\n \t\t\tclear_delta_base_cache();\n \t\t\tclose_pack_windows(p);\n-\t\t\tif (p->pack_fd != -1) {\n-\t\t\t\tclose(p->pack_fd);\n-\t\t\t\tpack_open_fds--;\n-\t\t\t}\n+\t\t\tclose_pack_fd(p);\n \t\t\tclose_pack_index(p);\n \t\t\tfree(p->bad_object_sha1);\n \t\t\t*pp = p->next;\n@@ -1037,11 +1042,7 @@ static int open_packed_git(struct packed_git *p)\n {\n \tif (!open_packed_git_1(p))\n \t\treturn 0;\n-\tif (p->pack_fd != -1) {\n-\t\tclose(p->pack_fd);\n-\t\tpack_open_fds--;\n-\t\tp->pack_fd = -1;\n-\t}\n+\tclose_pack_fd(p);\n \treturn -1;\n }\n \n@@ -1107,11 +1108,8 @@ unsigned char *use_pack(struct packed_git *p,\n \t\t\t\t\tp->pack_name,\n \t\t\t\t\tstrerror(errno));\n \t\t\tif (!win->offset && win->len == p->pack_size\n-\t\t\t\t&& !p->do_not_close) {\n-\t\t\t\tclose(p->pack_fd);\n-\t\t\t\tpack_open_fds--;\n-\t\t\t\tp->pack_fd = -1;\n-\t\t\t}\n+\t\t\t\t&& !p->do_not_close)\n+\t\t\t\tclose_pack_fd(p);\n \t\t\tpack_mmap_calls++;\n \t\t\tpack_open_windows++;\n \t\t\tif (pack_mapped > peak_pack_mapped)\n-- \n2.5.3.windows.1.3.gc322723\n"},{"id":"271168","messageId":"78d0edaeb0f707da24ea08f4bb8517f9ad5be775.1444076827.git.johannes.schindelin@gmx.de","threadId":"40443","inReplyTo":"cover.1444076827.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 3/4] Add a function to release all packs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-05T20:33:06Z","receivedAt":"2015-10-05T20:33:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"On Windows, files that are in use cannot be removed or renamed. That\nmeans that we have to release pack files when we are about to, say,\nrepack them. Let's introduce a convenient function to close them\npack files.\n\nWhile at it, we consolidate the close windows/close fd/close index\nstanza in `free_pack_by_name()` into the `close_pack()` function that\nis used by the new `close_all_packs()` function to avoid repeated code.\n\nSigned-off-by: Johannes Schindelin \n---\n cache.h | 1 +\n sha1_file.c | 20 +++++++++++++++++---\n 2 files changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 752031e..57f6a74 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1275,6 +1275,7 @@ extern void close_pack_index(struct packed_git *);\n \n extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned long *);\n extern void close_pack_windows(struct packed_git *);\n+extern void close_all_packs(void);\n extern void unuse_pack(struct pack_window **);\n extern void free_pack_by_name(const char *);\n extern void clear_delta_base_cache(void);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 8c3c913..fe823fe 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -798,6 +798,22 @@ static int close_pack_fd(struct packed_git *p)\n \treturn 1;\n }\n \n+static void close_pack(struct packed_git *p)\n+{\n+\tclose_pack_windows(p);\n+\tclose_pack_fd(p);\n+\tclose_pack_index(p);\n+}\n+\n+void close_all_packs(void)\n+{\n+\tstruct packed_git *p;\n+\n+\tfor (p = packed_git; p; p = p->next)\n+\t\tclose_pack(p);\n+}\n+\n+\n /*\n * The LRU pack is the one with the oldest MRU window, preferring packs\n * with no used windows, or the oldest mtime if it has no windows allocated.\n@@ -906,9 +922,7 @@ void free_pack_by_name(const char *pack_name)\n \t\tp = *pp;\n \t\tif (strcmp(pack_name, p->pack_name) == 0) {\n \t\t\tclear_delta_base_cache();\n-\t\t\tclose_pack_windows(p);\n-\t\t\tclose_pack_fd(p);\n-\t\t\tclose_pack_index(p);\n+\t\t\tclose_pack(p);\n \t\t\tfree(p->bad_object_sha1);\n \t\t\t*pp = p->next;\n \t\t\tif (last_found_pack == p)\n-- \n2.5.3.windows.1.3.gc322723\n"},{"id":"271169","messageId":"73315ceddd3ee6aa7d1c2f01bd1b64441b42ad22.1444076827.git.johannes.schindelin@gmx.de","threadId":"40443","inReplyTo":"cover.1444076827.git.johannes.schindelin@gmx.de","subject":"[PATCH v2 4/4] clone --dissociate: avoid locking pack files","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-05T20:33:34Z","receivedAt":"2015-10-05T20:33:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When `git clone` is asked to dissociate the repository from the\nreference repository whose objects were used, it is quite possible that\nthe pack files need to be repacked. In that case, the pack files need to\nbe deleted that were originally hard-links to the reference repository's\npack files.\n\nOn platforms where a file cannot be deleted if another process still\nholds a handle on it, we therefore need to take pains to release all\npack files and indexes before dissociating.\n\nThis fixes https://github.com/git-for-windows/git/issues/446\n\nThe test case to demonstrate the breakage technically does not need to\nbe run on Linux or MacOSX. It won't hurt, either, though.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/clone.c | 4 +++-\n t/t5700-clone-reference.sh | 2 +-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 578da85..cc896e2 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1064,8 +1064,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \ttransport_unlock_pack(transport);\n \ttransport_disconnect(transport);\n \n-\tif (option_dissociate)\n+\tif (option_dissociate) {\n+\t\tclose_all_packs();\n \t\tdissociate_from_references();\n+\t}\n \n \tjunk_mode = JUNK_LEAVE_REPO;\n \terr = checkout();\ndiff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh\nindex f7cec85..2250ef4 100755\n--- a/t/t5700-clone-reference.sh\n+++ b/t/t5700-clone-reference.sh\n@@ -188,7 +188,7 @@ test_expect_success 'clone and dissociate from reference' '\n \ttest_must_fail git -C R fsck &&\n \tgit -C S fsck\n '\n-test_expect_failure MINGW 'clone, dissociate from partial reference and repack' '\n+test_expect_success 'clone, dissociate from partial reference and repack' '\n \trm -fr P Q R &&\n \tgit init P &&\n \t(\n-- \n2.5.3.windows.1.3.gc322723\n"},{"id":"271177","messageId":"xmqqio6lxcci.fsf@gitster.mtv.corp.google.com","threadId":"40443","inReplyTo":"a1f0d9339a9de95ba3f5eaaaf2e1a712629ad5d4.1444076827.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 2/4] Consolidate code to close a pack's file descriptor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-05T20:57:33Z","receivedAt":"2015-10-05T20:57:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> There was a lot of repeated code to close the file descriptor of\n> a given pack. Let's just refactor this code into a single function.\n\nThat is a very good idea, but...\n\n> +static int close_pack_fd(struct packed_git *p)\n> +{\n> +\tif (p->pack_fd < 0)\n> +\t\treturn 0;\n\nIs this \"return 0\" compatible with ...\n\n> +\tclose(p->pack_fd);\n> +\tpack_open_fds--;\n> +\tp->pack_fd = -1;\n> +\n> +\treturn 1;\n> +}\n> +\n> /*\n> * The LRU pack is the one with the oldest MRU window, preferring packs\n> * with no used windows, or the oldest mtime if it has no windows allocated.\n> @@ -853,12 +865,8 @@ static int close_one_pack(void)\n> \t\tfind_lru_pack(p, &lru_p, &mru_w, &accept_windows_inuse);\n> \t}\n> \n> -\tif (lru_p) {\n> -\t\tclose(lru_p->pack_fd);\n> -\t\tpack_open_fds--;\n> -\t\tlru_p->pack_fd = -1;\n> -\t\treturn 1;\n> -\t}\n> +\tif (lru_p)\n> +\t\treturn close_pack_fd(lru_p);\n\n... what is returned from here?\n\nIt seems to me that it would be a bug if (p->pack_fd < 0) in this\ncodepath, so in practice nobody will receive a newly invented return\nvalue 0 from this function, but it feels wrong.\n\n> \treturn 0;\n> }\n> @@ -899,10 +907,7 @@ void free_pack_by_name(const char *pack_name)\n> \t\tif (strcmp(pack_name, p->pack_name) == 0) {\n> \t\t\tclear_delta_base_cache();\n> \t\t\tclose_pack_windows(p);\n> -\t\t\tif (p->pack_fd != -1) {\n> -\t\t\t\tclose(p->pack_fd);\n> -\t\t\t\tpack_open_fds--;\n> -\t\t\t}\n> +\t\t\tclose_pack_fd(p);\n\nAnd here, the closer _must_ be (and it currently is) very aware that\nthe pack chosen may not be open anymore. By giving a function that\nconditionally closes if the pack is still open too bland a name,\nthat distinction is lost at these two call sites.\n\nAlso closing \"fd\" is not the only thing the new helper does, so in\nthat sense its name is suboptimal, too.\n\nPerhaps a new helper function that is close_pack(), which is\nunconditional, with another close_pack_if_open() around it?\n\n> \t\t\tif (!win->offset && win->len == p->pack_size\n> -\t\t\t\t&& !p->do_not_close) {\n> -\t\t\t\tclose(p->pack_fd);\n> -\t\t\t\tpack_open_fds--;\n> -\t\t\t\tp->pack_fd = -1;\n> -\t\t\t}\n> +\t\t\t\t&& !p->do_not_close)\n> +\t\t\t\tclose_pack_fd(p);\n\nI wonder how this do_not_close bit should interact with \"we are\nshutting down (or we are spawning another to do the real work, while\nwe won't do anything useful anymore), so close everything down\".\n\nI'd imagine we'll see the answer in the next patch ;-)\n"},{"id":"271178","messageId":"xmqqbncdxc8c.fsf@gitster.mtv.corp.google.com","threadId":"40443","inReplyTo":"73315ceddd3ee6aa7d1c2f01bd1b64441b42ad22.1444076827.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v2 4/4] clone --dissociate: avoid locking pack files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-05T21:00:03Z","receivedAt":"2015-10-05T21:00:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index 578da85..cc896e2 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -1064,8 +1064,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n> \ttransport_unlock_pack(transport);\n> \ttransport_disconnect(transport);\n> \n> -\tif (option_dissociate)\n> +\tif (option_dissociate) {\n> +\t\tclose_all_packs();\n> \t\tdissociate_from_references();\n> +\t}\n\nMuch simpler and looks very good.\n"},{"id":"271183","messageId":"b7d0f89bb94f88cb8d600a461dfe7ea1@dscho.org","threadId":"40443","inReplyTo":"xmqqio6lxcci.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/4] Consolidate code to close a pack's file descriptor","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-05T21:52:06Z","receivedAt":"2015-10-05T21:52:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn 2015-10-05 22:57, Junio C Hamano wrote:\n> Johannes Schindelin writes:\n> \n>> There was a lot of repeated code to close the file descriptor of\n>> a given pack. Let's just refactor this code into a single function.\n> \n> That is a very good idea, but...\n> \n>> +static int close_pack_fd(struct packed_git *p)\n>> +{\n>> +\tif (p->pack_fd < 0)\n>> +\t\treturn 0;\n> \n> Is this \"return 0\" compatible with ...\n> \n>> +\tclose(p->pack_fd);\n>> +\tpack_open_fds--;\n>> +\tp->pack_fd = -1;\n>> +\n>> +\treturn 1;\n>> +}\n>> +\n>> /*\n>> * The LRU pack is the one with the oldest MRU window, preferring packs\n>> * with no used windows, or the oldest mtime if it has no windows allocated.\n>> @@ -853,12 +865,8 @@ static int close_one_pack(void)\n>> \t\tfind_lru_pack(p, &lru_p, &mru_w, &accept_windows_inuse);\n>> \t}\n>>\n>> -\tif (lru_p) {\n>> -\t\tclose(lru_p->pack_fd);\n>> -\t\tpack_open_fds--;\n>> -\t\tlru_p->pack_fd = -1;\n>> -\t\treturn 1;\n>> -\t}\n>> +\tif (lru_p)\n>> +\t\treturn close_pack_fd(lru_p);\n> \n> ... what is returned from here?\n\nYes. At this point, `lru_p` can only be non-NULL if lru_p->pack_fd is not larger than 0 (hence the call to `close(lru_p->pack_fd)` does not fail all the time, and hence the `pack_open_fds--` does not result in inconsistent state).\n\n> It seems to me that it would be a bug if (p->pack_fd < 0) in this\n> codepath, so in practice nobody will receive a newly invented return\n> value 0 from this function, but it feels wrong.\n\nYes, it would be a bug. And more subtle things would go wrong if that was the case, too.\n\n> \n>> \treturn 0;\n>> }\n>> @@ -899,10 +907,7 @@ void free_pack_by_name(const char *pack_name)\n>> \t\tif (strcmp(pack_name, p->pack_name) == 0) {\n>> \t\t\tclear_delta_base_cache();\n>> \t\t\tclose_pack_windows(p);\n>> -\t\t\tif (p->pack_fd != -1) {\n>> -\t\t\t\tclose(p->pack_fd);\n>> -\t\t\t\tpack_open_fds--;\n>> -\t\t\t}\n>> +\t\t\tclose_pack_fd(p);\n> \n> And here, the closer _must_ be (and it currently is) very aware that\n> the pack chosen may not be open anymore. By giving a function that\n> conditionally closes if the pack is still open too bland a name,\n> that distinction is lost at these two call sites.\n\nPlease note that the `close_pack_fd(p)` call does not invalidate the data. It is the caller (`free_pack_by_name()`) that does. Which is safe.\n\n> Also closing \"fd\" is not the only thing the new helper does, so in\n> that sense its name is suboptimal, too.\n\nYes, it also decrements the counter. But that is a necessary consequence of closing the file descriptor, otherwise the state would be inconsistent.\n\n> Perhaps a new helper function that is close_pack(), which is\n> unconditional, with another close_pack_if_open() around it?\n\nNext patch. And that `close_pack()` actually does do more than closing the file descriptor.\n\n>> \t\t\tif (!win->offset && win->len == p->pack_size\n>> -\t\t\t\t&& !p->do_not_close) {\n>> -\t\t\t\tclose(p->pack_fd);\n>> -\t\t\t\tpack_open_fds--;\n>> -\t\t\t\tp->pack_fd = -1;\n>> -\t\t\t}\n>> +\t\t\t\t&& !p->do_not_close)\n>> +\t\t\t\tclose_pack_fd(p);\n> \n> I wonder how this do_not_close bit should interact with \"we are\n> shutting down (or we are spawning another to do the real work, while\n> we won't do anything useful anymore), so close everything down\".\n\nThe `close_all_packs()` function is meant for the use case where you really close all the packs, no question asked.\n\nI cannot think of a use case where it would make sense to try to be gentle, yet still ask for *all* of the packs to be closed. Maybe you can think of one to convince me that it might make sense to respect the `do_not_close` flag in such a function? Because so far, I am totally unconvinced.\n\nCiao,\nJohannes\n"},{"id":"271185","messageId":"xmqq1td9x8pz.fsf@gitster.mtv.corp.google.com","threadId":"40443","inReplyTo":"b7d0f89bb94f88cb8d600a461dfe7ea1@dscho.org","subject":"Re: [PATCH v2 2/4] Consolidate code to close a pack's file descriptor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-05T22:15:52Z","receivedAt":"2015-10-05T22:15:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>>> +\t\t\t\t&& !p->do_not_close)\n>>> +\t\t\t\tclose_pack_fd(p);\n>> \n>> I wonder how this do_not_close bit should interact with \"we are\n>> shutting down (or we are spawning another to do the real work, while\n>> we won't do anything useful anymore), so close everything down\".\n>\n> The `close_all_packs()` function is meant for the use case where you\n> really close all the packs, no question asked.\n>\n> I cannot think of a use case where it would make sense to try to be\n> gentle, yet still ask for *all* of the packs to be closed. Maybe you\n> can think of one to convince me that it might make sense to respect\n> the `do_not_close` flag in such a function? Because so far, I am\n> totally unconvinced.\n\nDo not wait for being convinced forever, as I am not interested in\nconvincing you either way. Decision by made-up examples or lack of\nimagination is a waste of time ;-).\n\nMy earlier \"I wonder\" leads me to totally different conclusion,\nwhich is \"we declare that it is a bug for any caller to call\nclose-all-packs while marking any open pack with do_not_close bit\".\n\nWhich merely means that while iterating over packs in the\n\"close-all\" loop, we should throw a die(\"BUG\") at the caller if that\nbit is on. That way, we won't have to rely on \"we did not think of\na good use case so we unconditionally closed the packs without\ntelling the remainder of the system, hopefully we broke nothing.\"\n\nInstead we'd make it absolutely clear what our assumption of this\nchange and the new helper function is, and a caller that may appear\nin the future that wants to have do_not_close bit can complain. At\nthat point, maybe we can decide to either loosen it if that caller\nhas a good reason, or tell the caller to drop do_not_close bit\nbefore calling close-all.\n"},{"id":"271218","messageId":"cover.1444131898.git.johannes.schindelin@gmx.de","threadId":"40443","inReplyTo":"682991036f1e8e974ed8ecd7d20dbcc6fb86c344.1443469464.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 0/4] Fix locking issues on Windows with `git clone --dissociate`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-06T13:17:36Z","receivedAt":"2015-10-06T13:17:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This is version 3, adding that BUG! message if do_not_close was set.\n\nMax, I still hope that this patch series helps also your use case!\n\nInterdiff below the diffstat.\n\nJohannes Schindelin (4):\n Demonstrate a Windows file locking issue with `git clone --dissociate`\n Consolidate code to close a pack's file descriptor\n Add a function to release all packs\n clone --dissociate: avoid locking pack files\n\n builtin/clone.c | 4 +++-\n cache.h | 1 +\n sha1_file.c | 59 +++++++++++++++++++++++++++++-----------------\n t/t5700-clone-reference.sh | 21 +++++++++++++++++\n 4 files changed, 62 insertions(+), 23 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex fe823fe..ca699d7 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -810,7 +810,10 @@ void close_all_packs(void)\n \tstruct packed_git *p;\n \n \tfor (p = packed_git; p; p = p->next)\n-\t\tclose_pack(p);\n+\t\tif (p->do_not_close)\n+\t\t\tdie(\"BUG! Want to close pack marked 'do-not-close'\");\n+\t\telse\n+\t\t\tclose_pack(p);\n }\n \n \n-- \n2.6.1.windows.1\n"},{"id":"271219","messageId":"278799b6ec328bf9abc028a03298c6c29222adcb.1444131898.git.johannes.schindelin@gmx.de","threadId":"40443","inReplyTo":"cover.1444131898.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 1/4] Demonstrate a Windows file locking issue with `git clone --dissociate`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-06T13:18:03Z","receivedAt":"2015-10-06T13:18:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"On Windows, dissociating from a reference can fail very easily due to\npack files that are still in use when they want to be removed.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t5700-clone-reference.sh | 21 +++++++++++++++++++++\n 1 file changed, 21 insertions(+)\n\ndiff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh\nindex ef1779f..f7cec85 100755\n--- a/t/t5700-clone-reference.sh\n+++ b/t/t5700-clone-reference.sh\n@@ -188,5 +188,26 @@ test_expect_success 'clone and dissociate from reference' '\n \ttest_must_fail git -C R fsck &&\n \tgit -C S fsck\n '\n+test_expect_failure MINGW 'clone, dissociate from partial reference and repack' '\n+\trm -fr P Q R &&\n+\tgit init P &&\n+\t(\n+\t\tcd P &&\n+\t\ttest_commit one &&\n+\t\tgit repack &&\n+\t\ttest_commit two &&\n+\t\tgit repack\n+\t) &&\n+\tgit clone --bare P Q &&\n+\t(\n+\t\tcd P &&\n+\t\tgit checkout -b second &&\n+\t\ttest_commit three &&\n+\t\tgit repack\n+\t) &&\n+\tgit clone --bare --dissociate --reference=P Q R &&\n+\tls R/objects/pack/*.pack >packs.txt &&\n+\ttest_line_count = 1 packs.txt\n+'\n \n test_done\n-- \n2.6.1.windows.1\n"},{"id":"271220","messageId":"93c67c6cb4a30e68e2bfdb759a1e29661231b186.1444131898.git.johannes.schindelin@gmx.de","threadId":"40443","inReplyTo":"cover.1444131898.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 2/4] Consolidate code to close a pack's file descriptor","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-06T13:18:19Z","receivedAt":"2015-10-06T13:18:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"There was a lot of repeated code to close the file descriptor of\na given pack. Let's just refactor this code into a single function.\n\nSigned-off-by: Johannes Schindelin \n---\n sha1_file.c | 38 ++++++++++++++++++--------------------\n 1 file changed, 18 insertions(+), 20 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex d295a32..8c3c913 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -786,6 +786,18 @@ void close_pack_windows(struct packed_git *p)\n \t}\n }\n \n+static int close_pack_fd(struct packed_git *p)\n+{\n+\tif (p->pack_fd < 0)\n+\t\treturn 0;\n+\n+\tclose(p->pack_fd);\n+\tpack_open_fds--;\n+\tp->pack_fd = -1;\n+\n+\treturn 1;\n+}\n+\n /*\n * The LRU pack is the one with the oldest MRU window, preferring packs\n * with no used windows, or the oldest mtime if it has no windows allocated.\n@@ -853,12 +865,8 @@ static int close_one_pack(void)\n \t\tfind_lru_pack(p, &lru_p, &mru_w, &accept_windows_inuse);\n \t}\n \n-\tif (lru_p) {\n-\t\tclose(lru_p->pack_fd);\n-\t\tpack_open_fds--;\n-\t\tlru_p->pack_fd = -1;\n-\t\treturn 1;\n-\t}\n+\tif (lru_p)\n+\t\treturn close_pack_fd(lru_p);\n \n \treturn 0;\n }\n@@ -899,10 +907,7 @@ void free_pack_by_name(const char *pack_name)\n \t\tif (strcmp(pack_name, p->pack_name) == 0) {\n \t\t\tclear_delta_base_cache();\n \t\t\tclose_pack_windows(p);\n-\t\t\tif (p->pack_fd != -1) {\n-\t\t\t\tclose(p->pack_fd);\n-\t\t\t\tpack_open_fds--;\n-\t\t\t}\n+\t\t\tclose_pack_fd(p);\n \t\t\tclose_pack_index(p);\n \t\t\tfree(p->bad_object_sha1);\n \t\t\t*pp = p->next;\n@@ -1037,11 +1042,7 @@ static int open_packed_git(struct packed_git *p)\n {\n \tif (!open_packed_git_1(p))\n \t\treturn 0;\n-\tif (p->pack_fd != -1) {\n-\t\tclose(p->pack_fd);\n-\t\tpack_open_fds--;\n-\t\tp->pack_fd = -1;\n-\t}\n+\tclose_pack_fd(p);\n \treturn -1;\n }\n \n@@ -1107,11 +1108,8 @@ unsigned char *use_pack(struct packed_git *p,\n \t\t\t\t\tp->pack_name,\n \t\t\t\t\tstrerror(errno));\n \t\t\tif (!win->offset && win->len == p->pack_size\n-\t\t\t\t&& !p->do_not_close) {\n-\t\t\t\tclose(p->pack_fd);\n-\t\t\t\tpack_open_fds--;\n-\t\t\t\tp->pack_fd = -1;\n-\t\t\t}\n+\t\t\t\t&& !p->do_not_close)\n+\t\t\t\tclose_pack_fd(p);\n \t\t\tpack_mmap_calls++;\n \t\t\tpack_open_windows++;\n \t\t\tif (pack_mapped > peak_pack_mapped)\n-- \n2.6.1.windows.1\n"},{"id":"271221","messageId":"9953f8ed40d0cf99f1be7cfe2fa0b699b7769645.1444131898.git.johannes.schindelin@gmx.de","threadId":"40443","inReplyTo":"cover.1444131898.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 3/4] Add a function to release all packs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-06T13:18:34Z","receivedAt":"2015-10-06T13:18:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"On Windows, files that are in use cannot be removed or renamed. That\nmeans that we have to release pack files when we are about to, say,\nrepack them. Let's introduce a convenient function to close them\npack files.\n\nWhile at it, we consolidate the close windows/close fd/close index\nstanza in `free_pack_by_name()` into the `close_pack()` function that\nis used by the new `close_all_packs()` function to avoid repeated code.\n\nSigned-off-by: Johannes Schindelin \n---\n cache.h | 1 +\n sha1_file.c | 23 ++++++++++++++++++++---\n 2 files changed, 21 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 752031e..57f6a74 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1275,6 +1275,7 @@ extern void close_pack_index(struct packed_git *);\n \n extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned long *);\n extern void close_pack_windows(struct packed_git *);\n+extern void close_all_packs(void);\n extern void unuse_pack(struct pack_window **);\n extern void free_pack_by_name(const char *);\n extern void clear_delta_base_cache(void);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 8c3c913..ca699d7 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -798,6 +798,25 @@ static int close_pack_fd(struct packed_git *p)\n \treturn 1;\n }\n \n+static void close_pack(struct packed_git *p)\n+{\n+\tclose_pack_windows(p);\n+\tclose_pack_fd(p);\n+\tclose_pack_index(p);\n+}\n+\n+void close_all_packs(void)\n+{\n+\tstruct packed_git *p;\n+\n+\tfor (p = packed_git; p; p = p->next)\n+\t\tif (p->do_not_close)\n+\t\t\tdie(\"BUG! Want to close pack marked 'do-not-close'\");\n+\t\telse\n+\t\t\tclose_pack(p);\n+}\n+\n+\n /*\n * The LRU pack is the one with the oldest MRU window, preferring packs\n * with no used windows, or the oldest mtime if it has no windows allocated.\n@@ -906,9 +925,7 @@ void free_pack_by_name(const char *pack_name)\n \t\tp = *pp;\n \t\tif (strcmp(pack_name, p->pack_name) == 0) {\n \t\t\tclear_delta_base_cache();\n-\t\t\tclose_pack_windows(p);\n-\t\t\tclose_pack_fd(p);\n-\t\t\tclose_pack_index(p);\n+\t\t\tclose_pack(p);\n \t\t\tfree(p->bad_object_sha1);\n \t\t\t*pp = p->next;\n \t\t\tif (last_found_pack == p)\n-- \n2.6.1.windows.1\n"},{"id":"271222","messageId":"282ac01bc4e83aaa0cee4bd9e6de63d91ca93d30.1444131898.git.johannes.schindelin@gmx.de","threadId":"40443","inReplyTo":"cover.1444131898.git.johannes.schindelin@gmx.de","subject":"[PATCH v3 4/4] clone --dissociate: avoid locking pack files","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-06T13:18:47Z","receivedAt":"2015-10-06T13:18:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When `git clone` is asked to dissociate the repository from the\nreference repository whose objects were used, it is quite possible that\nthe pack files need to be repacked. In that case, the pack files need to\nbe deleted that were originally hard-links to the reference repository's\npack files.\n\nOn platforms where a file cannot be deleted if another process still\nholds a handle on it, we therefore need to take pains to release all\npack files and indexes before dissociating.\n\nThis fixes https://github.com/git-for-windows/git/issues/446\n\nThe test case to demonstrate the breakage technically does not need to\nbe run on Linux or MacOSX. It won't hurt, either, though.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/clone.c | 4 +++-\n t/t5700-clone-reference.sh | 2 +-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 578da85..cc896e2 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -1064,8 +1064,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \ttransport_unlock_pack(transport);\n \ttransport_disconnect(transport);\n \n-\tif (option_dissociate)\n+\tif (option_dissociate) {\n+\t\tclose_all_packs();\n \t\tdissociate_from_references();\n+\t}\n \n \tjunk_mode = JUNK_LEAVE_REPO;\n \terr = checkout();\ndiff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh\nindex f7cec85..2250ef4 100755\n--- a/t/t5700-clone-reference.sh\n+++ b/t/t5700-clone-reference.sh\n@@ -188,7 +188,7 @@ test_expect_success 'clone and dissociate from reference' '\n \ttest_must_fail git -C R fsck &&\n \tgit -C S fsck\n '\n-test_expect_failure MINGW 'clone, dissociate from partial reference and repack' '\n+test_expect_success 'clone, dissociate from partial reference and repack' '\n \trm -fr P Q R &&\n \tgit init P &&\n \t(\n-- \n2.6.1.windows.1\n"},{"id":"271224","messageId":"fb0ee65afcb4087de1137d27991dc896@dscho.org","threadId":"40443","inReplyTo":"xmqq1td9x8pz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/4] Consolidate code to close a pack's file descriptor","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-06T13:42:34Z","receivedAt":"2015-10-06T13:42:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn 2015-10-06 00:15, Junio C Hamano wrote:\n> Johannes Schindelin writes:\n> \n>>>> +\t\t\t\t&& !p->do_not_close)\n>>>> +\t\t\t\tclose_pack_fd(p);\n>>>\n>>> I wonder how this do_not_close bit should interact with \"we are\n>>> shutting down (or we are spawning another to do the real work, while\n>>> we won't do anything useful anymore), so close everything down\".\n>>\n>> The `close_all_packs()` function is meant for the use case where you\n>> really close all the packs, no question asked.\n>>\n>> I cannot think of a use case where it would make sense to try to be\n>> gentle, yet still ask for *all* of the packs to be closed. Maybe you\n>> can think of one to convince me that it might make sense to respect\n>> the `do_not_close` flag in such a function? Because so far, I am\n>> totally unconvinced.\n> \n> Do not wait for being convinced forever, as I am not interested in\n> convincing you either way. Decision by made-up examples or lack of\n> imagination is a waste of time ;-).\n> \n> My earlier \"I wonder\" leads me to totally different conclusion,\n> which is \"we declare that it is a bug for any caller to call\n> close-all-packs while marking any open pack with do_not_close bit\".\n> \n> Which merely means that while iterating over packs in the\n> \"close-all\" loop, we should throw a die(\"BUG\") at the caller if that\n> bit is on. That way, we won't have to rely on \"we did not think of\n> a good use case so we unconditionally closed the packs without\n> telling the remainder of the system, hopefully we broke nothing.\"\n\nI just sent another iteration out that adds this sanity check.\n\nCiao,\nDscho\n"},{"id":"271267","messageId":"xmqq4mi2va9u.fsf@gitster.mtv.corp.google.com","threadId":"40443","inReplyTo":"9953f8ed40d0cf99f1be7cfe2fa0b699b7769645.1444131898.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v3 3/4] Add a function to release all packs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-07T17:49:49Z","receivedAt":"2015-10-07T17:49:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Windows, files that are in use cannot be removed or renamed. That\n> means that we have to release pack files when we are about to, say,\n> repack them. Let's introduce a convenient function to close them\n> pack files.\n>\n> While at it, we consolidate the close windows/close fd/close index\n> stanza in `free_pack_by_name()` into the `close_pack()` function that\n> is used by the new `close_all_packs()` function to avoid repeated code.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n\nThis is the only one that was updated among the four and the change\nlooks sensible. I'd reword the end of the first paragraph of the\nproposed log message, though, to replace \"close them pack files\"\nwith \"close all the pack files and their idx files\".\n\nThanks. \n\n> cache.h | 1 +\n> sha1_file.c | 23 ++++++++++++++++++++---\n> 2 files changed, 21 insertions(+), 3 deletions(-)\n>\n> diff --git a/cache.h b/cache.h\n> index 752031e..57f6a74 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1275,6 +1275,7 @@ extern void close_pack_index(struct packed_git *);\n> \n> extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned long *);\n> extern void close_pack_windows(struct packed_git *);\n> +extern void close_all_packs(void);\n> extern void unuse_pack(struct pack_window **);\n> extern void free_pack_by_name(const char *);\n> extern void clear_delta_base_cache(void);\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 8c3c913..ca699d7 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -798,6 +798,25 @@ static int close_pack_fd(struct packed_git *p)\n> \treturn 1;\n> }\n> \n> +static void close_pack(struct packed_git *p)\n> +{\n> +\tclose_pack_windows(p);\n> +\tclose_pack_fd(p);\n> +\tclose_pack_index(p);\n> +}\n> +\n> +void close_all_packs(void)\n> +{\n> +\tstruct packed_git *p;\n> +\n> +\tfor (p = packed_git; p; p = p->next)\n> +\t\tif (p->do_not_close)\n> +\t\t\tdie(\"BUG! Want to close pack marked 'do-not-close'\");\n> +\t\telse\n> +\t\t\tclose_pack(p);\n> +}\n> +\n> +\n> /*\n> * The LRU pack is the one with the oldest MRU window, preferring packs\n> * with no used windows, or the oldest mtime if it has no windows allocated.\n> @@ -906,9 +925,7 @@ void free_pack_by_name(const char *pack_name)\n> \t\tp = *pp;\n> \t\tif (strcmp(pack_name, p->pack_name) == 0) {\n> \t\t\tclear_delta_base_cache();\n> -\t\t\tclose_pack_windows(p);\n> -\t\t\tclose_pack_fd(p);\n> -\t\t\tclose_pack_index(p);\n> +\t\t\tclose_pack(p);\n> \t\t\tfree(p->bad_object_sha1);\n> \t\t\t*pp = p->next;\n> \t\t\tif (last_found_pack == p)\n"},{"id":"271340","messageId":"991eaf53b382e8156ef9486014c1a4a5@dscho.org","threadId":"40443","inReplyTo":"xmqq4mi2va9u.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 3/4] Add a function to release all packs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-10-08T19:10:46Z","receivedAt":"2015-10-08T19:10:46Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn 2015-10-07 19:49, Junio C Hamano wrote:\n> Johannes Schindelin writes:\n> \n>> On Windows, files that are in use cannot be removed or renamed. That\n>> means that we have to release pack files when we are about to, say,\n>> repack them. Let's introduce a convenient function to close them\n>> pack files.\n>>\n>> While at it, we consolidate the close windows/close fd/close index\n>> stanza in `free_pack_by_name()` into the `close_pack()` function that\n>> is used by the new `close_all_packs()` function to avoid repeated code.\n>>\n>> Signed-off-by: Johannes Schindelin \n>> ---\n> \n> This is the only one that was updated among the four and the change\n> looks sensible. I'd reword the end of the first paragraph of the\n> proposed log message, though, to replace \"close them pack files\"\n> with \"close all the pack files and their idx files\".\n> \n> Thanks.\n\nThis time I was more clever and checked `pu` before sending off a fixed patch, seeing that you patched this already.\n\nThanks,\nDscho\n"},{"id":"271408","messageId":"20151011104551.GA13032@wheezy.local","threadId":"40443","inReplyTo":"cover.1444131898.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH v3 0/4] Fix locking issues on Windows with `git clone --dissociate`","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-10-11T10:45:51Z","receivedAt":"2015-10-11T10:45:51Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Tue, Oct 06, 2015 at 03:17:36PM +0200, Johannes Schindelin wrote:\n> This is version 3, adding that BUG! message if do_not_close was set.\n> \n> Max, I still hope that this patch series helps also your use case!\n\nThanks, this mostly makes gone one of my commits. I only\nneed to invoke the function after builtin is done. And maybe\nspend some time to check that dies do not happen.\n\nThe other one with cloexec I think I will still need.\n\nIt migh take some time before I return to it.\n\n-- \nMax\n"}]}