# [PATCH] clone --dissociate: avoid locking pack files 23 messages from 2015-09-28 to 2015-10-11. Participants: Johannes Schindelin, Max Kirillov, Junio C Hamano. Thread: https://gitlist.dev/t/40443 ## Johannes Schindelin, 2015-09-28 19:44 Subject: [PATCH] clone --dissociate: avoid locking pack files Message-ID: <682991036f1e8e974ed8ecd7d20dbcc6fb86c344.1443469464.git.johannes.schindelin@gmx.de> URL: https://gitlist.dev/e/682991036f1e8e974ed8ecd7d20dbcc6fb86c344.1443469464.git.johannes.schindelin%40gmx.de ``` When `git clone` is asked to dissociate the repository from the reference repository whose objects were used, it is quite possible that the pack files need to be repacked. In that case, the pack files need to be deleted that were originally hard-links to the reference repository's pack files. On platforms where a file cannot be deleted if another process still holds a handle on it, we therefore need to take pains to release all pack files and indexes before dissociating. This fixes https://github.com/git-for-windows/git/issues/446 Signed-off-by: Johannes Schindelin --- builtin/clone.c | 9 ++++++++- t/t5700-clone-reference.sh | 21 +++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/builtin/clone.c b/builtin/clone.c index 578da85..223adc4 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -1064,8 +1064,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix) transport_unlock_pack(transport); transport_disconnect(transport); - if (option_dissociate) + if (option_dissociate) { + struct packed_git *p; + + for (p = packed_git; p; p = p->next) { + close_pack_windows(p); + close_pack_index(p); + } dissociate_from_references(); + } junk_mode = JUNK_LEAVE_REPO; err = checkout(); diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh index ef1779f..2250ef4 100755 --- a/t/t5700-clone-reference.sh +++ b/t/t5700-clone-reference.sh @@ -188,5 +188,26 @@ test_expect_success 'clone and dissociate from reference' ' test_must_fail git -C R fsck && git -C S fsck ' +test_expect_success 'clone, dissociate from partial reference and repack' ' + rm -fr P Q R && + git init P && + ( + cd P && + test_commit one && + git repack && + test_commit two && + git repack + ) && + git clone --bare P Q && + ( + cd P && + git checkout -b second && + test_commit three && + git repack + ) && + git clone --bare --dissociate --reference=P Q R && + ls R/objects/pack/*.pack >packs.txt && + test_line_count = 1 packs.txt +' test_done -- 2.5.3.windows.1.3.gc322723 ``` ## Max Kirillov, 2015-09-30 19:28 Subject: Re: [PATCH] clone --dissociate: avoid locking pack files Message-ID: <20150930192814.GA26154@wheezy.local> URL: https://gitlist.dev/e/20150930192814.GA26154%40wheezy.local In-Reply-To: <682991036f1e8e974ed8ecd7d20dbcc6fb86c344.1443469464.git.johannes.schindelin@gmx.de> ``` On Mon, Sep 28, 2015 at 09:44:57PM +0200, Johannes Schindelin wrote: > When `git clone` is asked to dissociate the repository from the > reference repository whose objects were used, it is quite possible that > the pack files need to be repacked. In that case, the pack files need to > be deleted that were originally hard-links to the reference repository's > pack files. Hello. For 1.9.* I used to have some hack for closing files also. The case was to allow scheduled git gc to remove packs even if I forgot to quit some less in some console. > On platforms where a file cannot be deleted if another process still > holds a handle on it, we therefore need to take pains to release all > pack files and indexes before dissociating. > > This fixes https://github.com/git-for-windows/git/issues/446 > > Signed-off-by: Johannes Schindelin > --- > builtin/clone.c | 9 ++++++++- > t/t5700-clone-reference.sh | 21 +++++++++++++++++++++ > 2 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/builtin/clone.c b/builtin/clone.c > index 578da85..223adc4 100644 > --- a/builtin/clone.c > +++ b/builtin/clone.c > @@ -1064,8 +1064,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix) > transport_unlock_pack(transport); > transport_disconnect(transport); > > - if (option_dissociate) > + if (option_dissociate) { > + struct packed_git *p; > + > + for (p = packed_git; p; p = p->next) { > + close_pack_windows(p); > + close_pack_index(p); > + } > dissociate_from_references(); > + } This does not seem to close handles to the pack files themseves, does Windows still allow removing the files? I probably did not tried that, because I started from handles, and discovered mapped files only later. > junk_mode = JUNK_LEAVE_REPO; > err = checkout(); > diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh > index ef1779f..2250ef4 100755 > --- a/t/t5700-clone-reference.sh > +++ b/t/t5700-clone-reference.sh > @@ -188,5 +188,26 @@ test_expect_success 'clone and dissociate from reference' ' > test_must_fail git -C R fsck && > git -C S fsck > ' > +test_expect_success 'clone, dissociate from partial reference and repack' ' > + rm -fr P Q R && > + git init P && > + ( > + cd P && > + test_commit one && > + git repack && > + test_commit two && > + git repack > + ) && > + git clone --bare P Q && > + ( > + cd P && > + git checkout -b second && > + test_commit three && > + git repack > + ) && > + git clone --bare --dissociate --reference=P Q R && > + ls R/objects/pack/*.pack >packs.txt && > + test_line_count = 1 packs.txt > +' Unless it goes very lowlevel like running lsof of readin proc testing this should always pass on Linux, even if the issue is not fixed, maybe should be a conditional for Windows only? > test_done > -- > 2.5.3.windows.1.3.gc322723 > > > -- > To unsubscribe from this list: send the line "unsubscribe git" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ``` ## Junio C Hamano, 2015-09-30 19:42 Subject: Re: [PATCH] clone --dissociate: avoid locking pack files Message-ID: URL: https://gitlist.dev/e/xmqqlhbn7ky1.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20150930192814.GA26154@wheezy.local> ``` Max Kirillov writes: >> + if (option_dissociate) { >> + struct packed_git *p; >> + >> + for (p = packed_git; p; p = p->next) { >> + close_pack_windows(p); >> + close_pack_index(p); >> + } >> dissociate_from_references(); >> + } > > This does not seem to close handles to the pack files > themseves, does Windows still allow removing the files? I > probably did not tried that, because I started from handles, > and discovered mapped files only later. This is trying to (incompletely) mimic what free_pack_by_name() in sha1_file.c is doing but for all packs, I think. I wonder why we do not have to do other things here that are done over there, like clearing delta-base-cache, closing pack_fd and decrementing pack_open_fds, etc. The right approach may to have a helper in sha1_file.c that closes and cleans up _all_ packs, and call it from here, instead of having builtin/clone.c even know about implementation details such as packed_git is a linked list, etc. > >> junk_mode = JUNK_LEAVE_REPO; >> err = checkout(); >> diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh >> index ef1779f..2250ef4 100755 >> --- a/t/t5700-clone-reference.sh >> +++ b/t/t5700-clone-reference.sh >> @@ -188,5 +188,26 @@ test_expect_success 'clone and dissociate from reference' ' >> test_must_fail git -C R fsck && >> git -C S fsck >> ' >> +test_expect_success 'clone, dissociate from partial reference and repack' ' >> + rm -fr P Q R && >> + git init P && >> + ( >> + cd P && >> + test_commit one && >> + git repack && >> + test_commit two && >> + git repack >> + ) && >> + git clone --bare P Q && >> + ( >> + cd P && >> + git checkout -b second && >> + test_commit three && >> + git repack >> + ) && >> + git clone --bare --dissociate --reference=P Q R && >> + ls R/objects/pack/*.pack >packs.txt && >> + test_line_count = 1 packs.txt >> +' > > Unless it goes very lowlevel like running lsof of readin > proc testing this should always pass on Linux, even if the > issue is not fixed, maybe should be a conditional for > Windows only? > >> test_done >> -- >> 2.5.3.windows.1.3.gc322723 >> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe git" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html ``` ## Max Kirillov, 2015-10-01 04:39 Subject: Re: [PATCH] clone --dissociate: avoid locking pack files Message-ID: <20151001043936.GB26154@wheezy.local> URL: https://gitlist.dev/e/20151001043936.GB26154%40wheezy.local In-Reply-To: <20150930192814.GA26154@wheezy.local> ``` On Wed, Sep 30, 2015 at 10:28:14PM +0300, Max Kirillov wrote: > On Mon, Sep 28, 2015 at 09:44:57PM +0200, Johannes Schindelin wrote: >> - if (option_dissociate) >> + if (option_dissociate) { >> + struct packed_git *p; >> + >> + for (p = packed_git; p; p = p->next) { >> + close_pack_windows(p); >> + close_pack_index(p); >> + } >> dissociate_from_references(); >> + } > This does not seem to close handles to the pack files > themseves, does Windows still allow removing the files? I > probably did not tried that, because I started from handles, > and discovered mapped files only later. Apparently, pack file is closed just after mapping if it's smaller than core.packedGitWindowSize. Could it be the reason that this patch worked in you test case? -- Max ``` ## Johannes Schindelin, 2015-10-05 18:32 Subject: Re: [PATCH] clone --dissociate: avoid locking pack files Message-ID: <4033cd50d604c7088633e6b99f0f9b6a@dscho.org> URL: https://gitlist.dev/e/4033cd50d604c7088633e6b99f0f9b6a%40dscho.org In-Reply-To: <20151001043936.GB26154@wheezy.local> ``` Hi Max, On 2015-10-01 06:39, Max Kirillov wrote: > On Wed, Sep 30, 2015 at 10:28:14PM +0300, Max Kirillov wrote: >> On Mon, Sep 28, 2015 at 09:44:57PM +0200, Johannes Schindelin wrote: >>> - if (option_dissociate) >>> + if (option_dissociate) { >>> + struct packed_git *p; >>> + >>> + for (p = packed_git; p; p = p->next) { >>> + close_pack_windows(p); >>> + close_pack_index(p); >>> + } >>> dissociate_from_references(); >>> + } > >> This does not seem to close handles to the pack files >> themseves, does Windows still allow removing the files? I >> probably did not tried that, because I started from handles, >> and discovered mapped files only later. > > Apparently, pack file is closed just after mapping if it's > smaller than core.packedGitWindowSize. Could it be the > reason that this patch worked in you test case? That must be the reason, thanks! Ciao, Dscho ``` ## Johannes Schindelin, 2015-10-05 20:29 Subject: [PATCH v2 0/4] Fix locking issues on Windows with `git clone --dissociate` Message-ID: URL: https://gitlist.dev/e/cover.1444076827.git.johannes.schindelin%40gmx.de In-Reply-To: <682991036f1e8e974ed8ecd7d20dbcc6fb86c344.1443469464.git.johannes.schindelin@gmx.de> ``` This is version 2, split into multiple commits for easier digestion. Max, I hope that this helps also your use case! Johannes Schindelin (4): Demonstrate a Windows file locking issue with `git clone --dissociate` Consolidate code to close a pack's file descriptor Add a function to release all packs clone --dissociate: avoid locking pack files builtin/clone.c | 4 +++- cache.h | 1 + sha1_file.c | 56 ++++++++++++++++++++++++++++------------------ t/t5700-clone-reference.sh | 21 +++++++++++++++++ 4 files changed, 59 insertions(+), 23 deletions(-) -- 2.5.3.windows.1.3.gc322723 ``` ## Johannes Schindelin, 2015-10-05 20:29 Subject: [PATCH v2 1/4] Demonstrate a Windows file locking issue with `git clone --dissociate` Message-ID: <499e9d3bdd4e975eb6339e6469966f1e0cb2079f.1444076827.git.johannes.schindelin@gmx.de> URL: https://gitlist.dev/e/499e9d3bdd4e975eb6339e6469966f1e0cb2079f.1444076827.git.johannes.schindelin%40gmx.de In-Reply-To: ``` On Windows, dissociating from a reference can fail very easily due to pack files that are still in use when they want to be removed. Signed-off-by: Johannes Schindelin --- t/t5700-clone-reference.sh | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh index ef1779f..f7cec85 100755 --- a/t/t5700-clone-reference.sh +++ b/t/t5700-clone-reference.sh @@ -188,5 +188,26 @@ test_expect_success 'clone and dissociate from reference' ' test_must_fail git -C R fsck && git -C S fsck ' +test_expect_failure MINGW 'clone, dissociate from partial reference and repack' ' + rm -fr P Q R && + git init P && + ( + cd P && + test_commit one && + git repack && + test_commit two && + git repack + ) && + git clone --bare P Q && + ( + cd P && + git checkout -b second && + test_commit three && + git repack + ) && + git clone --bare --dissociate --reference=P Q R && + ls R/objects/pack/*.pack >packs.txt && + test_line_count = 1 packs.txt +' test_done -- 2.5.3.windows.1.3.gc322723 ``` ## Johannes Schindelin, 2015-10-05 20:30 Subject: [PATCH v2 2/4] Consolidate code to close a pack's file descriptor Message-ID: URL: https://gitlist.dev/e/a1f0d9339a9de95ba3f5eaaaf2e1a712629ad5d4.1444076827.git.johannes.schindelin%40gmx.de In-Reply-To: ``` There was a lot of repeated code to close the file descriptor of a given pack. Let's just refactor this code into a single function. Signed-off-by: Johannes Schindelin --- sha1_file.c | 38 ++++++++++++++++++-------------------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/sha1_file.c b/sha1_file.c index d295a32..8c3c913 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -786,6 +786,18 @@ void close_pack_windows(struct packed_git *p) } } +static int close_pack_fd(struct packed_git *p) +{ + if (p->pack_fd < 0) + return 0; + + close(p->pack_fd); + pack_open_fds--; + p->pack_fd = -1; + + return 1; +} + /* * The LRU pack is the one with the oldest MRU window, preferring packs * with no used windows, or the oldest mtime if it has no windows allocated. @@ -853,12 +865,8 @@ static int close_one_pack(void) find_lru_pack(p, &lru_p, &mru_w, &accept_windows_inuse); } - if (lru_p) { - close(lru_p->pack_fd); - pack_open_fds--; - lru_p->pack_fd = -1; - return 1; - } + if (lru_p) + return close_pack_fd(lru_p); return 0; } @@ -899,10 +907,7 @@ void free_pack_by_name(const char *pack_name) if (strcmp(pack_name, p->pack_name) == 0) { clear_delta_base_cache(); close_pack_windows(p); - if (p->pack_fd != -1) { - close(p->pack_fd); - pack_open_fds--; - } + close_pack_fd(p); close_pack_index(p); free(p->bad_object_sha1); *pp = p->next; @@ -1037,11 +1042,7 @@ static int open_packed_git(struct packed_git *p) { if (!open_packed_git_1(p)) return 0; - if (p->pack_fd != -1) { - close(p->pack_fd); - pack_open_fds--; - p->pack_fd = -1; - } + close_pack_fd(p); return -1; } @@ -1107,11 +1108,8 @@ unsigned char *use_pack(struct packed_git *p, p->pack_name, strerror(errno)); if (!win->offset && win->len == p->pack_size - && !p->do_not_close) { - close(p->pack_fd); - pack_open_fds--; - p->pack_fd = -1; - } + && !p->do_not_close) + close_pack_fd(p); pack_mmap_calls++; pack_open_windows++; if (pack_mapped > peak_pack_mapped) -- 2.5.3.windows.1.3.gc322723 ``` ## Johannes Schindelin, 2015-10-05 20:33 Subject: [PATCH v2 3/4] Add a function to release all packs Message-ID: <78d0edaeb0f707da24ea08f4bb8517f9ad5be775.1444076827.git.johannes.schindelin@gmx.de> URL: https://gitlist.dev/e/78d0edaeb0f707da24ea08f4bb8517f9ad5be775.1444076827.git.johannes.schindelin%40gmx.de In-Reply-To: ``` On Windows, files that are in use cannot be removed or renamed. That means that we have to release pack files when we are about to, say, repack them. Let's introduce a convenient function to close them pack files. While at it, we consolidate the close windows/close fd/close index stanza in `free_pack_by_name()` into the `close_pack()` function that is used by the new `close_all_packs()` function to avoid repeated code. Signed-off-by: Johannes Schindelin --- cache.h | 1 + sha1_file.c | 20 +++++++++++++++++--- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/cache.h b/cache.h index 752031e..57f6a74 100644 --- a/cache.h +++ b/cache.h @@ -1275,6 +1275,7 @@ extern void close_pack_index(struct packed_git *); extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned long *); extern void close_pack_windows(struct packed_git *); +extern void close_all_packs(void); extern void unuse_pack(struct pack_window **); extern void free_pack_by_name(const char *); extern void clear_delta_base_cache(void); diff --git a/sha1_file.c b/sha1_file.c index 8c3c913..fe823fe 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -798,6 +798,22 @@ static int close_pack_fd(struct packed_git *p) return 1; } +static void close_pack(struct packed_git *p) +{ + close_pack_windows(p); + close_pack_fd(p); + close_pack_index(p); +} + +void close_all_packs(void) +{ + struct packed_git *p; + + for (p = packed_git; p; p = p->next) + close_pack(p); +} + + /* * The LRU pack is the one with the oldest MRU window, preferring packs * with no used windows, or the oldest mtime if it has no windows allocated. @@ -906,9 +922,7 @@ void free_pack_by_name(const char *pack_name) p = *pp; if (strcmp(pack_name, p->pack_name) == 0) { clear_delta_base_cache(); - close_pack_windows(p); - close_pack_fd(p); - close_pack_index(p); + close_pack(p); free(p->bad_object_sha1); *pp = p->next; if (last_found_pack == p) -- 2.5.3.windows.1.3.gc322723 ``` ## Johannes Schindelin, 2015-10-05 20:33 Subject: [PATCH v2 4/4] clone --dissociate: avoid locking pack files Message-ID: <73315ceddd3ee6aa7d1c2f01bd1b64441b42ad22.1444076827.git.johannes.schindelin@gmx.de> URL: https://gitlist.dev/e/73315ceddd3ee6aa7d1c2f01bd1b64441b42ad22.1444076827.git.johannes.schindelin%40gmx.de In-Reply-To: ``` When `git clone` is asked to dissociate the repository from the reference repository whose objects were used, it is quite possible that the pack files need to be repacked. In that case, the pack files need to be deleted that were originally hard-links to the reference repository's pack files. On platforms where a file cannot be deleted if another process still holds a handle on it, we therefore need to take pains to release all pack files and indexes before dissociating. This fixes https://github.com/git-for-windows/git/issues/446 The test case to demonstrate the breakage technically does not need to be run on Linux or MacOSX. It won't hurt, either, though. Signed-off-by: Johannes Schindelin --- builtin/clone.c | 4 +++- t/t5700-clone-reference.sh | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index 578da85..cc896e2 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -1064,8 +1064,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix) transport_unlock_pack(transport); transport_disconnect(transport); - if (option_dissociate) + if (option_dissociate) { + close_all_packs(); dissociate_from_references(); + } junk_mode = JUNK_LEAVE_REPO; err = checkout(); diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh index f7cec85..2250ef4 100755 --- a/t/t5700-clone-reference.sh +++ b/t/t5700-clone-reference.sh @@ -188,7 +188,7 @@ test_expect_success 'clone and dissociate from reference' ' test_must_fail git -C R fsck && git -C S fsck ' -test_expect_failure MINGW 'clone, dissociate from partial reference and repack' ' +test_expect_success 'clone, dissociate from partial reference and repack' ' rm -fr P Q R && git init P && ( -- 2.5.3.windows.1.3.gc322723 ``` ## Junio C Hamano, 2015-10-05 20:57 Subject: Re: [PATCH v2 2/4] Consolidate code to close a pack's file descriptor Message-ID: URL: https://gitlist.dev/e/xmqqio6lxcci.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Johannes Schindelin writes: > There was a lot of repeated code to close the file descriptor of > a given pack. Let's just refactor this code into a single function. That is a very good idea, but... > +static int close_pack_fd(struct packed_git *p) > +{ > + if (p->pack_fd < 0) > + return 0; Is this "return 0" compatible with ... > + close(p->pack_fd); > + pack_open_fds--; > + p->pack_fd = -1; > + > + return 1; > +} > + > /* > * The LRU pack is the one with the oldest MRU window, preferring packs > * with no used windows, or the oldest mtime if it has no windows allocated. > @@ -853,12 +865,8 @@ static int close_one_pack(void) > find_lru_pack(p, &lru_p, &mru_w, &accept_windows_inuse); > } > > - if (lru_p) { > - close(lru_p->pack_fd); > - pack_open_fds--; > - lru_p->pack_fd = -1; > - return 1; > - } > + if (lru_p) > + return close_pack_fd(lru_p); ... what is returned from here? It seems to me that it would be a bug if (p->pack_fd < 0) in this codepath, so in practice nobody will receive a newly invented return value 0 from this function, but it feels wrong. > return 0; > } > @@ -899,10 +907,7 @@ void free_pack_by_name(const char *pack_name) > if (strcmp(pack_name, p->pack_name) == 0) { > clear_delta_base_cache(); > close_pack_windows(p); > - if (p->pack_fd != -1) { > - close(p->pack_fd); > - pack_open_fds--; > - } > + close_pack_fd(p); And here, the closer _must_ be (and it currently is) very aware that the pack chosen may not be open anymore. By giving a function that conditionally closes if the pack is still open too bland a name, that distinction is lost at these two call sites. Also closing "fd" is not the only thing the new helper does, so in that sense its name is suboptimal, too. Perhaps a new helper function that is close_pack(), which is unconditional, with another close_pack_if_open() around it? > if (!win->offset && win->len == p->pack_size > - && !p->do_not_close) { > - close(p->pack_fd); > - pack_open_fds--; > - p->pack_fd = -1; > - } > + && !p->do_not_close) > + close_pack_fd(p); I wonder how this do_not_close bit should interact with "we are shutting down (or we are spawning another to do the real work, while we won't do anything useful anymore), so close everything down". I'd imagine we'll see the answer in the next patch ;-) ``` ## Junio C Hamano, 2015-10-05 21:00 Subject: Re: [PATCH v2 4/4] clone --dissociate: avoid locking pack files Message-ID: URL: https://gitlist.dev/e/xmqqbncdxc8c.fsf%40gitster.mtv.corp.google.com In-Reply-To: <73315ceddd3ee6aa7d1c2f01bd1b64441b42ad22.1444076827.git.johannes.schindelin@gmx.de> ``` Johannes Schindelin writes: > diff --git a/builtin/clone.c b/builtin/clone.c > index 578da85..cc896e2 100644 > --- a/builtin/clone.c > +++ b/builtin/clone.c > @@ -1064,8 +1064,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix) > transport_unlock_pack(transport); > transport_disconnect(transport); > > - if (option_dissociate) > + if (option_dissociate) { > + close_all_packs(); > dissociate_from_references(); > + } Much simpler and looks very good. ``` ## Johannes Schindelin, 2015-10-05 21:52 Subject: Re: [PATCH v2 2/4] Consolidate code to close a pack's file descriptor Message-ID: URL: https://gitlist.dev/e/b7d0f89bb94f88cb8d600a461dfe7ea1%40dscho.org In-Reply-To: ``` Hi Junio, On 2015-10-05 22:57, Junio C Hamano wrote: > Johannes Schindelin writes: > >> There was a lot of repeated code to close the file descriptor of >> a given pack. Let's just refactor this code into a single function. > > That is a very good idea, but... > >> +static int close_pack_fd(struct packed_git *p) >> +{ >> + if (p->pack_fd < 0) >> + return 0; > > Is this "return 0" compatible with ... > >> + close(p->pack_fd); >> + pack_open_fds--; >> + p->pack_fd = -1; >> + >> + return 1; >> +} >> + >> /* >> * The LRU pack is the one with the oldest MRU window, preferring packs >> * with no used windows, or the oldest mtime if it has no windows allocated. >> @@ -853,12 +865,8 @@ static int close_one_pack(void) >> find_lru_pack(p, &lru_p, &mru_w, &accept_windows_inuse); >> } >> >> - if (lru_p) { >> - close(lru_p->pack_fd); >> - pack_open_fds--; >> - lru_p->pack_fd = -1; >> - return 1; >> - } >> + if (lru_p) >> + return close_pack_fd(lru_p); > > ... what is returned from here? Yes. 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). > It seems to me that it would be a bug if (p->pack_fd < 0) in this > codepath, so in practice nobody will receive a newly invented return > value 0 from this function, but it feels wrong. Yes, it would be a bug. And more subtle things would go wrong if that was the case, too. > >> return 0; >> } >> @@ -899,10 +907,7 @@ void free_pack_by_name(const char *pack_name) >> if (strcmp(pack_name, p->pack_name) == 0) { >> clear_delta_base_cache(); >> close_pack_windows(p); >> - if (p->pack_fd != -1) { >> - close(p->pack_fd); >> - pack_open_fds--; >> - } >> + close_pack_fd(p); > > And here, the closer _must_ be (and it currently is) very aware that > the pack chosen may not be open anymore. By giving a function that > conditionally closes if the pack is still open too bland a name, > that distinction is lost at these two call sites. Please 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. > Also closing "fd" is not the only thing the new helper does, so in > that sense its name is suboptimal, too. Yes, it also decrements the counter. But that is a necessary consequence of closing the file descriptor, otherwise the state would be inconsistent. > Perhaps a new helper function that is close_pack(), which is > unconditional, with another close_pack_if_open() around it? Next patch. And that `close_pack()` actually does do more than closing the file descriptor. >> if (!win->offset && win->len == p->pack_size >> - && !p->do_not_close) { >> - close(p->pack_fd); >> - pack_open_fds--; >> - p->pack_fd = -1; >> - } >> + && !p->do_not_close) >> + close_pack_fd(p); > > I wonder how this do_not_close bit should interact with "we are > shutting down (or we are spawning another to do the real work, while > we won't do anything useful anymore), so close everything down". The `close_all_packs()` function is meant for the use case where you really close all the packs, no question asked. I 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. Ciao, Johannes ``` ## Junio C Hamano, 2015-10-05 22:15 Subject: Re: [PATCH v2 2/4] Consolidate code to close a pack's file descriptor Message-ID: URL: https://gitlist.dev/e/xmqq1td9x8pz.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Johannes Schindelin writes: >>> + && !p->do_not_close) >>> + close_pack_fd(p); >> >> I wonder how this do_not_close bit should interact with "we are >> shutting down (or we are spawning another to do the real work, while >> we won't do anything useful anymore), so close everything down". > > The `close_all_packs()` function is meant for the use case where you > really close all the packs, no question asked. > > I 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. Do not wait for being convinced forever, as I am not interested in convincing you either way. Decision by made-up examples or lack of imagination is a waste of time ;-). My earlier "I wonder" leads me to totally different conclusion, which is "we declare that it is a bug for any caller to call close-all-packs while marking any open pack with do_not_close bit". Which merely means that while iterating over packs in the "close-all" loop, we should throw a die("BUG") at the caller if that bit is on. That way, we won't have to rely on "we did not think of a good use case so we unconditionally closed the packs without telling the remainder of the system, hopefully we broke nothing." Instead we'd make it absolutely clear what our assumption of this change and the new helper function is, and a caller that may appear in the future that wants to have do_not_close bit can complain. At that point, maybe we can decide to either loosen it if that caller has a good reason, or tell the caller to drop do_not_close bit before calling close-all. ``` ## Johannes Schindelin, 2015-10-06 13:17 Subject: [PATCH v3 0/4] Fix locking issues on Windows with `git clone --dissociate` Message-ID: URL: https://gitlist.dev/e/cover.1444131898.git.johannes.schindelin%40gmx.de In-Reply-To: <682991036f1e8e974ed8ecd7d20dbcc6fb86c344.1443469464.git.johannes.schindelin@gmx.de> ``` This is version 3, adding that BUG! message if do_not_close was set. Max, I still hope that this patch series helps also your use case! Interdiff below the diffstat. Johannes Schindelin (4): Demonstrate a Windows file locking issue with `git clone --dissociate` Consolidate code to close a pack's file descriptor Add a function to release all packs clone --dissociate: avoid locking pack files builtin/clone.c | 4 +++- cache.h | 1 + sha1_file.c | 59 +++++++++++++++++++++++++++++----------------- t/t5700-clone-reference.sh | 21 +++++++++++++++++ 4 files changed, 62 insertions(+), 23 deletions(-) diff --git a/sha1_file.c b/sha1_file.c index fe823fe..ca699d7 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -810,7 +810,10 @@ void close_all_packs(void) struct packed_git *p; for (p = packed_git; p; p = p->next) - close_pack(p); + if (p->do_not_close) + die("BUG! Want to close pack marked 'do-not-close'"); + else + close_pack(p); } -- 2.6.1.windows.1 ``` ## Johannes Schindelin, 2015-10-06 13:18 Subject: [PATCH v3 1/4] Demonstrate a Windows file locking issue with `git clone --dissociate` Message-ID: <278799b6ec328bf9abc028a03298c6c29222adcb.1444131898.git.johannes.schindelin@gmx.de> URL: https://gitlist.dev/e/278799b6ec328bf9abc028a03298c6c29222adcb.1444131898.git.johannes.schindelin%40gmx.de In-Reply-To: ``` On Windows, dissociating from a reference can fail very easily due to pack files that are still in use when they want to be removed. Signed-off-by: Johannes Schindelin --- t/t5700-clone-reference.sh | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh index ef1779f..f7cec85 100755 --- a/t/t5700-clone-reference.sh +++ b/t/t5700-clone-reference.sh @@ -188,5 +188,26 @@ test_expect_success 'clone and dissociate from reference' ' test_must_fail git -C R fsck && git -C S fsck ' +test_expect_failure MINGW 'clone, dissociate from partial reference and repack' ' + rm -fr P Q R && + git init P && + ( + cd P && + test_commit one && + git repack && + test_commit two && + git repack + ) && + git clone --bare P Q && + ( + cd P && + git checkout -b second && + test_commit three && + git repack + ) && + git clone --bare --dissociate --reference=P Q R && + ls R/objects/pack/*.pack >packs.txt && + test_line_count = 1 packs.txt +' test_done -- 2.6.1.windows.1 ``` ## Johannes Schindelin, 2015-10-06 13:18 Subject: [PATCH v3 2/4] Consolidate code to close a pack's file descriptor Message-ID: <93c67c6cb4a30e68e2bfdb759a1e29661231b186.1444131898.git.johannes.schindelin@gmx.de> URL: https://gitlist.dev/e/93c67c6cb4a30e68e2bfdb759a1e29661231b186.1444131898.git.johannes.schindelin%40gmx.de In-Reply-To: ``` There was a lot of repeated code to close the file descriptor of a given pack. Let's just refactor this code into a single function. Signed-off-by: Johannes Schindelin --- sha1_file.c | 38 ++++++++++++++++++-------------------- 1 file changed, 18 insertions(+), 20 deletions(-) diff --git a/sha1_file.c b/sha1_file.c index d295a32..8c3c913 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -786,6 +786,18 @@ void close_pack_windows(struct packed_git *p) } } +static int close_pack_fd(struct packed_git *p) +{ + if (p->pack_fd < 0) + return 0; + + close(p->pack_fd); + pack_open_fds--; + p->pack_fd = -1; + + return 1; +} + /* * The LRU pack is the one with the oldest MRU window, preferring packs * with no used windows, or the oldest mtime if it has no windows allocated. @@ -853,12 +865,8 @@ static int close_one_pack(void) find_lru_pack(p, &lru_p, &mru_w, &accept_windows_inuse); } - if (lru_p) { - close(lru_p->pack_fd); - pack_open_fds--; - lru_p->pack_fd = -1; - return 1; - } + if (lru_p) + return close_pack_fd(lru_p); return 0; } @@ -899,10 +907,7 @@ void free_pack_by_name(const char *pack_name) if (strcmp(pack_name, p->pack_name) == 0) { clear_delta_base_cache(); close_pack_windows(p); - if (p->pack_fd != -1) { - close(p->pack_fd); - pack_open_fds--; - } + close_pack_fd(p); close_pack_index(p); free(p->bad_object_sha1); *pp = p->next; @@ -1037,11 +1042,7 @@ static int open_packed_git(struct packed_git *p) { if (!open_packed_git_1(p)) return 0; - if (p->pack_fd != -1) { - close(p->pack_fd); - pack_open_fds--; - p->pack_fd = -1; - } + close_pack_fd(p); return -1; } @@ -1107,11 +1108,8 @@ unsigned char *use_pack(struct packed_git *p, p->pack_name, strerror(errno)); if (!win->offset && win->len == p->pack_size - && !p->do_not_close) { - close(p->pack_fd); - pack_open_fds--; - p->pack_fd = -1; - } + && !p->do_not_close) + close_pack_fd(p); pack_mmap_calls++; pack_open_windows++; if (pack_mapped > peak_pack_mapped) -- 2.6.1.windows.1 ``` ## Johannes Schindelin, 2015-10-06 13:18 Subject: [PATCH v3 3/4] Add a function to release all packs Message-ID: <9953f8ed40d0cf99f1be7cfe2fa0b699b7769645.1444131898.git.johannes.schindelin@gmx.de> URL: https://gitlist.dev/e/9953f8ed40d0cf99f1be7cfe2fa0b699b7769645.1444131898.git.johannes.schindelin%40gmx.de In-Reply-To: ``` On Windows, files that are in use cannot be removed or renamed. That means that we have to release pack files when we are about to, say, repack them. Let's introduce a convenient function to close them pack files. While at it, we consolidate the close windows/close fd/close index stanza in `free_pack_by_name()` into the `close_pack()` function that is used by the new `close_all_packs()` function to avoid repeated code. Signed-off-by: Johannes Schindelin --- cache.h | 1 + sha1_file.c | 23 ++++++++++++++++++++--- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/cache.h b/cache.h index 752031e..57f6a74 100644 --- a/cache.h +++ b/cache.h @@ -1275,6 +1275,7 @@ extern void close_pack_index(struct packed_git *); extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned long *); extern void close_pack_windows(struct packed_git *); +extern void close_all_packs(void); extern void unuse_pack(struct pack_window **); extern void free_pack_by_name(const char *); extern void clear_delta_base_cache(void); diff --git a/sha1_file.c b/sha1_file.c index 8c3c913..ca699d7 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -798,6 +798,25 @@ static int close_pack_fd(struct packed_git *p) return 1; } +static void close_pack(struct packed_git *p) +{ + close_pack_windows(p); + close_pack_fd(p); + close_pack_index(p); +} + +void close_all_packs(void) +{ + struct packed_git *p; + + for (p = packed_git; p; p = p->next) + if (p->do_not_close) + die("BUG! Want to close pack marked 'do-not-close'"); + else + close_pack(p); +} + + /* * The LRU pack is the one with the oldest MRU window, preferring packs * with no used windows, or the oldest mtime if it has no windows allocated. @@ -906,9 +925,7 @@ void free_pack_by_name(const char *pack_name) p = *pp; if (strcmp(pack_name, p->pack_name) == 0) { clear_delta_base_cache(); - close_pack_windows(p); - close_pack_fd(p); - close_pack_index(p); + close_pack(p); free(p->bad_object_sha1); *pp = p->next; if (last_found_pack == p) -- 2.6.1.windows.1 ``` ## Johannes Schindelin, 2015-10-06 13:18 Subject: [PATCH v3 4/4] clone --dissociate: avoid locking pack files Message-ID: <282ac01bc4e83aaa0cee4bd9e6de63d91ca93d30.1444131898.git.johannes.schindelin@gmx.de> URL: https://gitlist.dev/e/282ac01bc4e83aaa0cee4bd9e6de63d91ca93d30.1444131898.git.johannes.schindelin%40gmx.de In-Reply-To: ``` When `git clone` is asked to dissociate the repository from the reference repository whose objects were used, it is quite possible that the pack files need to be repacked. In that case, the pack files need to be deleted that were originally hard-links to the reference repository's pack files. On platforms where a file cannot be deleted if another process still holds a handle on it, we therefore need to take pains to release all pack files and indexes before dissociating. This fixes https://github.com/git-for-windows/git/issues/446 The test case to demonstrate the breakage technically does not need to be run on Linux or MacOSX. It won't hurt, either, though. Signed-off-by: Johannes Schindelin --- builtin/clone.c | 4 +++- t/t5700-clone-reference.sh | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index 578da85..cc896e2 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -1064,8 +1064,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix) transport_unlock_pack(transport); transport_disconnect(transport); - if (option_dissociate) + if (option_dissociate) { + close_all_packs(); dissociate_from_references(); + } junk_mode = JUNK_LEAVE_REPO; err = checkout(); diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh index f7cec85..2250ef4 100755 --- a/t/t5700-clone-reference.sh +++ b/t/t5700-clone-reference.sh @@ -188,7 +188,7 @@ test_expect_success 'clone and dissociate from reference' ' test_must_fail git -C R fsck && git -C S fsck ' -test_expect_failure MINGW 'clone, dissociate from partial reference and repack' ' +test_expect_success 'clone, dissociate from partial reference and repack' ' rm -fr P Q R && git init P && ( -- 2.6.1.windows.1 ``` ## Johannes Schindelin, 2015-10-06 13:42 Subject: Re: [PATCH v2 2/4] Consolidate code to close a pack's file descriptor Message-ID: URL: https://gitlist.dev/e/fb0ee65afcb4087de1137d27991dc896%40dscho.org In-Reply-To: ``` Hi Junio, On 2015-10-06 00:15, Junio C Hamano wrote: > Johannes Schindelin writes: > >>>> + && !p->do_not_close) >>>> + close_pack_fd(p); >>> >>> I wonder how this do_not_close bit should interact with "we are >>> shutting down (or we are spawning another to do the real work, while >>> we won't do anything useful anymore), so close everything down". >> >> The `close_all_packs()` function is meant for the use case where you >> really close all the packs, no question asked. >> >> I 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. > > Do not wait for being convinced forever, as I am not interested in > convincing you either way. Decision by made-up examples or lack of > imagination is a waste of time ;-). > > My earlier "I wonder" leads me to totally different conclusion, > which is "we declare that it is a bug for any caller to call > close-all-packs while marking any open pack with do_not_close bit". > > Which merely means that while iterating over packs in the > "close-all" loop, we should throw a die("BUG") at the caller if that > bit is on. That way, we won't have to rely on "we did not think of > a good use case so we unconditionally closed the packs without > telling the remainder of the system, hopefully we broke nothing." I just sent another iteration out that adds this sanity check. Ciao, Dscho ``` ## Junio C Hamano, 2015-10-07 17:49 Subject: Re: [PATCH v3 3/4] Add a function to release all packs Message-ID: URL: https://gitlist.dev/e/xmqq4mi2va9u.fsf%40gitster.mtv.corp.google.com In-Reply-To: <9953f8ed40d0cf99f1be7cfe2fa0b699b7769645.1444131898.git.johannes.schindelin@gmx.de> ``` Johannes Schindelin writes: > On Windows, files that are in use cannot be removed or renamed. That > means that we have to release pack files when we are about to, say, > repack them. Let's introduce a convenient function to close them > pack files. > > While at it, we consolidate the close windows/close fd/close index > stanza in `free_pack_by_name()` into the `close_pack()` function that > is used by the new `close_all_packs()` function to avoid repeated code. > > Signed-off-by: Johannes Schindelin > --- This is the only one that was updated among the four and the change looks sensible. I'd reword the end of the first paragraph of the proposed log message, though, to replace "close them pack files" with "close all the pack files and their idx files". Thanks. > cache.h | 1 + > sha1_file.c | 23 ++++++++++++++++++++--- > 2 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/cache.h b/cache.h > index 752031e..57f6a74 100644 > --- a/cache.h > +++ b/cache.h > @@ -1275,6 +1275,7 @@ extern void close_pack_index(struct packed_git *); > > extern unsigned char *use_pack(struct packed_git *, struct pack_window **, off_t, unsigned long *); > extern void close_pack_windows(struct packed_git *); > +extern void close_all_packs(void); > extern void unuse_pack(struct pack_window **); > extern void free_pack_by_name(const char *); > extern void clear_delta_base_cache(void); > diff --git a/sha1_file.c b/sha1_file.c > index 8c3c913..ca699d7 100644 > --- a/sha1_file.c > +++ b/sha1_file.c > @@ -798,6 +798,25 @@ static int close_pack_fd(struct packed_git *p) > return 1; > } > > +static void close_pack(struct packed_git *p) > +{ > + close_pack_windows(p); > + close_pack_fd(p); > + close_pack_index(p); > +} > + > +void close_all_packs(void) > +{ > + struct packed_git *p; > + > + for (p = packed_git; p; p = p->next) > + if (p->do_not_close) > + die("BUG! Want to close pack marked 'do-not-close'"); > + else > + close_pack(p); > +} > + > + > /* > * The LRU pack is the one with the oldest MRU window, preferring packs > * with no used windows, or the oldest mtime if it has no windows allocated. > @@ -906,9 +925,7 @@ void free_pack_by_name(const char *pack_name) > p = *pp; > if (strcmp(pack_name, p->pack_name) == 0) { > clear_delta_base_cache(); > - close_pack_windows(p); > - close_pack_fd(p); > - close_pack_index(p); > + close_pack(p); > free(p->bad_object_sha1); > *pp = p->next; > if (last_found_pack == p) ``` ## Johannes Schindelin, 2015-10-08 19:10 Subject: Re: [PATCH v3 3/4] Add a function to release all packs Message-ID: <991eaf53b382e8156ef9486014c1a4a5@dscho.org> URL: https://gitlist.dev/e/991eaf53b382e8156ef9486014c1a4a5%40dscho.org In-Reply-To: ``` Hi Junio, On 2015-10-07 19:49, Junio C Hamano wrote: > Johannes Schindelin writes: > >> On Windows, files that are in use cannot be removed or renamed. That >> means that we have to release pack files when we are about to, say, >> repack them. Let's introduce a convenient function to close them >> pack files. >> >> While at it, we consolidate the close windows/close fd/close index >> stanza in `free_pack_by_name()` into the `close_pack()` function that >> is used by the new `close_all_packs()` function to avoid repeated code. >> >> Signed-off-by: Johannes Schindelin >> --- > > This is the only one that was updated among the four and the change > looks sensible. I'd reword the end of the first paragraph of the > proposed log message, though, to replace "close them pack files" > with "close all the pack files and their idx files". > > Thanks. This time I was more clever and checked `pu` before sending off a fixed patch, seeing that you patched this already. Thanks, Dscho ``` ## Max Kirillov, 2015-10-11 10:45 Subject: Re: [PATCH v3 0/4] Fix locking issues on Windows with `git clone --dissociate` Message-ID: <20151011104551.GA13032@wheezy.local> URL: https://gitlist.dev/e/20151011104551.GA13032%40wheezy.local In-Reply-To: ``` On Tue, Oct 06, 2015 at 03:17:36PM +0200, Johannes Schindelin wrote: > This is version 3, adding that BUG! message if do_not_close was set. > > Max, I still hope that this patch series helps also your use case! Thanks, this mostly makes gone one of my commits. I only need to invoke the function after builtin is done. And maybe spend some time to check that dies do not happen. The other one with cloexec I think I will still need. It migh take some time before I return to it. -- Max ```