* [PATCH 0/1] bundle: fix issue when bundles would be empty @ 2018-11-13 15:09 Johannes Schindelin via GitGitGadget 2018-11-13 15:09 ` [PATCH 1/1] bundle: refuse to create empty bundle Gaël Lhez via GitGitGadget 2018-11-14 15:25 ` [PATCH v2 0/1] bundle: fix issue when bundles would be empty Johannes Schindelin via GitGitGadget 0 siblings, 2 replies; 20+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2018-11-13 15:09 UTC (permalink / raw) To: git; +Cc: Junio C Hamano And yet another patch coming through Git for Windows... Gaël Lhez (1): bundle: refuse to create empty bundle bundle.c | 7 ++++--- t/t5607-clone-bundle.sh | 4 ++++ 2 files changed, 8 insertions(+), 3 deletions(-) base-commit: 8858448bb49332d353febc078ce4a3abcc962efe Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-79%2Fdscho%2Fcreate-empty-bundle-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-79/dscho/create-empty-bundle-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/79 -- gitgitgadget ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/1] bundle: refuse to create empty bundle 2018-11-13 15:09 [PATCH 0/1] bundle: fix issue when bundles would be empty Johannes Schindelin via GitGitGadget @ 2018-11-13 15:09 ` Gaël Lhez via GitGitGadget 2018-11-13 19:28 ` Stefan Beller 2018-11-14 15:25 ` [PATCH v2 0/1] bundle: fix issue when bundles would be empty Johannes Schindelin via GitGitGadget 1 sibling, 1 reply; 20+ messages in thread From: Gaël Lhez via GitGitGadget @ 2018-11-13 15:09 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Gaël Lhez From: =?UTF-8?q?Ga=C3=ABl=20Lhez?= <gael.lhez@gmail.com> When an user tries to create an empty bundle via `git bundle create <bundle> <revlist>` where `<revlist>` resolves to an empty list (for example, like `master..master`), the command fails and warns the user about how it does not want to create empty bundle. However, the `.lock` file was still open and on Windows that means that it could not be deleted properly. This patch fixes that issue. This closes https://github.com/git-for-windows/git/issues/790 Signed-off-by: Gaël Lhez <gael.lhez@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- bundle.c | 7 ++++--- t/t5607-clone-bundle.sh | 4 ++++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/bundle.c b/bundle.c index 1ef584b93b..4e349feff9 100644 --- a/bundle.c +++ b/bundle.c @@ -457,10 +457,11 @@ int create_bundle(struct bundle_header *header, const char *path, object_array_remove_duplicates(&revs.pending); ref_count = write_bundle_refs(bundle_fd, &revs); - if (!ref_count) - die(_("Refusing to create empty bundle.")); - else if (ref_count < 0) + if (ref_count <= 0) { + if (!ref_count) + error(_("Refusing to create empty bundle.")); goto err; + } /* write pack */ if (write_pack_data(bundle_fd, &revs)) { diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh index 348d9b3bc7..f84b875950 100755 --- a/t/t5607-clone-bundle.sh +++ b/t/t5607-clone-bundle.sh @@ -71,4 +71,8 @@ test_expect_success 'prerequisites with an empty commit message' ' git bundle verify bundle ' +test_expect_success 'try to create a bundle with empty ref count' ' + test_expect_code 1 git bundle create foobar.bundle master..master +' + test_done -- gitgitgadget ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/1] bundle: refuse to create empty bundle 2018-11-13 15:09 ` [PATCH 1/1] bundle: refuse to create empty bundle Gaël Lhez via GitGitGadget @ 2018-11-13 19:28 ` Stefan Beller 2018-11-13 20:37 ` Gaël Lhez [not found] ` <CAK8L4uiMHrsdwJz9+rD1tSCywL2kHosx-hKZdS=UtZDHLy464A@mail.gmail.com> 0 siblings, 2 replies; 20+ messages in thread From: Stefan Beller @ 2018-11-13 19:28 UTC (permalink / raw) To: gitgitgadget; +Cc: git, Junio C Hamano, gael.lhez On Tue, Nov 13, 2018 at 7:09 AM Gaël Lhez via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: =?UTF-8?q?Ga=C3=ABl=20Lhez?= <gael.lhez@gmail.com> > > When an user tries to create an empty bundle via `git bundle create > <bundle> <revlist>` where `<revlist>` resolves to an empty list (for > example, like `master..master`), the command fails and warns the user > about how it does not want to create empty bundle. > > However, the `.lock` file was still open and on Windows that means > that it could not be deleted properly. This patch fixes that issue. > > This closes https://github.com/git-for-windows/git/issues/790 > > Signed-off-by: Gaël Lhez <gael.lhez@gmail.com> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> The code and the commit message make sense, but by reading the subject line I would have expected a different commit. Maybe "bundle: cleanup lock files on error" ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/1] bundle: refuse to create empty bundle 2018-11-13 19:28 ` Stefan Beller @ 2018-11-13 20:37 ` Gaël Lhez [not found] ` <CAK8L4uiMHrsdwJz9+rD1tSCywL2kHosx-hKZdS=UtZDHLy464A@mail.gmail.com> 1 sibling, 0 replies; 20+ messages in thread From: Gaël Lhez @ 2018-11-13 20:37 UTC (permalink / raw) To: Stefan Beller; +Cc: gitgitgadget, git, Junio C Hamano Hello, I don't know why I receive these message (and especially now given the time at which I pushed this) but I suppose someone (Johannes Schindelin ?) probably pushed back my original commit from git for windows github to GIT git repository. If you think "bundle: cleanup lock files on error" is better, then no problem with me. I'm not a native english speaker and I simply expressed the reason for my problem but - after reading back my commit - neither this mail' subject and my original commit subject (see https://github.com/git-for-windows/git/pull/797/commits/0ef742a1a92ae53188189238adbd16086fabf80a) express the core problem. As I'm not accustomed to pushing on GIT 'git repository' , please let me know if I have something else to do ? (Sent back in text mode because HTML is considered SPAM or Virus by git@vger.kernel.org). Regards, Gaël Le mar. 13 nov. 2018 à 20:28, Stefan Beller <sbeller@google.com> a écrit : > > On Tue, Nov 13, 2018 at 7:09 AM Gaël Lhez via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > > > From: =?UTF-8?q?Ga=C3=ABl=20Lhez?= <gael.lhez@gmail.com> > > > > When an user tries to create an empty bundle via `git bundle create > > <bundle> <revlist>` where `<revlist>` resolves to an empty list (for > > example, like `master..master`), the command fails and warns the user > > about how it does not want to create empty bundle. > > > > However, the `.lock` file was still open and on Windows that means > > that it could not be deleted properly. This patch fixes that issue. > > > > This closes https://github.com/git-for-windows/git/issues/790 > > > > Signed-off-by: Gaël Lhez <gael.lhez@gmail.com> > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > The code and the commit message make sense, but by reading the subject line > > I would have expected a different commit. Maybe > "bundle: cleanup lock files on error" ^ permalink raw reply [flat|nested] 20+ messages in thread
[parent not found: <CAK8L4uiMHrsdwJz9+rD1tSCywL2kHosx-hKZdS=UtZDHLy464A@mail.gmail.com>]
* Re: [PATCH 1/1] bundle: refuse to create empty bundle [not found] ` <CAK8L4uiMHrsdwJz9+rD1tSCywL2kHosx-hKZdS=UtZDHLy464A@mail.gmail.com> @ 2018-11-13 21:11 ` Stefan Beller 2018-11-14 15:23 ` Johannes Schindelin 0 siblings, 1 reply; 20+ messages in thread From: Stefan Beller @ 2018-11-13 21:11 UTC (permalink / raw) To: gael.lhez; +Cc: gitgitgadget, git, Junio C Hamano On Tue, Nov 13, 2018 at 12:33 PM Gaël Lhez <gael.lhez@gmail.com> wrote: > > Hello, > > I don't know why I receive these message (and especially now given the time at which I pushed this) but I suppose someone (Johannes Schindelin ?) probably pushed back my original commit from git for windows github to GIT git repository. Yes that is pretty much what is happening. Johannes (GfW maintainer) tries to push a lot of patches upstream to git and cc's people who originally authored the patch. Thanks for taking a look, again, even after this long time! > > If you think "bundle: cleanup lock files on error" is better, then no problem with me. I'm not a native english speaker and I simply expressed the reason for my problem but - after reading back my commit - neither this mail' subject and my original commit subject (see https://github.com/git-for-windows/git/pull/797/commits/0ef742a1a92ae53188189238adbd16086fabf80a) express the core problem. I am not a native speaker either, which makes it extra hard to understand some commits. ;-) So I propose a wording which would have helped me. > As I'm not accustomed to pushing on GIT 'git repository' , please let me know if I have something else to do ? I don't know how Johannes handles pushing changes upstream, maybe he will take on the work of resending a reworded patch. Let's hear his thoughts on it. I would guess you're more than welcome to take your patches from GitForWindows into Git itself. Cheers, Stefan ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/1] bundle: refuse to create empty bundle 2018-11-13 21:11 ` Stefan Beller @ 2018-11-14 15:23 ` Johannes Schindelin 0 siblings, 0 replies; 20+ messages in thread From: Johannes Schindelin @ 2018-11-14 15:23 UTC (permalink / raw) To: Stefan Beller; +Cc: gael.lhez, gitgitgadget, git, Junio C Hamano [-- Attachment #1: Type: text/plain, Size: 2146 bytes --] Hi Stefan, On Tue, 13 Nov 2018, Stefan Beller wrote: > On Tue, Nov 13, 2018 at 12:33 PM Gaël Lhez <gael.lhez@gmail.com> wrote: > > > > Hello, > > > > I don't know why I receive these message (and especially now given the time at which I pushed this) but I suppose someone (Johannes Schindelin ?) probably pushed back my original commit from git for windows github to GIT git repository. > > Yes that is pretty much what is happening. Johannes (GfW maintainer) > tries to push a lot of patches upstream to git and cc's people who > originally authored the patch. > Thanks for taking a look, again, even after this long time! > > > > > If you think "bundle: cleanup lock files on error" is better, then no problem with me. I'm not a native english speaker and I simply expressed the reason for my problem but - after reading back my commit - neither this mail' subject and my original commit subject (see https://github.com/git-for-windows/git/pull/797/commits/0ef742a1a92ae53188189238adbd16086fabf80a) express the core problem. > > I am not a native speaker either, which makes it extra hard to > understand some commits. ;-) So I propose a wording which would have > helped me. > > > As I'm not accustomed to pushing on GIT 'git repository' , please let me know if I have something else to do ? > > I don't know how Johannes handles pushing changes upstream, maybe he > will take on the work of resending a reworded patch. He will. > Let's hear his thoughts on it. I would guess you're more than welcome > to take your patches from GitForWindows into Git itself. As I merged the patch into Git for Windows' `master`, I consider it my responsibility to push this upstream (unless the contributor wants to take matters into their own hands). In the future, my hope is that GitGitGadget will make contributing patches to the Git mailing list a more convenient experience, to the point that it will hopefully be pretty much as easy as iterating a PR in https://github.com/git-for-windows/git. At that point, I will ask contributors to do exactly that in order to shepherd their patches into git.git. Ciao, Dscho > > Cheers, > Stefan > ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 0/1] bundle: fix issue when bundles would be empty 2018-11-13 15:09 [PATCH 0/1] bundle: fix issue when bundles would be empty Johannes Schindelin via GitGitGadget 2018-11-13 15:09 ` [PATCH 1/1] bundle: refuse to create empty bundle Gaël Lhez via GitGitGadget @ 2018-11-14 15:25 ` Johannes Schindelin via GitGitGadget 2018-11-14 15:25 ` [PATCH v2 1/1] bundle: cleanup lock files on error Gaël Lhez via GitGitGadget 1 sibling, 1 reply; 20+ messages in thread From: Johannes Schindelin via GitGitGadget @ 2018-11-14 15:25 UTC (permalink / raw) To: git; +Cc: Stefan Beller, Junio C Hamano And yet another patch coming through Git for Windows... Change since v1: * Using a better oneline now. Gaël Lhez (1): bundle: cleanup lock files on error bundle.c | 7 ++++--- t/t5607-clone-bundle.sh | 4 ++++ 2 files changed, 8 insertions(+), 3 deletions(-) base-commit: 8858448bb49332d353febc078ce4a3abcc962efe Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-79%2Fdscho%2Fcreate-empty-bundle-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-79/dscho/create-empty-bundle-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/79 Range-diff vs v1: 1: 6276372ad3 ! 1: c88887f05a bundle: refuse to create empty bundle @@ -1,6 +1,6 @@ Author: Gaël Lhez <gael.lhez@gmail.com> - bundle: refuse to create empty bundle + bundle: cleanup lock files on error When an user tries to create an empty bundle via `git bundle create <bundle> <revlist>` where `<revlist>` resolves to an empty list (for -- gitgitgadget ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 1/1] bundle: cleanup lock files on error 2018-11-14 15:25 ` [PATCH v2 0/1] bundle: fix issue when bundles would be empty Johannes Schindelin via GitGitGadget @ 2018-11-14 15:25 ` Gaël Lhez via GitGitGadget 2018-11-14 21:43 ` Martin Ågren 0 siblings, 1 reply; 20+ messages in thread From: Gaël Lhez via GitGitGadget @ 2018-11-14 15:25 UTC (permalink / raw) To: git; +Cc: Stefan Beller, Junio C Hamano, Gaël Lhez From: =?UTF-8?q?Ga=C3=ABl=20Lhez?= <gael.lhez@gmail.com> When an user tries to create an empty bundle via `git bundle create <bundle> <revlist>` where `<revlist>` resolves to an empty list (for example, like `master..master`), the command fails and warns the user about how it does not want to create empty bundle. However, the `.lock` file was still open and on Windows that means that it could not be deleted properly. This patch fixes that issue. This closes https://github.com/git-for-windows/git/issues/790 Signed-off-by: Gaël Lhez <gael.lhez@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- bundle.c | 7 ++++--- t/t5607-clone-bundle.sh | 4 ++++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/bundle.c b/bundle.c index 1ef584b93b..4e349feff9 100644 --- a/bundle.c +++ b/bundle.c @@ -457,10 +457,11 @@ int create_bundle(struct bundle_header *header, const char *path, object_array_remove_duplicates(&revs.pending); ref_count = write_bundle_refs(bundle_fd, &revs); - if (!ref_count) - die(_("Refusing to create empty bundle.")); - else if (ref_count < 0) + if (ref_count <= 0) { + if (!ref_count) + error(_("Refusing to create empty bundle.")); goto err; + } /* write pack */ if (write_pack_data(bundle_fd, &revs)) { diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh index 348d9b3bc7..f84b875950 100755 --- a/t/t5607-clone-bundle.sh +++ b/t/t5607-clone-bundle.sh @@ -71,4 +71,8 @@ test_expect_success 'prerequisites with an empty commit message' ' git bundle verify bundle ' +test_expect_success 'try to create a bundle with empty ref count' ' + test_expect_code 1 git bundle create foobar.bundle master..master +' + test_done -- gitgitgadget ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] bundle: cleanup lock files on error 2018-11-14 15:25 ` [PATCH v2 1/1] bundle: cleanup lock files on error Gaël Lhez via GitGitGadget @ 2018-11-14 21:43 ` Martin Ågren 2018-11-14 22:08 ` Stefan Beller 0 siblings, 1 reply; 20+ messages in thread From: Martin Ågren @ 2018-11-14 21:43 UTC (permalink / raw) To: gitgitgadget; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano, gael.lhez On Wed, 14 Nov 2018 at 16:26, Gaël Lhez via GitGitGadget <gitgitgadget@gmail.com> wrote: > However, the `.lock` file was still open and on Windows that means > that it could not be deleted properly. This patch fixes that issue. Hmmm, doesn't the tempfile machinery remove the lock file when we die? > ref_count = write_bundle_refs(bundle_fd, &revs); > - if (!ref_count) > - die(_("Refusing to create empty bundle.")); > - else if (ref_count < 0) > + if (ref_count <= 0) { > + if (!ref_count) > + error(_("Refusing to create empty bundle.")); > goto err; > + } One less `die()` in libgit -- nice. > +test_expect_success 'try to create a bundle with empty ref count' ' > + test_expect_code 1 git bundle create foobar.bundle master..master > +' This tries to make sure that we don't just die, but that we exit in a "controlled" way with exit code 1. After reading the log message, I had perhaps expected something like test_must_fail git bundle [...] && test_path_is_missing foobar.bundle.lock That relies on magically knowing the ".lock" suffix, but my point is that for me (on Linux), this alternative test passes both before and after the patch. Before, because tempfile.c cleans up; after, because bundle.c does so. Doesn't this happen on Windows too? What am I missing? Martin ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] bundle: cleanup lock files on error 2018-11-14 21:43 ` Martin Ågren @ 2018-11-14 22:08 ` Stefan Beller 2018-11-15 4:34 ` Jeff King 0 siblings, 1 reply; 20+ messages in thread From: Stefan Beller @ 2018-11-14 22:08 UTC (permalink / raw) To: Martin Ågren; +Cc: gitgitgadget, git, Junio C Hamano, Gaël Lhez On Wed, Nov 14, 2018 at 1:43 PM Martin Ågren <martin.agren@gmail.com> wrote: > > On Wed, 14 Nov 2018 at 16:26, Gaël Lhez via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > However, the `.lock` file was still open and on Windows that means > > that it could not be deleted properly. This patch fixes that issue. > > Hmmm, doesn't the tempfile machinery remove the lock file when we die? On Windows this seems not to be the case. (Open files cannot be deleted as the open file is not kept by inode or similar but by the file path there?) Rewording your concern: Could the tempfile machinery be taught to work properly on Windows, e.g. by first closing all files and then deleting them afterwards? There was a refactoring of tempfiles merged in 89563ec379 (Merge branch 'jk/incore-lockfile-removal', 2017-09-19), which sounded promising at first, as it is dated after the original patch[1] date (June 2016), but it has no references for Windows being different, so we might still have the original issue; most of the lockfile infrastructure was done in 2015 via db86e61cbb (Merge branch 'mh/tempfile', 2015-08-25) [1] https://github.com/git-for-windows/git/pull/797 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] bundle: cleanup lock files on error 2018-11-14 22:08 ` Stefan Beller @ 2018-11-15 4:34 ` Jeff King 2018-11-15 12:57 ` Johannes Schindelin 0 siblings, 1 reply; 20+ messages in thread From: Jeff King @ 2018-11-15 4:34 UTC (permalink / raw) To: Stefan Beller Cc: Martin Ågren, gitgitgadget, git, Junio C Hamano, Gaël Lhez On Wed, Nov 14, 2018 at 02:08:48PM -0800, Stefan Beller wrote: > On Wed, Nov 14, 2018 at 1:43 PM Martin Ågren <martin.agren@gmail.com> wrote: > > > > On Wed, 14 Nov 2018 at 16:26, Gaël Lhez via GitGitGadget > > <gitgitgadget@gmail.com> wrote: > > > However, the `.lock` file was still open and on Windows that means > > > that it could not be deleted properly. This patch fixes that issue. > > > > Hmmm, doesn't the tempfile machinery remove the lock file when we die? > > On Windows this seems not to be the case. (Open files cannot be deleted > as the open file is not kept by inode or similar but by the file path there?) > > Rewording your concern: Could the tempfile machinery be taught to > work properly on Windows, e.g. by first closing all files and then deleting > them afterwards? It already tries to do so. See delete_tempfile(), or more likely in the die() case, the remove_tempfiles() handler which is called at exit. Are we sure this is still a problem? I looked at the test to see if it would pass, but it is not even checking anything about lockfiles! It just checks that we exit 1 by returning up the callstack instead of calling die(). And of course it would not have a problem under Linux either way. But if I run something similar under strace, I see: $ strace ./git bundle create foobar.bundle HEAD..HEAD [...] openat(AT_FDCWD, "/home/peff/compile/git/foobar.bundle.lock", O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3 [...] close(3) = 0 unlink("/home/peff/compile/git/foobar.bundle.lock") = 0 exit_group(128) = ? which seems right. -Peff ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] bundle: cleanup lock files on error 2018-11-15 4:34 ` Jeff King @ 2018-11-15 12:57 ` Johannes Schindelin 2018-11-15 13:37 ` Jeff King 2018-11-15 16:49 ` [PATCH v2 1/1] bundle: cleanup lock files on error Duy Nguyen 0 siblings, 2 replies; 20+ messages in thread From: Johannes Schindelin @ 2018-11-15 12:57 UTC (permalink / raw) To: Jeff King Cc: Stefan Beller, Martin Ågren, gitgitgadget, git, Junio C Hamano, Gaël Lhez [-- Attachment #1: Type: text/plain, Size: 3338 bytes --] Hi Peff, On Wed, 14 Nov 2018, Jeff King wrote: > On Wed, Nov 14, 2018 at 02:08:48PM -0800, Stefan Beller wrote: > > > On Wed, Nov 14, 2018 at 1:43 PM Martin Ågren <martin.agren@gmail.com> wrote: > > > > > > On Wed, 14 Nov 2018 at 16:26, Gaël Lhez via GitGitGadget > > > <gitgitgadget@gmail.com> wrote: > > > > However, the `.lock` file was still open and on Windows that means > > > > that it could not be deleted properly. This patch fixes that issue. > > > > > > Hmmm, doesn't the tempfile machinery remove the lock file when we die? > > > > On Windows this seems not to be the case. (Open files cannot be deleted > > as the open file is not kept by inode or similar but by the file path there?) > > > > Rewording your concern: Could the tempfile machinery be taught to > > work properly on Windows, e.g. by first closing all files and then deleting > > them afterwards? > > It already tries to do so. See delete_tempfile(), or more likely in the > die() case, the remove_tempfiles() handler which is called at exit. > > Are we sure this is still a problem? > > I looked at the test to see if it would pass, but it is not even > checking anything about lockfiles! It just checks that we exit 1 by > returning up the callstack instead of calling die(). And of course it > would not have a problem under Linux either way. But if I run something > similar under strace, I see: > > $ strace ./git bundle create foobar.bundle HEAD..HEAD > [...] > openat(AT_FDCWD, "/home/peff/compile/git/foobar.bundle.lock", O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3 > [...] > close(3) = 0 > unlink("/home/peff/compile/git/foobar.bundle.lock") = 0 > exit_group(128) = ? > > which seems right. Without the fix, the added regression test fails thusly: -- snip -- [...] ++ test_expect_code 1 git bundle create foobar.bundle master..master ++ want_code=1 ++ shift ++ git bundle create foobar.bundle master..master fatal: Refusing to create empty bundle. warning: unable to unlink 'C:/git-sdk-64/usr/src/git/wip2/t/trash directory.t5607-clone-bundle/foobar.bundle.lock': Permission denied ++ exit_code=128 ++ test 128 = 1 ++ echo 'test_expect_code: command exited with 128, we wanted 1 git bundle create foobar.bundle master..master' test_expect_code: command exited with 128, we wanted 1 git bundle create foobar.bundle master..master ++ return 1 error: last command exited with $?=1 not ok 9 - try to create a bundle with empty ref count # # test_expect_code 1 git bundle create foobar.bundle master..master # -- snap -- So yes, we are trying to unlink the `.lock` file, and as far as I can tell that `unlink()` call comes from the tempfile cleanup asked for by Martin. However, as we still have a handle open to that file, that call fails. I do not think that there is any better way to fix this than to close the file explicitly. If we tried to just close whatever file descriptor is still open to that file before deleting it, we would possibly cause problems in code that is still to be executed and assumes that it has a perfectly valid file descriptor. Besides, trying to do this kind of "automatically" won't work, like, at all, when it is one child process that holds an open file descriptor while another process wants to delete the file. Ciao, Dscho ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] bundle: cleanup lock files on error 2018-11-15 12:57 ` Johannes Schindelin @ 2018-11-15 13:37 ` Jeff King 2018-11-15 16:32 ` Johannes Schindelin 2018-11-15 16:49 ` [PATCH v2 1/1] bundle: cleanup lock files on error Duy Nguyen 1 sibling, 1 reply; 20+ messages in thread From: Jeff King @ 2018-11-15 13:37 UTC (permalink / raw) To: Johannes Schindelin Cc: Stefan Beller, Martin Ågren, gitgitgadget, git, Junio C Hamano, Gaël Lhez On Thu, Nov 15, 2018 at 01:57:45PM +0100, Johannes Schindelin wrote: > > I looked at the test to see if it would pass, but it is not even > > checking anything about lockfiles! It just checks that we exit 1 by > > returning up the callstack instead of calling die(). And of course it > > would not have a problem under Linux either way. But if I run something > > similar under strace, I see: > > > > $ strace ./git bundle create foobar.bundle HEAD..HEAD > > [...] > > openat(AT_FDCWD, "/home/peff/compile/git/foobar.bundle.lock", O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3 > > [...] > > close(3) = 0 > > unlink("/home/peff/compile/git/foobar.bundle.lock") = 0 > > exit_group(128) = ? > > > > which seems right. > > Without the fix, the added regression test fails thusly: > > -- snip -- > [...] > ++ test_expect_code 1 git bundle create foobar.bundle master..master > ++ want_code=1 > ++ shift > ++ git bundle create foobar.bundle master..master > fatal: Refusing to create empty bundle. > warning: unable to unlink 'C:/git-sdk-64/usr/src/git/wip2/t/trash directory.t5607-clone-bundle/foobar.bundle.lock': Permission denied Hmph. So who has it open, and why isn't the tempfile code working as designed? Aha, I see the problem. We dup() the descriptor in create_bundle(). So the patch _is_ necessary and (fairly) correct. But the explanation probably ought to be something like: In create_bundle(), we duplicate the lockfile descriptor via dup(). This means that even though the lockfile code carefully calls close() before unlinking the lockfile, we may still have the file open. Unix systems don't care, but under Windows, this prevents the unlink (causing an annoying warning and a stale lockfile). But that also means that all of the other places we could die (e.g., in write_or_die) are going to have the same problem. We've fixed only one. Is there a way we can avoid doing the dup() in the first place? The comment there explains that we duplicate because write_pack_data() will close the descriptor. Unfortunately, that's hard to change because it comes from run-command. But we don't actually need the descriptor ourselves after it's closed; we're just trying to appease the lockfile code; see e54c347c1c (create_bundle(): duplicate file descriptor to avoid closing it twice, 2015-08-10). We just need some reasonable way of telling the lock code what's happening. Something like the patch below, which is a moral revert of e54c347c1c, but instead wrapping the "lock->fd = -1" in an official API. Does this make your warning go away? diff --git a/bundle.c b/bundle.c index 1ef584b93b..dc26551b83 100644 --- a/bundle.c +++ b/bundle.c @@ -244,7 +244,7 @@ static int is_tag_in_date_range(struct object *tag, struct rev_info *revs) /* Write the pack data to bundle_fd, then close it if it is > 1. */ -static int write_pack_data(int bundle_fd, struct rev_info *revs) +static int write_pack_data(int bundle_fd, struct lock_file *lock, struct rev_info *revs) { struct child_process pack_objects = CHILD_PROCESS_INIT; int i; @@ -256,6 +256,14 @@ static int write_pack_data(int bundle_fd, struct rev_info *revs) pack_objects.in = -1; pack_objects.out = bundle_fd; pack_objects.git_cmd = 1; + + /* + * At this point we know that start_command is going to close our + * bundle_fd, whether successful or not. Tell the lock code that + * it is no longer in charge of it, so we don't try to double-close. + */ + lock_file_release_descriptor(lock); + if (start_command(&pack_objects)) return error(_("Could not spawn pack-objects")); @@ -421,21 +429,10 @@ int create_bundle(struct bundle_header *header, const char *path, bundle_to_stdout = !strcmp(path, "-"); if (bundle_to_stdout) bundle_fd = 1; - else { + else bundle_fd = hold_lock_file_for_update(&lock, path, LOCK_DIE_ON_ERROR); - /* - * write_pack_data() will close the fd passed to it, - * but commit_lock_file() will also try to close the - * lockfile's fd. So make a copy of the file - * descriptor to avoid trying to close it twice. - */ - bundle_fd = dup(bundle_fd); - if (bundle_fd < 0) - die_errno("unable to dup file descriptor"); - } - /* write signature */ write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature)); @@ -463,10 +460,8 @@ int create_bundle(struct bundle_header *header, const char *path, goto err; /* write pack */ - if (write_pack_data(bundle_fd, &revs)) { - bundle_fd = -1; /* already closed by the above call */ + if (write_pack_data(bundle_fd, &lock, &revs)) goto err; - } if (!bundle_to_stdout) { if (commit_lock_file(&lock)) @@ -474,11 +469,7 @@ int create_bundle(struct bundle_header *header, const char *path, } return 0; err: - if (!bundle_to_stdout) { - if (0 <= bundle_fd) - close(bundle_fd); - rollback_lock_file(&lock); - } + rollback_lock_file(&lock); return -1; } diff --git a/lockfile.h b/lockfile.h index 35403ccc0d..968cd0e4f6 100644 --- a/lockfile.h +++ b/lockfile.h @@ -304,4 +304,14 @@ static inline void rollback_lock_file(struct lock_file *lk) delete_tempfile(&lk->tempfile); } +/* + * Release the file descriptor and/or file pointer for an open lockfile, + * passing ownership to the caller (who is responsible for closing it). It's a + * NOOP to call this on an inactive or already-closed lockfile. + */ +static inline void lock_file_release_descriptor(struct lock_file *lk) +{ + tempfile_release_descriptor(lk->tempfile); +} + #endif /* LOCKFILE_H */ diff --git a/tempfile.c b/tempfile.c index d43ad8c191..59bbdcf4ba 100644 --- a/tempfile.c +++ b/tempfile.c @@ -319,3 +319,9 @@ void delete_tempfile(struct tempfile **tempfile_p) deactivate_tempfile(tempfile); *tempfile_p = NULL; } + +void tempfile_release_descriptor(struct tempfile *tempfile) +{ + tempfile->fd = -1; + tempfile->fp = NULL; +} diff --git a/tempfile.h b/tempfile.h index 61d8dc4d1b..7489c5e4e9 100644 --- a/tempfile.h +++ b/tempfile.h @@ -262,4 +262,11 @@ extern void delete_tempfile(struct tempfile **tempfile_p); */ extern int rename_tempfile(struct tempfile **tempfile_p, const char *path); +/* + * Release the file descriptor and/or file pointer for an open tempfile, + * passing ownership to the caller (who is responsible for closing it). It's a + * NOOP to call this on an inactive or already-closed tempfile. + */ +void tempfile_release_descriptor(struct tempfile *tempfile); + #endif /* TEMPFILE_H */ ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] bundle: cleanup lock files on error 2018-11-15 13:37 ` Jeff King @ 2018-11-15 16:32 ` Johannes Schindelin 2018-11-15 16:43 ` Jeff King 0 siblings, 1 reply; 20+ messages in thread From: Johannes Schindelin @ 2018-11-15 16:32 UTC (permalink / raw) To: Jeff King Cc: Stefan Beller, Martin Ågren, gitgitgadget, git, Junio C Hamano, Gaël Lhez Hi Peff, On Thu, 15 Nov 2018, Jeff King wrote: > On Thu, Nov 15, 2018 at 01:57:45PM +0100, Johannes Schindelin wrote: > > > > I looked at the test to see if it would pass, but it is not even > > > checking anything about lockfiles! It just checks that we exit 1 by > > > returning up the callstack instead of calling die(). And of course it > > > would not have a problem under Linux either way. But if I run something > > > similar under strace, I see: > > > > > > $ strace ./git bundle create foobar.bundle HEAD..HEAD > > > [...] > > > openat(AT_FDCWD, "/home/peff/compile/git/foobar.bundle.lock", O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3 > > > [...] > > > close(3) = 0 > > > unlink("/home/peff/compile/git/foobar.bundle.lock") = 0 > > > exit_group(128) = ? > > > > > > which seems right. > > > > Without the fix, the added regression test fails thusly: > > > > -- snip -- > > [...] > > ++ test_expect_code 1 git bundle create foobar.bundle master..master > > ++ want_code=1 > > ++ shift > > ++ git bundle create foobar.bundle master..master > > fatal: Refusing to create empty bundle. > > warning: unable to unlink 'C:/git-sdk-64/usr/src/git/wip2/t/trash directory.t5607-clone-bundle/foobar.bundle.lock': Permission denied > > Hmph. So who has it open, and why isn't the tempfile code working as > designed? > > Aha, I see the problem. We dup() the descriptor in create_bundle(). So > the patch _is_ necessary and (fairly) correct. But the explanation > probably ought to be something like: > > In create_bundle(), we duplicate the lockfile descriptor via dup(). > This means that even though the lockfile code carefully calls close() > before unlinking the lockfile, we may still have the file open. Unix > systems don't care, but under Windows, this prevents the unlink > (causing an annoying warning and a stale lockfile). > > But that also means that all of the other places we could die (e.g., in > write_or_die) are going to have the same problem. We've fixed only one. > Is there a way we can avoid doing the dup() in the first place? > > The comment there explains that we duplicate because write_pack_data() > will close the descriptor. Unfortunately, that's hard to change because > it comes from run-command. But we don't actually need the descriptor > ourselves after it's closed; we're just trying to appease the lockfile > code; see e54c347c1c (create_bundle(): duplicate file descriptor to > avoid closing it twice, 2015-08-10). > > We just need some reasonable way of telling the lock code what's > happening. Something like the patch below, which is a moral revert of > e54c347c1c, but instead wrapping the "lock->fd = -1" in an official API. > > Does this make your warning go away? > > diff --git a/bundle.c b/bundle.c > [...] I cannot claim that I wrapped my head around your explanation or your diff (I am busy trying to prepare Git for Windows' `master` for rebasing to v2.20.0-rc0), but it does fix the problem. Thank you so much! The line `test_expect_code 1 ...` needs to be adjusted to `test_expect_code 128`, of course, and to discern from the fixed problem (which also exits with code 128), the error output should be verified, like so: -- snip -- test_expect_success 'try to create a bundle with empty ref count' ' test_must_fail git bundle create foobar.bundle master..master 2>err && test_i18ngrep "Refusing to create empty bundle" err ' -- snap -- Do you want to integrate this test into your patch and run with it, or do you want me to shepherd your patch? Ciao, Dscho ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] bundle: cleanup lock files on error 2018-11-15 16:32 ` Johannes Schindelin @ 2018-11-15 16:43 ` Jeff King 2018-11-15 20:01 ` Johannes Schindelin 0 siblings, 1 reply; 20+ messages in thread From: Jeff King @ 2018-11-15 16:43 UTC (permalink / raw) To: Johannes Schindelin Cc: Stefan Beller, Martin Ågren, gitgitgadget, git, Junio C Hamano, Gaël Lhez On Thu, Nov 15, 2018 at 05:32:15PM +0100, Johannes Schindelin wrote: > I cannot claim that I wrapped my head around your explanation or your diff (I am > busy trying to prepare Git for Windows' `master` for rebasing to v2.20.0-rc0), > but it does fix the problem. Thank you so much! > > The line `test_expect_code 1 ...` needs to be adjusted to `test_expect_code > 128`, of course, and to discern from the fixed problem (which also exits with > code 128), the error output should be verified, like so: > > -- snip -- > test_expect_success 'try to create a bundle with empty ref count' ' > test_must_fail git bundle create foobar.bundle master..master 2>err && > test_i18ngrep "Refusing to create empty bundle" err > ' > -- snap -- It seems like we should be checking that the stale lockfile isn't left, which is the real problem (the warning is annoying, but is a symptom of the same thing). I.e., something like: test_must_fail git bundle create foobar.bundle master..master && test_path_is_missing foobar.bundle.lock That would already pass on non-Windows platforms, but that's OK. It will never give a false failure. If you don't mind, can you confirm that the test above fails without either of the two patches under discussion? > Do you want to integrate this test into your patch and run with it, or do you > want me to shepherd your patch? I'll wrap it up with a commit message and a test. -Peff ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] bundle: cleanup lock files on error 2018-11-15 16:43 ` Jeff King @ 2018-11-15 20:01 ` Johannes Schindelin 2018-11-16 9:43 ` [PATCH] bundle: dup() output descriptor closer to point-of-use Jeff King 0 siblings, 1 reply; 20+ messages in thread From: Johannes Schindelin @ 2018-11-15 20:01 UTC (permalink / raw) To: Jeff King Cc: Stefan Beller, Martin Ågren, gitgitgadget, git, Junio C Hamano, Gaël Lhez [-- Attachment #1: Type: text/plain, Size: 1707 bytes --] Hi Peff, On Thu, 15 Nov 2018, Jeff King wrote: > On Thu, Nov 15, 2018 at 05:32:15PM +0100, Johannes Schindelin wrote: > > > I cannot claim that I wrapped my head around your explanation or your > > diff (I am busy trying to prepare Git for Windows' `master` for > > rebasing to v2.20.0-rc0), but it does fix the problem. Thank you so > > much! > > > > The line `test_expect_code 1 ...` needs to be adjusted to > > `test_expect_code 128`, of course, and to discern from the fixed > > problem (which also exits with code 128), the error output should be > > verified, like so: > > > > -- snip -- > > test_expect_success 'try to create a bundle with empty ref count' ' > > test_must_fail git bundle create foobar.bundle master..master 2>err && > > test_i18ngrep "Refusing to create empty bundle" err > > ' > > -- snap -- > > It seems like we should be checking that the stale lockfile isn't left, > which is the real problem (the warning is annoying, but is a symptom of > the same thing). I.e., something like: > > test_must_fail git bundle create foobar.bundle master..master && > test_path_is_missing foobar.bundle.lock > > That would already pass on non-Windows platforms, but that's OK. It will > never give a false failure. > > If you don't mind, can you confirm that the test above fails without > either of the two patches under discussion? This test succeeds with your patch as well as with Gaël's, and fails when neither patch is applied. So you're right, it is the much better test. > > Do you want to integrate this test into your patch and run with it, or > > do you want me to shepherd your patch? > > I'll wrap it up with a commit message and a test. Thank you so much! Dscho ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH] bundle: dup() output descriptor closer to point-of-use 2018-11-15 20:01 ` Johannes Schindelin @ 2018-11-16 9:43 ` Jeff King 2018-11-16 15:06 ` Johannes Schindelin 2018-11-17 7:05 ` Junio C Hamano 0 siblings, 2 replies; 20+ messages in thread From: Jeff King @ 2018-11-16 9:43 UTC (permalink / raw) To: Johannes Schindelin Cc: Stefan Beller, Martin Ågren, gitgitgadget, git, Junio C Hamano, Gaël Lhez On Thu, Nov 15, 2018 at 09:01:07PM +0100, Johannes Schindelin wrote: > > It seems like we should be checking that the stale lockfile isn't left, > > which is the real problem (the warning is annoying, but is a symptom of > > the same thing). I.e., something like: > > > > test_must_fail git bundle create foobar.bundle master..master && > > test_path_is_missing foobar.bundle.lock > > > > That would already pass on non-Windows platforms, but that's OK. It will > > never give a false failure. > > > > If you don't mind, can you confirm that the test above fails without > > either of the two patches under discussion? > > This test succeeds with your patch as well as with Gaël's, and fails when > neither patch is applied. So you're right, it is the much better test. Thanks for checking! > > > Do you want to integrate this test into your patch and run with it, or > > > do you want me to shepherd your patch? > > > > I'll wrap it up with a commit message and a test. Actually, I realized there's an even simpler way to do this. Here it is. -- >8 -- Subject: [PATCH] bundle: dup() output descriptor closer to point-of-use When writing a bundle to a file, the bundle code actually creates "your.bundle.lock" using our lockfile interface. We feed that output descriptor to a child git-pack-objects via run-command, which has the quirk that it closes the output descriptor in the parent. To avoid confusing the lockfile code (which still thinks the descriptor is valid), we dup() it, and operate on the duplicate. However, this has a confusing side effect: after the dup() but before we call pack-objects, we have _two_ descriptors open to the lockfile. If we call die() during that time, the lockfile code will try to clean up the partially-written file. It knows to close() the file before unlinking, since on some platforms (i.e., Windows) the open file would block the deletion. But it doesn't know about the duplicate descriptor. On Windows, triggering an error at the right part of the code will result in the cleanup failing and the lockfile being left in the filesystem. We can solve this by moving the dup() much closer to start_command(), shrinking the window in which we have the second descriptor open. It's easy to place this in such a way that no die() is possible. We could still die due to a signal in the exact wrong moment, but we already tolerate races there (e.g., a signal could come before we manage to put the file on the cleanup list in the first place). As a bonus, this shields create_bundle() itself from the duplicate-fd trick, and we can simplify its error handling (note that the lock rollback now happens unconditionally, but that's OK; it's a noop if we didn't open the lock in the first place). The included test uses an empty bundle to cause a failure at the right spot in the code, because that's easy to trigger (the other likely errors are write() problems like ENOSPC). Note that it would already pass on non-Windows systems (because they are happy to unlink an already-open file). Based-on-a-patch-by: Gaël Lhez <gael.lhez@gmail.com> Signed-off-by: Jeff King <peff@peff.net> --- bundle.c | 39 ++++++++++++++++++--------------------- t/t5607-clone-bundle.sh | 6 ++++++ 2 files changed, 24 insertions(+), 21 deletions(-) diff --git a/bundle.c b/bundle.c index 1ef584b93b..6b0f6d8f10 100644 --- a/bundle.c +++ b/bundle.c @@ -243,7 +243,7 @@ static int is_tag_in_date_range(struct object *tag, struct rev_info *revs) } -/* Write the pack data to bundle_fd, then close it if it is > 1. */ +/* Write the pack data to bundle_fd */ static int write_pack_data(int bundle_fd, struct rev_info *revs) { struct child_process pack_objects = CHILD_PROCESS_INIT; @@ -256,6 +256,20 @@ static int write_pack_data(int bundle_fd, struct rev_info *revs) pack_objects.in = -1; pack_objects.out = bundle_fd; pack_objects.git_cmd = 1; + + /* + * start_command() will close our descriptor if it's >1. Duplicate it + * to avoid surprising the caller. + */ + if (pack_objects.out > 1) { + pack_objects.out = dup(pack_objects.out); + if (pack_objects.out < 0) { + error_errno(_("unable to dup bundle descriptor")); + child_process_clear(&pack_objects); + return -1; + } + } + if (start_command(&pack_objects)) return error(_("Could not spawn pack-objects")); @@ -421,21 +435,10 @@ int create_bundle(struct bundle_header *header, const char *path, bundle_to_stdout = !strcmp(path, "-"); if (bundle_to_stdout) bundle_fd = 1; - else { + else bundle_fd = hold_lock_file_for_update(&lock, path, LOCK_DIE_ON_ERROR); - /* - * write_pack_data() will close the fd passed to it, - * but commit_lock_file() will also try to close the - * lockfile's fd. So make a copy of the file - * descriptor to avoid trying to close it twice. - */ - bundle_fd = dup(bundle_fd); - if (bundle_fd < 0) - die_errno("unable to dup file descriptor"); - } - /* write signature */ write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature)); @@ -463,10 +466,8 @@ int create_bundle(struct bundle_header *header, const char *path, goto err; /* write pack */ - if (write_pack_data(bundle_fd, &revs)) { - bundle_fd = -1; /* already closed by the above call */ + if (write_pack_data(bundle_fd, &revs)) goto err; - } if (!bundle_to_stdout) { if (commit_lock_file(&lock)) @@ -474,11 +475,7 @@ int create_bundle(struct bundle_header *header, const char *path, } return 0; err: - if (!bundle_to_stdout) { - if (0 <= bundle_fd) - close(bundle_fd); - rollback_lock_file(&lock); - } + rollback_lock_file(&lock); return -1; } diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh index 348d9b3bc7..cf39e9e243 100755 --- a/t/t5607-clone-bundle.sh +++ b/t/t5607-clone-bundle.sh @@ -71,4 +71,10 @@ test_expect_success 'prerequisites with an empty commit message' ' git bundle verify bundle ' +test_expect_success 'failed bundle creation does not leave cruft' ' + # This fails because the bundle would be empty. + test_must_fail git bundle create fail.bundle master..master && + test_path_is_missing fail.bundle.lock +' + test_done -- 2.19.1.1636.gc7a073d580 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] bundle: dup() output descriptor closer to point-of-use 2018-11-16 9:43 ` [PATCH] bundle: dup() output descriptor closer to point-of-use Jeff King @ 2018-11-16 15:06 ` Johannes Schindelin 2018-11-17 7:05 ` Junio C Hamano 1 sibling, 0 replies; 20+ messages in thread From: Johannes Schindelin @ 2018-11-16 15:06 UTC (permalink / raw) To: Jeff King Cc: Stefan Beller, Martin Ågren, gitgitgadget, git, Junio C Hamano, Gaël Lhez [-- Attachment #1: Type: text/plain, Size: 6859 bytes --] Hi Peff, On Fri, 16 Nov 2018, Jeff King wrote: > On Thu, Nov 15, 2018 at 09:01:07PM +0100, Johannes Schindelin wrote: > > > > It seems like we should be checking that the stale lockfile isn't left, > > > which is the real problem (the warning is annoying, but is a symptom of > > > the same thing). I.e., something like: > > > > > > test_must_fail git bundle create foobar.bundle master..master && > > > test_path_is_missing foobar.bundle.lock > > > > > > That would already pass on non-Windows platforms, but that's OK. It will > > > never give a false failure. > > > > > > If you don't mind, can you confirm that the test above fails without > > > either of the two patches under discussion? > > > > This test succeeds with your patch as well as with Gaël's, and fails when > > neither patch is applied. So you're right, it is the much better test. > > Thanks for checking! > > > > > Do you want to integrate this test into your patch and run with it, or > > > > do you want me to shepherd your patch? > > > > > > I'll wrap it up with a commit message and a test. > > Actually, I realized there's an even simpler way to do this. Here it is. > > -- >8 -- > Subject: [PATCH] bundle: dup() output descriptor closer to point-of-use > > When writing a bundle to a file, the bundle code actually creates > "your.bundle.lock" using our lockfile interface. We feed that output > descriptor to a child git-pack-objects via run-command, which has the > quirk that it closes the output descriptor in the parent. > > To avoid confusing the lockfile code (which still thinks the descriptor > is valid), we dup() it, and operate on the duplicate. > > However, this has a confusing side effect: after the dup() but before we > call pack-objects, we have _two_ descriptors open to the lockfile. If we > call die() during that time, the lockfile code will try to clean up the > partially-written file. It knows to close() the file before unlinking, > since on some platforms (i.e., Windows) the open file would block the > deletion. But it doesn't know about the duplicate descriptor. On > Windows, triggering an error at the right part of the code will result > in the cleanup failing and the lockfile being left in the filesystem. > > We can solve this by moving the dup() much closer to start_command(), > shrinking the window in which we have the second descriptor open. It's > easy to place this in such a way that no die() is possible. We could > still die due to a signal in the exact wrong moment, but we already > tolerate races there (e.g., a signal could come before we manage to put > the file on the cleanup list in the first place). > > As a bonus, this shields create_bundle() itself from the duplicate-fd > trick, and we can simplify its error handling (note that the lock > rollback now happens unconditionally, but that's OK; it's a noop if we > didn't open the lock in the first place). > > The included test uses an empty bundle to cause a failure at the right > spot in the code, because that's easy to trigger (the other likely > errors are write() problems like ENOSPC). Note that it would already > pass on non-Windows systems (because they are happy to unlink an > already-open file). Thanks, this is a very nice explanation (and now that I do not feel so stressed as I did yesterday, I can easily wrap my head around it). I can confirm that the test fails without the changes to bundle.c, and succeeds with the changes. Thank you so much! Dscho > Based-on-a-patch-by: Gaël Lhez <gael.lhez@gmail.com> > Signed-off-by: Jeff King <peff@peff.net> > --- > bundle.c | 39 ++++++++++++++++++--------------------- > t/t5607-clone-bundle.sh | 6 ++++++ > 2 files changed, 24 insertions(+), 21 deletions(-) > > diff --git a/bundle.c b/bundle.c > index 1ef584b93b..6b0f6d8f10 100644 > --- a/bundle.c > +++ b/bundle.c > @@ -243,7 +243,7 @@ static int is_tag_in_date_range(struct object *tag, struct rev_info *revs) > } > > > -/* Write the pack data to bundle_fd, then close it if it is > 1. */ > +/* Write the pack data to bundle_fd */ > static int write_pack_data(int bundle_fd, struct rev_info *revs) > { > struct child_process pack_objects = CHILD_PROCESS_INIT; > @@ -256,6 +256,20 @@ static int write_pack_data(int bundle_fd, struct rev_info *revs) > pack_objects.in = -1; > pack_objects.out = bundle_fd; > pack_objects.git_cmd = 1; > + > + /* > + * start_command() will close our descriptor if it's >1. Duplicate it > + * to avoid surprising the caller. > + */ > + if (pack_objects.out > 1) { > + pack_objects.out = dup(pack_objects.out); > + if (pack_objects.out < 0) { > + error_errno(_("unable to dup bundle descriptor")); > + child_process_clear(&pack_objects); > + return -1; > + } > + } > + > if (start_command(&pack_objects)) > return error(_("Could not spawn pack-objects")); > > @@ -421,21 +435,10 @@ int create_bundle(struct bundle_header *header, const char *path, > bundle_to_stdout = !strcmp(path, "-"); > if (bundle_to_stdout) > bundle_fd = 1; > - else { > + else > bundle_fd = hold_lock_file_for_update(&lock, path, > LOCK_DIE_ON_ERROR); > > - /* > - * write_pack_data() will close the fd passed to it, > - * but commit_lock_file() will also try to close the > - * lockfile's fd. So make a copy of the file > - * descriptor to avoid trying to close it twice. > - */ > - bundle_fd = dup(bundle_fd); > - if (bundle_fd < 0) > - die_errno("unable to dup file descriptor"); > - } > - > /* write signature */ > write_or_die(bundle_fd, bundle_signature, strlen(bundle_signature)); > > @@ -463,10 +466,8 @@ int create_bundle(struct bundle_header *header, const char *path, > goto err; > > /* write pack */ > - if (write_pack_data(bundle_fd, &revs)) { > - bundle_fd = -1; /* already closed by the above call */ > + if (write_pack_data(bundle_fd, &revs)) > goto err; > - } > > if (!bundle_to_stdout) { > if (commit_lock_file(&lock)) > @@ -474,11 +475,7 @@ int create_bundle(struct bundle_header *header, const char *path, > } > return 0; > err: > - if (!bundle_to_stdout) { > - if (0 <= bundle_fd) > - close(bundle_fd); > - rollback_lock_file(&lock); > - } > + rollback_lock_file(&lock); > return -1; > } > > diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh > index 348d9b3bc7..cf39e9e243 100755 > --- a/t/t5607-clone-bundle.sh > +++ b/t/t5607-clone-bundle.sh > @@ -71,4 +71,10 @@ test_expect_success 'prerequisites with an empty commit message' ' > git bundle verify bundle > ' > > +test_expect_success 'failed bundle creation does not leave cruft' ' > + # This fails because the bundle would be empty. > + test_must_fail git bundle create fail.bundle master..master && > + test_path_is_missing fail.bundle.lock > +' > + > test_done > -- > 2.19.1.1636.gc7a073d580 > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] bundle: dup() output descriptor closer to point-of-use 2018-11-16 9:43 ` [PATCH] bundle: dup() output descriptor closer to point-of-use Jeff King 2018-11-16 15:06 ` Johannes Schindelin @ 2018-11-17 7:05 ` Junio C Hamano 1 sibling, 0 replies; 20+ messages in thread From: Junio C Hamano @ 2018-11-17 7:05 UTC (permalink / raw) To: Jeff King Cc: Johannes Schindelin, Stefan Beller, Martin Ågren, gitgitgadget, git, Gaël Lhez Jeff King <peff@peff.net> writes: > Actually, I realized there's an even simpler way to do this. Here it is. > > -- >8 -- > Subject: [PATCH] bundle: dup() output descriptor closer to point-of-use > > When writing a bundle to a file, the bundle code actually creates > "your.bundle.lock" using our lockfile interface. We feed that output > descriptor to a child git-pack-objects via run-command, which has the > quirk that it closes the output descriptor in the parent. > > To avoid confusing the lockfile code (which still thinks the descriptor > is valid), we dup() it, and operate on the duplicate. Yes... > We can solve this by moving the dup() much closer to start_command(), > shrinking the window in which we have the second descriptor open. It's > easy to place this in such a way that no die() is possible. We could > still die due to a signal in the exact wrong moment, but we already > tolerate races there (e.g., a signal could come before we manage to put > the file on the cleanup list in the first place). > > As a bonus, this shields create_bundle() itself from the duplicate-fd > trick, and we can simplify its error handling (note that the lock > rollback now happens unconditionally, but that's OK; it's a noop if we > didn't open the lock in the first place). ... I found this way too clever for me, but by following the codeflow, it was easy to convince myself that this does the right thing. Almost magical ;-) Will queue, with a Tested-by: with Dscho's name on it. TAhanks, both. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 1/1] bundle: cleanup lock files on error 2018-11-15 12:57 ` Johannes Schindelin 2018-11-15 13:37 ` Jeff King @ 2018-11-15 16:49 ` Duy Nguyen 1 sibling, 0 replies; 20+ messages in thread From: Duy Nguyen @ 2018-11-15 16:49 UTC (permalink / raw) To: Johannes Schindelin Cc: Jeff King, Stefan Beller, Martin Ågren, gitgitgadget, Git Mailing List, Junio C Hamano, gael.lhez On Thu, Nov 15, 2018 at 1:59 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > So yes, we are trying to unlink the `.lock` file, and as far as I can tell that > `unlink()` call comes from the tempfile cleanup asked for by Martin. However, as > we still have a handle open to that file, that call fails. > > I do not think that there is any better way to fix this than to close the file > explicitly. I may be talking nonsense here but I remember someone mentioned something about deleting a file while it's still open on Windows and after a quick internet search, it may be FILE_SHARE_DELETE. Since _we_ open these files, can we just open them with this to be able to delete them? -- Duy ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2018-11-17 7:05 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-13 15:09 [PATCH 0/1] bundle: fix issue when bundles would be empty Johannes Schindelin via GitGitGadget 2018-11-13 15:09 ` [PATCH 1/1] bundle: refuse to create empty bundle Gaël Lhez via GitGitGadget 2018-11-13 19:28 ` Stefan Beller 2018-11-13 20:37 ` Gaël Lhez [not found] ` <CAK8L4uiMHrsdwJz9+rD1tSCywL2kHosx-hKZdS=UtZDHLy464A@mail.gmail.com> 2018-11-13 21:11 ` Stefan Beller 2018-11-14 15:23 ` Johannes Schindelin 2018-11-14 15:25 ` [PATCH v2 0/1] bundle: fix issue when bundles would be empty Johannes Schindelin via GitGitGadget 2018-11-14 15:25 ` [PATCH v2 1/1] bundle: cleanup lock files on error Gaël Lhez via GitGitGadget 2018-11-14 21:43 ` Martin Ågren 2018-11-14 22:08 ` Stefan Beller 2018-11-15 4:34 ` Jeff King 2018-11-15 12:57 ` Johannes Schindelin 2018-11-15 13:37 ` Jeff King 2018-11-15 16:32 ` Johannes Schindelin 2018-11-15 16:43 ` Jeff King 2018-11-15 20:01 ` Johannes Schindelin 2018-11-16 9:43 ` [PATCH] bundle: dup() output descriptor closer to point-of-use Jeff King 2018-11-16 15:06 ` Johannes Schindelin 2018-11-17 7:05 ` Junio C Hamano 2018-11-15 16:49 ` [PATCH v2 1/1] bundle: cleanup lock files on error Duy Nguyen
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).