{"thread":{"id":"49820","subject":"[PATCH 0/1] bundle: fix issue when bundles would be empty","startedAt":"2018-11-13T15:09:39Z","lastAt":"2018-11-17T07:05:51Z","messageCount":20,"participants":["Johannes Schindelin via GitGitGadget","Gaël Lhez via GitGitGadget","Stefan Beller","Gaël Lhez","Johannes Schindelin","Martin Ågren","Jeff King","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"363163","messageId":"pull.79.git.gitgitgadget@gmail.com","threadId":"49820","inReplyTo":null,"subject":"[PATCH 0/1] bundle: fix issue when bundles would be empty","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-13T15:09:36Z","receivedAt":"2018-11-13T15:09:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"And yet another patch coming through Git for Windows...\n\nGaël Lhez (1):\n  bundle: refuse to create empty bundle\n\n bundle.c                | 7 ++++---\n t/t5607-clone-bundle.sh | 4 ++++\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\n\nbase-commit: 8858448bb49332d353febc078ce4a3abcc962efe\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-79%2Fdscho%2Fcreate-empty-bundle-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-79/dscho/create-empty-bundle-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/79\n-- \ngitgitgadget\n"},{"id":"363164","messageId":"6276372ad3dc6fa4b9b352abb2b0192a6d010528.1542121775.git.gitgitgadget@gmail.com","threadId":"49820","inReplyTo":"pull.79.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] bundle: refuse to create empty bundle","fromName":"Gaël Lhez via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-13T15:09:38Z","receivedAt":"2018-11-13T15:09:40Z","isPatch":true,"sender":{"key":"gael.lhez@gmail.com","avatar":null},"body":"From: =?UTF-8?q?Ga=C3=ABl=20Lhez?= <gael.lhez@gmail.com>\n\nWhen an user tries to create an empty bundle via `git bundle create\n<bundle> <revlist>` where `<revlist>` resolves to an empty list (for\nexample, like `master..master`), the command fails and warns the user\nabout how it does not want to create empty bundle.\n\nHowever, the `.lock` file was still open and on Windows that means\nthat it could not be deleted properly. This patch fixes that issue.\n\nThis closes https://github.com/git-for-windows/git/issues/790\n\nSigned-off-by: Gaël Lhez <gael.lhez@gmail.com>\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n bundle.c                | 7 ++++---\n t/t5607-clone-bundle.sh | 4 ++++\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 1ef584b93b..4e349feff9 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -457,10 +457,11 @@ int create_bundle(struct bundle_header *header, const char *path,\n \tobject_array_remove_duplicates(&revs.pending);\n \n \tref_count = write_bundle_refs(bundle_fd, &revs);\n-\tif (!ref_count)\n-\t\tdie(_(\"Refusing to create empty bundle.\"));\n-\telse if (ref_count < 0)\n+\tif (ref_count <= 0)  {\n+\t\tif (!ref_count)\n+\t\t\terror(_(\"Refusing to create empty bundle.\"));\n \t\tgoto err;\n+\t}\n \n \t/* write pack */\n \tif (write_pack_data(bundle_fd, &revs)) {\ndiff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh\nindex 348d9b3bc7..f84b875950 100755\n--- a/t/t5607-clone-bundle.sh\n+++ b/t/t5607-clone-bundle.sh\n@@ -71,4 +71,8 @@ test_expect_success 'prerequisites with an empty commit message' '\n \tgit bundle verify bundle\n '\n \n+test_expect_success 'try to create a bundle with empty ref count' '\n+\ttest_expect_code 1 git bundle create foobar.bundle master..master\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"363203","messageId":"CAGZ79kYR_mQmtOuW7SEZ92K_o0f_C2enJJ1Yt=V3tyvi-A53+A@mail.gmail.com","threadId":"49820","inReplyTo":"6276372ad3dc6fa4b9b352abb2b0192a6d010528.1542121775.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] bundle: refuse to create empty bundle","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-13T19:28:13Z","receivedAt":"2018-11-13T19:28:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Nov 13, 2018 at 7:09 AM Gaël Lhez via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n>\n> From: =?UTF-8?q?Ga=C3=ABl=20Lhez?= <gael.lhez@gmail.com>\n>\n> When an user tries to create an empty bundle via `git bundle create\n> <bundle> <revlist>` where `<revlist>` resolves to an empty list (for\n> example, like `master..master`), the command fails and warns the user\n> about how it does not want to create empty bundle.\n>\n> However, the `.lock` file was still open and on Windows that means\n> that it could not be deleted properly. This patch fixes that issue.\n>\n> This closes https://github.com/git-for-windows/git/issues/790\n>\n> Signed-off-by: Gaël Lhez <gael.lhez@gmail.com>\n> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nThe code and the commit message make sense, but by reading the subject line\n\nI would have expected a different commit. Maybe\n    \"bundle: cleanup lock files on error\"\n"},{"id":"363215","messageId":"CAK8L4uhL4Z7nsZKha59vePHiPR6FtRgbp6WjB8eL8WOcsZRDnQ@mail.gmail.com","threadId":"49820","inReplyTo":"CAGZ79kYR_mQmtOuW7SEZ92K_o0f_C2enJJ1Yt=V3tyvi-A53+A@mail.gmail.com","subject":"Re: [PATCH 1/1] bundle: refuse to create empty bundle","fromName":"Gaël Lhez","fromEmail":"gael.lhez@gmail.com","sentAt":"2018-11-13T20:37:18Z","receivedAt":"2018-11-13T20:37:59Z","isPatch":true,"sender":{"key":"gael.lhez@gmail.com","avatar":null},"body":"Hello,\n\nI don't know why I receive these message (and especially now given the\ntime at which I pushed this) but I suppose someone (Johannes\nSchindelin ?) probably pushed back my original commit from git for\nwindows github to GIT git repository.\n\nIf you think \"bundle: cleanup lock files on error\" is better, then no\nproblem with me. I'm not a native english speaker and I simply\nexpressed the reason for my problem but - after reading back my commit\n- neither this mail' subject and my original commit subject (see\nhttps://github.com/git-for-windows/git/pull/797/commits/0ef742a1a92ae53188189238adbd16086fabf80a)\nexpress the core problem.\n\nAs I'm not accustomed to pushing on GIT 'git repository' , please let\nme know if I have something else to do ?\n\n(Sent back in text mode because HTML is considered SPAM or Virus by\ngit@vger.kernel.org).\n\nRegards,\nGaël\n\nLe mar. 13 nov. 2018 à 20:28, Stefan Beller <sbeller@google.com> a écrit :\n>\n> On Tue, Nov 13, 2018 at 7:09 AM Gaël Lhez via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n> >\n> > From: =?UTF-8?q?Ga=C3=ABl=20Lhez?= <gael.lhez@gmail.com>\n> >\n> > When an user tries to create an empty bundle via `git bundle create\n> > <bundle> <revlist>` where `<revlist>` resolves to an empty list (for\n> > example, like `master..master`), the command fails and warns the user\n> > about how it does not want to create empty bundle.\n> >\n> > However, the `.lock` file was still open and on Windows that means\n> > that it could not be deleted properly. This patch fixes that issue.\n> >\n> > This closes https://github.com/git-for-windows/git/issues/790\n> >\n> > Signed-off-by: Gaël Lhez <gael.lhez@gmail.com>\n> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n>\n> The code and the commit message make sense, but by reading the subject line\n>\n> I would have expected a different commit. Maybe\n>     \"bundle: cleanup lock files on error\"\n"},{"id":"363218","messageId":"CAGZ79ka8tpP7Ph_H8rguomPXoO2J6rDoOKAJTa1b6RSowRwZHQ@mail.gmail.com","threadId":"49820","inReplyTo":"CAK8L4uiMHrsdwJz9+rD1tSCywL2kHosx-hKZdS=UtZDHLy464A@mail.gmail.com","subject":"Re: [PATCH 1/1] bundle: refuse to create empty bundle","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-13T21:11:06Z","receivedAt":"2018-11-13T21:11:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Nov 13, 2018 at 12:33 PM Gaël Lhez <gael.lhez@gmail.com> wrote:\n>\n> Hello,\n>\n> 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.\n\nYes that is pretty much what is happening. Johannes (GfW maintainer)\ntries to push a lot of patches upstream to git and cc's people who\noriginally authored the patch.\nThanks for taking a look, again, even after this long time!\n\n>\n> 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.\n\nI am not a native speaker either, which makes it extra hard to\nunderstand some commits. ;-) So I propose a wording which would have\nhelped me.\n\n> As I'm not accustomed to pushing on GIT 'git repository' , please let me know if I have something else to do ?\n\nI don't know how Johannes handles pushing changes upstream, maybe he\nwill take on the work of resending a reworded patch.\nLet's hear his thoughts on it. I would guess you're more than welcome\nto take your patches from GitForWindows into Git itself.\n\nCheers,\nStefan\n"},{"id":"363370","messageId":"nycvar.QRO.7.76.6.1811141619580.39@tvgsbejvaqbjf.bet","threadId":"49820","inReplyTo":"CAGZ79ka8tpP7Ph_H8rguomPXoO2J6rDoOKAJTa1b6RSowRwZHQ@mail.gmail.com","subject":"Re: [PATCH 1/1] bundle: refuse to create empty bundle","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-14T15:23:05Z","receivedAt":"2018-11-14T15:23:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stefan,\n\nOn Tue, 13 Nov 2018, Stefan Beller wrote:\n\n> On Tue, Nov 13, 2018 at 12:33 PM Gaël Lhez <gael.lhez@gmail.com> wrote:\n> >\n> > Hello,\n> >\n> > 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.\n> \n> Yes that is pretty much what is happening. Johannes (GfW maintainer)\n> tries to push a lot of patches upstream to git and cc's people who\n> originally authored the patch.\n> Thanks for taking a look, again, even after this long time!\n> \n> >\n> > 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.\n> \n> I am not a native speaker either, which makes it extra hard to\n> understand some commits. ;-) So I propose a wording which would have\n> helped me.\n> \n> > As I'm not accustomed to pushing on GIT 'git repository' , please let me know if I have something else to do ?\n> \n> I don't know how Johannes handles pushing changes upstream, maybe he\n> will take on the work of resending a reworded patch.\n\nHe will.\n\n> Let's hear his thoughts on it. I would guess you're more than welcome\n> to take your patches from GitForWindows into Git itself.\n\nAs I merged the patch into Git for Windows' `master`, I consider it my\nresponsibility to push this upstream (unless the contributor wants to take\nmatters into their own hands).\n\nIn the future, my hope is that GitGitGadget will make contributing patches\nto the Git mailing list a more convenient experience, to the point that it\nwill hopefully be pretty much as easy as iterating a PR in\nhttps://github.com/git-for-windows/git. At that point, I will ask\ncontributors to do exactly that in order to shepherd their patches into\ngit.git.\n\nCiao,\nDscho\n\n> \n> Cheers,\n> Stefan\n> "},{"id":"363371","messageId":"pull.79.v2.git.gitgitgadget@gmail.com","threadId":"49820","inReplyTo":"pull.79.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/1] bundle: fix issue when bundles would be empty","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-14T15:25:13Z","receivedAt":"2018-11-14T15:25:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"And yet another patch coming through Git for Windows...\n\nChange since v1:\n\n * Using a better oneline now.\n\nGaël Lhez (1):\n  bundle: cleanup lock files on error\n\n bundle.c                | 7 ++++---\n t/t5607-clone-bundle.sh | 4 ++++\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\n\nbase-commit: 8858448bb49332d353febc078ce4a3abcc962efe\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-79%2Fdscho%2Fcreate-empty-bundle-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-79/dscho/create-empty-bundle-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/79\n\nRange-diff vs v1:\n\n 1:  6276372ad3 ! 1:  c88887f05a bundle: refuse to create empty bundle\n     @@ -1,6 +1,6 @@\n      Author: Gaël Lhez <gael.lhez@gmail.com>\n      \n     -    bundle: refuse to create empty bundle\n     +    bundle: cleanup lock files on error\n      \n          When an user tries to create an empty bundle via `git bundle create\n          <bundle> <revlist>` where `<revlist>` resolves to an empty list (for\n\n-- \ngitgitgadget\n"},{"id":"363372","messageId":"c88887f05a145709be9e86d56f4c1e620eb5ea89.1542209112.git.gitgitgadget@gmail.com","threadId":"49820","inReplyTo":"pull.79.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/1] bundle: cleanup lock files on error","fromName":"Gaël Lhez via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-11-14T15:25:15Z","receivedAt":"2018-11-14T15:25:18Z","isPatch":true,"sender":{"key":"gael.lhez@gmail.com","avatar":null},"body":"From: =?UTF-8?q?Ga=C3=ABl=20Lhez?= <gael.lhez@gmail.com>\n\nWhen an user tries to create an empty bundle via `git bundle create\n<bundle> <revlist>` where `<revlist>` resolves to an empty list (for\nexample, like `master..master`), the command fails and warns the user\nabout how it does not want to create empty bundle.\n\nHowever, the `.lock` file was still open and on Windows that means\nthat it could not be deleted properly. This patch fixes that issue.\n\nThis closes https://github.com/git-for-windows/git/issues/790\n\nSigned-off-by: Gaël Lhez <gael.lhez@gmail.com>\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n bundle.c                | 7 ++++---\n t/t5607-clone-bundle.sh | 4 ++++\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 1ef584b93b..4e349feff9 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -457,10 +457,11 @@ int create_bundle(struct bundle_header *header, const char *path,\n \tobject_array_remove_duplicates(&revs.pending);\n \n \tref_count = write_bundle_refs(bundle_fd, &revs);\n-\tif (!ref_count)\n-\t\tdie(_(\"Refusing to create empty bundle.\"));\n-\telse if (ref_count < 0)\n+\tif (ref_count <= 0)  {\n+\t\tif (!ref_count)\n+\t\t\terror(_(\"Refusing to create empty bundle.\"));\n \t\tgoto err;\n+\t}\n \n \t/* write pack */\n \tif (write_pack_data(bundle_fd, &revs)) {\ndiff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh\nindex 348d9b3bc7..f84b875950 100755\n--- a/t/t5607-clone-bundle.sh\n+++ b/t/t5607-clone-bundle.sh\n@@ -71,4 +71,8 @@ test_expect_success 'prerequisites with an empty commit message' '\n \tgit bundle verify bundle\n '\n \n+test_expect_success 'try to create a bundle with empty ref count' '\n+\ttest_expect_code 1 git bundle create foobar.bundle master..master\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"363398","messageId":"CAN0heSqsjKksKnBHjffVMDEyX4A2DAY6Naw4tbBXEm+AdhOLtA@mail.gmail.com","threadId":"49820","inReplyTo":"c88887f05a145709be9e86d56f4c1e620eb5ea89.1542209112.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/1] bundle: cleanup lock files on error","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-11-14T21:43:32Z","receivedAt":"2018-11-14T21:43:47Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Wed, 14 Nov 2018 at 16:26, Gaël Lhez via GitGitGadget\n<gitgitgadget@gmail.com> wrote:\n> However, the `.lock` file was still open and on Windows that means\n> that it could not be deleted properly. This patch fixes that issue.\n\nHmmm, doesn't the tempfile machinery remove the lock file when we die?\n\n>         ref_count = write_bundle_refs(bundle_fd, &revs);\n> -       if (!ref_count)\n> -               die(_(\"Refusing to create empty bundle.\"));\n> -       else if (ref_count < 0)\n> +       if (ref_count <= 0)  {\n> +               if (!ref_count)\n> +                       error(_(\"Refusing to create empty bundle.\"));\n>                 goto err;\n> +       }\n\nOne less `die()` in libgit -- nice.\n\n> +test_expect_success 'try to create a bundle with empty ref count' '\n> +       test_expect_code 1 git bundle create foobar.bundle master..master\n> +'\n\nThis tries to make sure that we don't just die, but that we exit in a\n\"controlled\" way with exit code 1. After reading the log message, I had\nperhaps expected something like\n\n  test_must_fail git bundle [...] &&\n  test_path_is_missing foobar.bundle.lock\n\nThat relies on magically knowing the \".lock\" suffix, but my point is\nthat for me (on Linux), this alternative test passes both before and\nafter the patch. Before, because tempfile.c cleans up; after, because\nbundle.c does so. Doesn't this happen on Windows too? What am I missing?\n\nMartin\n"},{"id":"363402","messageId":"CAGZ79kZ_wrQ=OdJ6xWbL+F5RDb38YAmzc87o8A=Zb8zBywMJBQ@mail.gmail.com","threadId":"49820","inReplyTo":"CAN0heSqsjKksKnBHjffVMDEyX4A2DAY6Naw4tbBXEm+AdhOLtA@mail.gmail.com","subject":"Re: [PATCH v2 1/1] bundle: cleanup lock files on error","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-11-14T22:08:48Z","receivedAt":"2018-11-14T22:36:51Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Nov 14, 2018 at 1:43 PM Martin Ågren <martin.agren@gmail.com> wrote:\n>\n> On Wed, 14 Nov 2018 at 16:26, Gaël Lhez via GitGitGadget\n> <gitgitgadget@gmail.com> wrote:\n> > However, the `.lock` file was still open and on Windows that means\n> > that it could not be deleted properly. This patch fixes that issue.\n>\n> Hmmm, doesn't the tempfile machinery remove the lock file when we die?\n\nOn Windows this seems not to be the case. (Open files cannot be deleted\nas the open file is not kept by inode or similar but by the file path there?)\n\nRewording your concern: Could the tempfile machinery be taught to\nwork properly on Windows, e.g. by first closing all files and then deleting\nthem afterwards?\n\nThere was a refactoring of tempfiles merged in 89563ec379\n(Merge branch 'jk/incore-lockfile-removal', 2017-09-19), which sounded\npromising at first, as it is dated after the original patch[1] date\n(June 2016), but it has no references for Windows being different,\nso we might still have the original issue; most of the lockfile\ninfrastructure was done in 2015 via db86e61cbb (Merge branch\n'mh/tempfile', 2015-08-25)\n\n[1] https://github.com/git-for-windows/git/pull/797\n"},{"id":"363412","messageId":"20181115043409.GA3419@sigill.intra.peff.net","threadId":"49820","inReplyTo":"CAGZ79kZ_wrQ=OdJ6xWbL+F5RDb38YAmzc87o8A=Zb8zBywMJBQ@mail.gmail.com","subject":"Re: [PATCH v2 1/1] bundle: cleanup lock files on error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-15T04:34:09Z","receivedAt":"2018-11-15T04:34:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 14, 2018 at 02:08:48PM -0800, Stefan Beller wrote:\n\n> On Wed, Nov 14, 2018 at 1:43 PM Martin Ågren <martin.agren@gmail.com> wrote:\n> >\n> > On Wed, 14 Nov 2018 at 16:26, Gaël Lhez via GitGitGadget\n> > <gitgitgadget@gmail.com> wrote:\n> > > However, the `.lock` file was still open and on Windows that means\n> > > that it could not be deleted properly. This patch fixes that issue.\n> >\n> > Hmmm, doesn't the tempfile machinery remove the lock file when we die?\n> \n> On Windows this seems not to be the case. (Open files cannot be deleted\n> as the open file is not kept by inode or similar but by the file path there?)\n> \n> Rewording your concern: Could the tempfile machinery be taught to\n> work properly on Windows, e.g. by first closing all files and then deleting\n> them afterwards?\n\nIt already tries to do so. See delete_tempfile(), or more likely in the\ndie() case, the remove_tempfiles() handler which is called at exit.\n\nAre we sure this is still a problem?\n\nI looked at the test to see if it would pass, but it is not even\nchecking anything about lockfiles! It just checks that we exit 1 by\nreturning up the callstack instead of calling die(). And of course it\nwould not have a problem under Linux either way. But if I run something\nsimilar under strace, I see:\n\n  $ strace ./git bundle create foobar.bundle HEAD..HEAD\n  [...]\n  openat(AT_FDCWD, \"/home/peff/compile/git/foobar.bundle.lock\", O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3\n  [...]\n  close(3)                                = 0\n  unlink(\"/home/peff/compile/git/foobar.bundle.lock\") = 0\n  exit_group(128)                         = ?\n\nwhich seems right.\n\n-Peff\n"},{"id":"363432","messageId":"nycvar.QRO.7.76.6.1811151354150.41@tvgsbejvaqbjf.bet","threadId":"49820","inReplyTo":"20181115043409.GA3419@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/1] bundle: cleanup lock files on error","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-15T12:57:45Z","receivedAt":"2018-11-15T12:58:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 14 Nov 2018, Jeff King wrote:\n\n> On Wed, Nov 14, 2018 at 02:08:48PM -0800, Stefan Beller wrote:\n> \n> > On Wed, Nov 14, 2018 at 1:43 PM Martin Ågren <martin.agren@gmail.com> wrote:\n> > >\n> > > On Wed, 14 Nov 2018 at 16:26, Gaël Lhez via GitGitGadget\n> > > <gitgitgadget@gmail.com> wrote:\n> > > > However, the `.lock` file was still open and on Windows that means\n> > > > that it could not be deleted properly. This patch fixes that issue.\n> > >\n> > > Hmmm, doesn't the tempfile machinery remove the lock file when we die?\n> > \n> > On Windows this seems not to be the case. (Open files cannot be deleted\n> > as the open file is not kept by inode or similar but by the file path there?)\n> > \n> > Rewording your concern: Could the tempfile machinery be taught to\n> > work properly on Windows, e.g. by first closing all files and then deleting\n> > them afterwards?\n> \n> It already tries to do so. See delete_tempfile(), or more likely in the\n> die() case, the remove_tempfiles() handler which is called at exit.\n> \n> Are we sure this is still a problem?\n> \n> I looked at the test to see if it would pass, but it is not even\n> checking anything about lockfiles! It just checks that we exit 1 by\n> returning up the callstack instead of calling die(). And of course it\n> would not have a problem under Linux either way. But if I run something\n> similar under strace, I see:\n> \n>   $ strace ./git bundle create foobar.bundle HEAD..HEAD\n>   [...]\n>   openat(AT_FDCWD, \"/home/peff/compile/git/foobar.bundle.lock\", O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3\n>   [...]\n>   close(3)                                = 0\n>   unlink(\"/home/peff/compile/git/foobar.bundle.lock\") = 0\n>   exit_group(128)                         = ?\n> \n> which seems right.\n\nWithout the fix, the added regression test fails thusly:\n\n-- snip --\n[...]\n++ test_expect_code 1 git bundle create foobar.bundle master..master\n++ want_code=1\n++ shift\n++ git bundle create foobar.bundle master..master\nfatal: Refusing to create empty bundle.\nwarning: unable to unlink 'C:/git-sdk-64/usr/src/git/wip2/t/trash directory.t5607-clone-bundle/foobar.bundle.lock': Permission denied\n++ exit_code=128\n++ test 128 = 1\n++ echo 'test_expect_code: command exited with 128, we wanted 1 git bundle create foobar.bundle master..master'\ntest_expect_code: command exited with 128, we wanted 1 git bundle create foobar.bundle master..master\n++ return 1\nerror: last command exited with $?=1\nnot ok 9 - try to create a bundle with empty ref count\n#\n#               test_expect_code 1 git bundle create foobar.bundle master..master\n#\n-- snap --\n\nSo yes, we are trying to unlink the `.lock` file, and as far as I can tell that\n`unlink()` call comes from the tempfile cleanup asked for by Martin. However, as\nwe still have a handle open to that file, that call fails.\n\nI do not think that there is any better way to fix this than to close the file\nexplicitly. If we tried to just close whatever file descriptor is still open to\nthat file before deleting it, we would possibly cause problems in code that is\nstill to be executed and assumes that it has a perfectly valid file descriptor.\nBesides, trying to do this kind of \"automatically\" won't work, like, at all,\nwhen it is one child process that holds an open file descriptor while another\nprocess wants to delete the file.\n\nCiao,\nDscho"},{"id":"363433","messageId":"20181115133749.GA26164@sigill.intra.peff.net","threadId":"49820","inReplyTo":"nycvar.QRO.7.76.6.1811151354150.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 1/1] bundle: cleanup lock files on error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-15T13:37:49Z","receivedAt":"2018-11-15T13:37:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 15, 2018 at 01:57:45PM +0100, Johannes Schindelin wrote:\n\n> > I looked at the test to see if it would pass, but it is not even\n> > checking anything about lockfiles! It just checks that we exit 1 by\n> > returning up the callstack instead of calling die(). And of course it\n> > would not have a problem under Linux either way. But if I run something\n> > similar under strace, I see:\n> > \n> >   $ strace ./git bundle create foobar.bundle HEAD..HEAD\n> >   [...]\n> >   openat(AT_FDCWD, \"/home/peff/compile/git/foobar.bundle.lock\", O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3\n> >   [...]\n> >   close(3)                                = 0\n> >   unlink(\"/home/peff/compile/git/foobar.bundle.lock\") = 0\n> >   exit_group(128)                         = ?\n> > \n> > which seems right.\n> \n> Without the fix, the added regression test fails thusly:\n> \n> -- snip --\n> [...]\n> ++ test_expect_code 1 git bundle create foobar.bundle master..master\n> ++ want_code=1\n> ++ shift\n> ++ git bundle create foobar.bundle master..master\n> fatal: Refusing to create empty bundle.\n> warning: unable to unlink 'C:/git-sdk-64/usr/src/git/wip2/t/trash directory.t5607-clone-bundle/foobar.bundle.lock': Permission denied\n\nHmph. So who has it open, and why isn't the tempfile code working as\ndesigned?\n\nAha, I see the problem. We dup() the descriptor in create_bundle(). So\nthe patch _is_ necessary and (fairly) correct. But the explanation\nprobably ought to be something like:\n\n  In create_bundle(), we duplicate the lockfile descriptor via dup().\n  This means that even though the lockfile code carefully calls close()\n  before unlinking the lockfile, we may still have the file open. Unix\n  systems don't care, but under Windows, this prevents the unlink\n  (causing an annoying warning and a stale lockfile).\n\nBut that also means that all of the other places we could die (e.g., in\nwrite_or_die) are going to have the same problem. We've fixed only one.\nIs there a way we can avoid doing the dup() in the first place?\n\nThe comment there explains that we duplicate because write_pack_data()\nwill close the descriptor. Unfortunately, that's hard to change because\nit comes from run-command. But we don't actually need the descriptor\nourselves after it's closed; we're just trying to appease the lockfile\ncode; see e54c347c1c (create_bundle(): duplicate file descriptor to\navoid closing it twice, 2015-08-10).\n\nWe just need some reasonable way of telling the lock code what's\nhappening. Something like the patch below, which is a moral revert of\ne54c347c1c, but instead wrapping the \"lock->fd = -1\" in an official API.\n\nDoes this make your warning go away?\n\ndiff --git a/bundle.c b/bundle.c\nindex 1ef584b93b..dc26551b83 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -244,7 +244,7 @@ static int is_tag_in_date_range(struct object *tag, struct rev_info *revs)\n \n \n /* Write the pack data to bundle_fd, then close it if it is > 1. */\n-static int write_pack_data(int bundle_fd, struct rev_info *revs)\n+static int write_pack_data(int bundle_fd, struct lock_file *lock, struct rev_info *revs)\n {\n \tstruct child_process pack_objects = CHILD_PROCESS_INIT;\n \tint i;\n@@ -256,6 +256,14 @@ static int write_pack_data(int bundle_fd, struct rev_info *revs)\n \tpack_objects.in = -1;\n \tpack_objects.out = bundle_fd;\n \tpack_objects.git_cmd = 1;\n+\n+\t/*\n+\t * At this point we know that start_command is going to close our\n+\t * bundle_fd, whether successful or not. Tell the lock code that\n+\t * it is no longer in charge of it, so we don't try to double-close.\n+\t */\n+\tlock_file_release_descriptor(lock);\n+\n \tif (start_command(&pack_objects))\n \t\treturn error(_(\"Could not spawn pack-objects\"));\n \n@@ -421,21 +429,10 @@ int create_bundle(struct bundle_header *header, const char *path,\n \tbundle_to_stdout = !strcmp(path, \"-\");\n \tif (bundle_to_stdout)\n \t\tbundle_fd = 1;\n-\telse {\n+\telse\n \t\tbundle_fd = hold_lock_file_for_update(&lock, path,\n \t\t\t\t\t\t      LOCK_DIE_ON_ERROR);\n \n-\t\t/*\n-\t\t * write_pack_data() will close the fd passed to it,\n-\t\t * but commit_lock_file() will also try to close the\n-\t\t * lockfile's fd. So make a copy of the file\n-\t\t * descriptor to avoid trying to close it twice.\n-\t\t */\n-\t\tbundle_fd = dup(bundle_fd);\n-\t\tif (bundle_fd < 0)\n-\t\t\tdie_errno(\"unable to dup file descriptor\");\n-\t}\n-\n \t/* write signature */\n \twrite_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));\n \n@@ -463,10 +460,8 @@ int create_bundle(struct bundle_header *header, const char *path,\n \t\tgoto err;\n \n \t/* write pack */\n-\tif (write_pack_data(bundle_fd, &revs)) {\n-\t\tbundle_fd = -1; /* already closed by the above call */\n+\tif (write_pack_data(bundle_fd, &lock, &revs))\n \t\tgoto err;\n-\t}\n \n \tif (!bundle_to_stdout) {\n \t\tif (commit_lock_file(&lock))\n@@ -474,11 +469,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n \t}\n \treturn 0;\n err:\n-\tif (!bundle_to_stdout) {\n-\t\tif (0 <= bundle_fd)\n-\t\t\tclose(bundle_fd);\n-\t\trollback_lock_file(&lock);\n-\t}\n+\trollback_lock_file(&lock);\n \treturn -1;\n }\n \ndiff --git a/lockfile.h b/lockfile.h\nindex 35403ccc0d..968cd0e4f6 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -304,4 +304,14 @@ static inline void rollback_lock_file(struct lock_file *lk)\n \tdelete_tempfile(&lk->tempfile);\n }\n \n+/*\n+ * Release the file descriptor and/or file pointer for an open lockfile,\n+ * passing ownership to the caller (who is responsible for closing it). It's a\n+ * NOOP to call this on an inactive or already-closed lockfile.\n+ */\n+static inline void lock_file_release_descriptor(struct lock_file *lk)\n+{\n+\ttempfile_release_descriptor(lk->tempfile);\n+}\n+\n #endif /* LOCKFILE_H */\ndiff --git a/tempfile.c b/tempfile.c\nindex d43ad8c191..59bbdcf4ba 100644\n--- a/tempfile.c\n+++ b/tempfile.c\n@@ -319,3 +319,9 @@ void delete_tempfile(struct tempfile **tempfile_p)\n \tdeactivate_tempfile(tempfile);\n \t*tempfile_p = NULL;\n }\n+\n+void tempfile_release_descriptor(struct tempfile *tempfile)\n+{\n+\ttempfile->fd = -1;\n+\ttempfile->fp = NULL;\n+}\ndiff --git a/tempfile.h b/tempfile.h\nindex 61d8dc4d1b..7489c5e4e9 100644\n--- a/tempfile.h\n+++ b/tempfile.h\n@@ -262,4 +262,11 @@ extern void delete_tempfile(struct tempfile **tempfile_p);\n  */\n extern int rename_tempfile(struct tempfile **tempfile_p, const char *path);\n \n+/*\n+ * Release the file descriptor and/or file pointer for an open tempfile,\n+ * passing ownership to the caller (who is responsible for closing it). It's a\n+ * NOOP to call this on an inactive or already-closed tempfile.\n+ */\n+void tempfile_release_descriptor(struct tempfile *tempfile);\n+\n #endif /* TEMPFILE_H */\n"},{"id":"363440","messageId":"nycvar.QRO.7.76.6.1811151725310.41@tvgsbejvaqbjf.bet","threadId":"49820","inReplyTo":"20181115133749.GA26164@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/1] bundle: cleanup lock files on error","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-15T16:32:15Z","receivedAt":"2018-11-15T16:32:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 15 Nov 2018, Jeff King wrote:\n\n> On Thu, Nov 15, 2018 at 01:57:45PM +0100, Johannes Schindelin wrote:\n> \n> > > I looked at the test to see if it would pass, but it is not even\n> > > checking anything about lockfiles! It just checks that we exit 1 by\n> > > returning up the callstack instead of calling die(). And of course it\n> > > would not have a problem under Linux either way. But if I run something\n> > > similar under strace, I see:\n> > > \n> > >   $ strace ./git bundle create foobar.bundle HEAD..HEAD\n> > >   [...]\n> > >   openat(AT_FDCWD, \"/home/peff/compile/git/foobar.bundle.lock\", O_RDWR|O_CREAT|O_EXCL|O_CLOEXEC, 0666) = 3\n> > >   [...]\n> > >   close(3)                                = 0\n> > >   unlink(\"/home/peff/compile/git/foobar.bundle.lock\") = 0\n> > >   exit_group(128)                         = ?\n> > > \n> > > which seems right.\n> > \n> > Without the fix, the added regression test fails thusly:\n> > \n> > -- snip --\n> > [...]\n> > ++ test_expect_code 1 git bundle create foobar.bundle master..master\n> > ++ want_code=1\n> > ++ shift\n> > ++ git bundle create foobar.bundle master..master\n> > fatal: Refusing to create empty bundle.\n> > warning: unable to unlink 'C:/git-sdk-64/usr/src/git/wip2/t/trash directory.t5607-clone-bundle/foobar.bundle.lock': Permission denied\n> \n> Hmph. So who has it open, and why isn't the tempfile code working as\n> designed?\n> \n> Aha, I see the problem. We dup() the descriptor in create_bundle(). So\n> the patch _is_ necessary and (fairly) correct. But the explanation\n> probably ought to be something like:\n> \n>   In create_bundle(), we duplicate the lockfile descriptor via dup().\n>   This means that even though the lockfile code carefully calls close()\n>   before unlinking the lockfile, we may still have the file open. Unix\n>   systems don't care, but under Windows, this prevents the unlink\n>   (causing an annoying warning and a stale lockfile).\n> \n> But that also means that all of the other places we could die (e.g., in\n> write_or_die) are going to have the same problem. We've fixed only one.\n> Is there a way we can avoid doing the dup() in the first place?\n> \n> The comment there explains that we duplicate because write_pack_data()\n> will close the descriptor. Unfortunately, that's hard to change because\n> it comes from run-command. But we don't actually need the descriptor\n> ourselves after it's closed; we're just trying to appease the lockfile\n> code; see e54c347c1c (create_bundle(): duplicate file descriptor to\n> avoid closing it twice, 2015-08-10).\n> \n> We just need some reasonable way of telling the lock code what's\n> happening. Something like the patch below, which is a moral revert of\n> e54c347c1c, but instead wrapping the \"lock->fd = -1\" in an official API.\n> \n> Does this make your warning go away?\n> \n> diff --git a/bundle.c b/bundle.c\n> [...]\n\nI cannot claim that I wrapped my head around your explanation or your diff (I am\nbusy trying to prepare Git for Windows' `master` for rebasing to v2.20.0-rc0),\nbut it does fix the problem. Thank you so much!\n\nThe line `test_expect_code 1 ...` needs to be adjusted to `test_expect_code\n128`, of course, and to discern from the fixed problem (which also exits with\ncode 128), the error output should be verified, like so:\n\n-- snip --\ntest_expect_success 'try to create a bundle with empty ref count' '\n\ttest_must_fail git bundle create foobar.bundle master..master 2>err &&\n\ttest_i18ngrep \"Refusing to create empty bundle\" err\n'\n-- snap --\n\nDo you want to integrate this test into your patch and run with it, or do you\nwant me to shepherd your patch?\n\nCiao,\nDscho\n"},{"id":"363441","messageId":"20181115164300.GA29290@sigill.intra.peff.net","threadId":"49820","inReplyTo":"nycvar.QRO.7.76.6.1811151725310.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 1/1] bundle: cleanup lock files on error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-15T16:43:01Z","receivedAt":"2018-11-15T16:43:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 15, 2018 at 05:32:15PM +0100, Johannes Schindelin wrote:\n\n> I cannot claim that I wrapped my head around your explanation or your diff (I am\n> busy trying to prepare Git for Windows' `master` for rebasing to v2.20.0-rc0),\n> but it does fix the problem. Thank you so much!\n> \n> The line `test_expect_code 1 ...` needs to be adjusted to `test_expect_code\n> 128`, of course, and to discern from the fixed problem (which also exits with\n> code 128), the error output should be verified, like so:\n> \n> -- snip --\n> test_expect_success 'try to create a bundle with empty ref count' '\n> \ttest_must_fail git bundle create foobar.bundle master..master 2>err &&\n> \ttest_i18ngrep \"Refusing to create empty bundle\" err\n> '\n> -- snap --\n\nIt seems like we should be checking that the stale lockfile isn't left,\nwhich is the real problem (the warning is annoying, but is a symptom of\nthe same thing). I.e., something like:\n\n  test_must_fail git bundle create foobar.bundle master..master &&\n  test_path_is_missing foobar.bundle.lock\n\nThat would already pass on non-Windows platforms, but that's OK. It will\nnever give a false failure.\n\nIf you don't mind, can you confirm that the test above fails without\neither of the two patches under discussion?\n\n> Do you want to integrate this test into your patch and run with it, or do you\n> want me to shepherd your patch?\n\nI'll wrap it up with a commit message and a test.\n\n-Peff\n"},{"id":"363442","messageId":"CACsJy8DZkEb6d=PW4bdkipTSkUSG7YUqk4tkG9PXx+bfngFW1A@mail.gmail.com","threadId":"49820","inReplyTo":"nycvar.QRO.7.76.6.1811151354150.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 1/1] bundle: cleanup lock files on error","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-15T16:49:37Z","receivedAt":"2018-11-15T16:50:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Nov 15, 2018 at 1:59 PM Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n> So yes, we are trying to unlink the `.lock` file, and as far as I can tell that\n> `unlink()` call comes from the tempfile cleanup asked for by Martin. However, as\n> we still have a handle open to that file, that call fails.\n>\n> I do not think that there is any better way to fix this than to close the file\n> explicitly.\n\nI may be talking nonsense here but I remember someone mentioned\nsomething about deleting a file while it's still open on Windows and\nafter a quick internet search, it may be FILE_SHARE_DELETE. Since _we_\nopen these files, can we just open them with this to be able to delete\nthem?\n-- \nDuy\n"},{"id":"363449","messageId":"nycvar.QRO.7.76.6.1811152059060.41@tvgsbejvaqbjf.bet","threadId":"49820","inReplyTo":"20181115164300.GA29290@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/1] bundle: cleanup lock files on error","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-15T20:01:07Z","receivedAt":"2018-11-15T20:01:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 15 Nov 2018, Jeff King wrote:\n\n> On Thu, Nov 15, 2018 at 05:32:15PM +0100, Johannes Schindelin wrote:\n> \n> > I cannot claim that I wrapped my head around your explanation or your\n> > diff (I am busy trying to prepare Git for Windows' `master` for\n> > rebasing to v2.20.0-rc0), but it does fix the problem. Thank you so\n> > much!\n> > \n> > The line `test_expect_code 1 ...` needs to be adjusted to\n> > `test_expect_code 128`, of course, and to discern from the fixed\n> > problem (which also exits with code 128), the error output should be\n> > verified, like so:\n> > \n> > -- snip --\n> > test_expect_success 'try to create a bundle with empty ref count' '\n> > \ttest_must_fail git bundle create foobar.bundle master..master 2>err &&\n> > \ttest_i18ngrep \"Refusing to create empty bundle\" err\n> > '\n> > -- snap --\n> \n> It seems like we should be checking that the stale lockfile isn't left,\n> which is the real problem (the warning is annoying, but is a symptom of\n> the same thing). I.e., something like:\n> \n>   test_must_fail git bundle create foobar.bundle master..master &&\n>   test_path_is_missing foobar.bundle.lock\n> \n> That would already pass on non-Windows platforms, but that's OK. It will\n> never give a false failure.\n> \n> If you don't mind, can you confirm that the test above fails without\n> either of the two patches under discussion?\n\nThis test succeeds with your patch as well as with Gaël's, and fails when\nneither patch is applied. So you're right, it is the much better test.\n\n> > Do you want to integrate this test into your patch and run with it, or\n> > do you want me to shepherd your patch?\n> \n> I'll wrap it up with a commit message and a test.\n\nThank you so much!\nDscho"},{"id":"363490","messageId":"20181116094358.GA6054@sigill.intra.peff.net","threadId":"49820","inReplyTo":"nycvar.QRO.7.76.6.1811152059060.41@tvgsbejvaqbjf.bet","subject":"[PATCH] bundle: dup() output descriptor closer to point-of-use","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-16T09:43:59Z","receivedAt":"2018-11-16T09:44:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 15, 2018 at 09:01:07PM +0100, Johannes Schindelin wrote:\n\n> > It seems like we should be checking that the stale lockfile isn't left,\n> > which is the real problem (the warning is annoying, but is a symptom of\n> > the same thing). I.e., something like:\n> > \n> >   test_must_fail git bundle create foobar.bundle master..master &&\n> >   test_path_is_missing foobar.bundle.lock\n> > \n> > That would already pass on non-Windows platforms, but that's OK. It will\n> > never give a false failure.\n> > \n> > If you don't mind, can you confirm that the test above fails without\n> > either of the two patches under discussion?\n> \n> This test succeeds with your patch as well as with Gaël's, and fails when\n> neither patch is applied. So you're right, it is the much better test.\n\nThanks for checking!\n\n> > > Do you want to integrate this test into your patch and run with it, or\n> > > do you want me to shepherd your patch?\n> > \n> > I'll wrap it up with a commit message and a test.\n\nActually, I realized there's an even simpler way to do this. Here it is.\n\n-- >8 --\nSubject: [PATCH] bundle: dup() output descriptor closer to point-of-use\n\nWhen writing a bundle to a file, the bundle code actually creates\n\"your.bundle.lock\" using our lockfile interface. We feed that output\ndescriptor to a child git-pack-objects via run-command, which has the\nquirk that it closes the output descriptor in the parent.\n\nTo avoid confusing the lockfile code (which still thinks the descriptor\nis valid), we dup() it, and operate on the duplicate.\n\nHowever, this has a confusing side effect: after the dup() but before we\ncall pack-objects, we have _two_ descriptors open to the lockfile. If we\ncall die() during that time, the lockfile code will try to clean up the\npartially-written file. It knows to close() the file before unlinking,\nsince on some platforms (i.e., Windows) the open file would block the\ndeletion. But it doesn't know about the duplicate descriptor. On\nWindows, triggering an error at the right part of the code will result\nin the cleanup failing and the lockfile being left in the filesystem.\n\nWe can solve this by moving the dup() much closer to start_command(),\nshrinking the window in which we have the second descriptor open. It's\neasy to place this in such a way that no die() is possible. We could\nstill die due to a signal in the exact wrong moment, but we already\ntolerate races there (e.g., a signal could come before we manage to put\nthe file on the cleanup list in the first place).\n\nAs a bonus, this shields create_bundle() itself from the duplicate-fd\ntrick, and we can simplify its error handling (note that the lock\nrollback now happens unconditionally, but that's OK; it's a noop if we\ndidn't open the lock in the first place).\n\nThe included test uses an empty bundle to cause a failure at the right\nspot in the code, because that's easy to trigger (the other likely\nerrors are write() problems like ENOSPC).  Note that it would already\npass on non-Windows systems (because they are happy to unlink an\nalready-open file).\n\nBased-on-a-patch-by: Gaël Lhez <gael.lhez@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n bundle.c                | 39 ++++++++++++++++++---------------------\n t/t5607-clone-bundle.sh |  6 ++++++\n 2 files changed, 24 insertions(+), 21 deletions(-)\n\ndiff --git a/bundle.c b/bundle.c\nindex 1ef584b93b..6b0f6d8f10 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -243,7 +243,7 @@ static int is_tag_in_date_range(struct object *tag, struct rev_info *revs)\n }\n \n \n-/* Write the pack data to bundle_fd, then close it if it is > 1. */\n+/* Write the pack data to bundle_fd */\n static int write_pack_data(int bundle_fd, struct rev_info *revs)\n {\n \tstruct child_process pack_objects = CHILD_PROCESS_INIT;\n@@ -256,6 +256,20 @@ static int write_pack_data(int bundle_fd, struct rev_info *revs)\n \tpack_objects.in = -1;\n \tpack_objects.out = bundle_fd;\n \tpack_objects.git_cmd = 1;\n+\n+\t/*\n+\t * start_command() will close our descriptor if it's >1. Duplicate it\n+\t * to avoid surprising the caller.\n+\t */\n+\tif (pack_objects.out > 1) {\n+\t\tpack_objects.out = dup(pack_objects.out);\n+\t\tif (pack_objects.out < 0) {\n+\t\t\terror_errno(_(\"unable to dup bundle descriptor\"));\n+\t\t\tchild_process_clear(&pack_objects);\n+\t\t\treturn -1;\n+\t\t}\n+\t}\n+\n \tif (start_command(&pack_objects))\n \t\treturn error(_(\"Could not spawn pack-objects\"));\n \n@@ -421,21 +435,10 @@ int create_bundle(struct bundle_header *header, const char *path,\n \tbundle_to_stdout = !strcmp(path, \"-\");\n \tif (bundle_to_stdout)\n \t\tbundle_fd = 1;\n-\telse {\n+\telse\n \t\tbundle_fd = hold_lock_file_for_update(&lock, path,\n \t\t\t\t\t\t      LOCK_DIE_ON_ERROR);\n \n-\t\t/*\n-\t\t * write_pack_data() will close the fd passed to it,\n-\t\t * but commit_lock_file() will also try to close the\n-\t\t * lockfile's fd. So make a copy of the file\n-\t\t * descriptor to avoid trying to close it twice.\n-\t\t */\n-\t\tbundle_fd = dup(bundle_fd);\n-\t\tif (bundle_fd < 0)\n-\t\t\tdie_errno(\"unable to dup file descriptor\");\n-\t}\n-\n \t/* write signature */\n \twrite_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));\n \n@@ -463,10 +466,8 @@ int create_bundle(struct bundle_header *header, const char *path,\n \t\tgoto err;\n \n \t/* write pack */\n-\tif (write_pack_data(bundle_fd, &revs)) {\n-\t\tbundle_fd = -1; /* already closed by the above call */\n+\tif (write_pack_data(bundle_fd, &revs))\n \t\tgoto err;\n-\t}\n \n \tif (!bundle_to_stdout) {\n \t\tif (commit_lock_file(&lock))\n@@ -474,11 +475,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n \t}\n \treturn 0;\n err:\n-\tif (!bundle_to_stdout) {\n-\t\tif (0 <= bundle_fd)\n-\t\t\tclose(bundle_fd);\n-\t\trollback_lock_file(&lock);\n-\t}\n+\trollback_lock_file(&lock);\n \treturn -1;\n }\n \ndiff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh\nindex 348d9b3bc7..cf39e9e243 100755\n--- a/t/t5607-clone-bundle.sh\n+++ b/t/t5607-clone-bundle.sh\n@@ -71,4 +71,10 @@ test_expect_success 'prerequisites with an empty commit message' '\n \tgit bundle verify bundle\n '\n \n+test_expect_success 'failed bundle creation does not leave cruft' '\n+\t# This fails because the bundle would be empty.\n+\ttest_must_fail git bundle create fail.bundle master..master &&\n+\ttest_path_is_missing fail.bundle.lock\n+'\n+\n test_done\n-- \n2.19.1.1636.gc7a073d580\n\n"},{"id":"363513","messageId":"nycvar.QRO.7.76.6.1811161557070.41@tvgsbejvaqbjf.bet","threadId":"49820","inReplyTo":"20181116094358.GA6054@sigill.intra.peff.net","subject":"Re: [PATCH] bundle: dup() output descriptor closer to point-of-use","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-11-16T15:06:14Z","receivedAt":"2018-11-16T15:06:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 16 Nov 2018, Jeff King wrote:\n\n> On Thu, Nov 15, 2018 at 09:01:07PM +0100, Johannes Schindelin wrote:\n> \n> > > It seems like we should be checking that the stale lockfile isn't left,\n> > > which is the real problem (the warning is annoying, but is a symptom of\n> > > the same thing). I.e., something like:\n> > > \n> > >   test_must_fail git bundle create foobar.bundle master..master &&\n> > >   test_path_is_missing foobar.bundle.lock\n> > > \n> > > That would already pass on non-Windows platforms, but that's OK. It will\n> > > never give a false failure.\n> > > \n> > > If you don't mind, can you confirm that the test above fails without\n> > > either of the two patches under discussion?\n> > \n> > This test succeeds with your patch as well as with Gaël's, and fails when\n> > neither patch is applied. So you're right, it is the much better test.\n> \n> Thanks for checking!\n> \n> > > > Do you want to integrate this test into your patch and run with it, or\n> > > > do you want me to shepherd your patch?\n> > > \n> > > I'll wrap it up with a commit message and a test.\n> \n> Actually, I realized there's an even simpler way to do this. Here it is.\n> \n> -- >8 --\n> Subject: [PATCH] bundle: dup() output descriptor closer to point-of-use\n> \n> When writing a bundle to a file, the bundle code actually creates\n> \"your.bundle.lock\" using our lockfile interface. We feed that output\n> descriptor to a child git-pack-objects via run-command, which has the\n> quirk that it closes the output descriptor in the parent.\n> \n> To avoid confusing the lockfile code (which still thinks the descriptor\n> is valid), we dup() it, and operate on the duplicate.\n> \n> However, this has a confusing side effect: after the dup() but before we\n> call pack-objects, we have _two_ descriptors open to the lockfile. If we\n> call die() during that time, the lockfile code will try to clean up the\n> partially-written file. It knows to close() the file before unlinking,\n> since on some platforms (i.e., Windows) the open file would block the\n> deletion. But it doesn't know about the duplicate descriptor. On\n> Windows, triggering an error at the right part of the code will result\n> in the cleanup failing and the lockfile being left in the filesystem.\n> \n> We can solve this by moving the dup() much closer to start_command(),\n> shrinking the window in which we have the second descriptor open. It's\n> easy to place this in such a way that no die() is possible. We could\n> still die due to a signal in the exact wrong moment, but we already\n> tolerate races there (e.g., a signal could come before we manage to put\n> the file on the cleanup list in the first place).\n> \n> As a bonus, this shields create_bundle() itself from the duplicate-fd\n> trick, and we can simplify its error handling (note that the lock\n> rollback now happens unconditionally, but that's OK; it's a noop if we\n> didn't open the lock in the first place).\n> \n> The included test uses an empty bundle to cause a failure at the right\n> spot in the code, because that's easy to trigger (the other likely\n> errors are write() problems like ENOSPC).  Note that it would already\n> pass on non-Windows systems (because they are happy to unlink an\n> already-open file).\n\nThanks, this is a very nice explanation (and now that I do not feel so\nstressed as I did yesterday, I can easily wrap my head around it).\n\nI can confirm that the test fails without the changes to bundle.c, and\nsucceeds with the changes.\n\nThank you so much!\nDscho\n\n> Based-on-a-patch-by: Gaël Lhez <gael.lhez@gmail.com>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  bundle.c                | 39 ++++++++++++++++++---------------------\n>  t/t5607-clone-bundle.sh |  6 ++++++\n>  2 files changed, 24 insertions(+), 21 deletions(-)\n> \n> diff --git a/bundle.c b/bundle.c\n> index 1ef584b93b..6b0f6d8f10 100644\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -243,7 +243,7 @@ static int is_tag_in_date_range(struct object *tag, struct rev_info *revs)\n>  }\n>  \n>  \n> -/* Write the pack data to bundle_fd, then close it if it is > 1. */\n> +/* Write the pack data to bundle_fd */\n>  static int write_pack_data(int bundle_fd, struct rev_info *revs)\n>  {\n>  \tstruct child_process pack_objects = CHILD_PROCESS_INIT;\n> @@ -256,6 +256,20 @@ static int write_pack_data(int bundle_fd, struct rev_info *revs)\n>  \tpack_objects.in = -1;\n>  \tpack_objects.out = bundle_fd;\n>  \tpack_objects.git_cmd = 1;\n> +\n> +\t/*\n> +\t * start_command() will close our descriptor if it's >1. Duplicate it\n> +\t * to avoid surprising the caller.\n> +\t */\n> +\tif (pack_objects.out > 1) {\n> +\t\tpack_objects.out = dup(pack_objects.out);\n> +\t\tif (pack_objects.out < 0) {\n> +\t\t\terror_errno(_(\"unable to dup bundle descriptor\"));\n> +\t\t\tchild_process_clear(&pack_objects);\n> +\t\t\treturn -1;\n> +\t\t}\n> +\t}\n> +\n>  \tif (start_command(&pack_objects))\n>  \t\treturn error(_(\"Could not spawn pack-objects\"));\n>  \n> @@ -421,21 +435,10 @@ int create_bundle(struct bundle_header *header, const char *path,\n>  \tbundle_to_stdout = !strcmp(path, \"-\");\n>  \tif (bundle_to_stdout)\n>  \t\tbundle_fd = 1;\n> -\telse {\n> +\telse\n>  \t\tbundle_fd = hold_lock_file_for_update(&lock, path,\n>  \t\t\t\t\t\t      LOCK_DIE_ON_ERROR);\n>  \n> -\t\t/*\n> -\t\t * write_pack_data() will close the fd passed to it,\n> -\t\t * but commit_lock_file() will also try to close the\n> -\t\t * lockfile's fd. So make a copy of the file\n> -\t\t * descriptor to avoid trying to close it twice.\n> -\t\t */\n> -\t\tbundle_fd = dup(bundle_fd);\n> -\t\tif (bundle_fd < 0)\n> -\t\t\tdie_errno(\"unable to dup file descriptor\");\n> -\t}\n> -\n>  \t/* write signature */\n>  \twrite_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));\n>  \n> @@ -463,10 +466,8 @@ int create_bundle(struct bundle_header *header, const char *path,\n>  \t\tgoto err;\n>  \n>  \t/* write pack */\n> -\tif (write_pack_data(bundle_fd, &revs)) {\n> -\t\tbundle_fd = -1; /* already closed by the above call */\n> +\tif (write_pack_data(bundle_fd, &revs))\n>  \t\tgoto err;\n> -\t}\n>  \n>  \tif (!bundle_to_stdout) {\n>  \t\tif (commit_lock_file(&lock))\n> @@ -474,11 +475,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n>  \t}\n>  \treturn 0;\n>  err:\n> -\tif (!bundle_to_stdout) {\n> -\t\tif (0 <= bundle_fd)\n> -\t\t\tclose(bundle_fd);\n> -\t\trollback_lock_file(&lock);\n> -\t}\n> +\trollback_lock_file(&lock);\n>  \treturn -1;\n>  }\n>  \n> diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh\n> index 348d9b3bc7..cf39e9e243 100755\n> --- a/t/t5607-clone-bundle.sh\n> +++ b/t/t5607-clone-bundle.sh\n> @@ -71,4 +71,10 @@ test_expect_success 'prerequisites with an empty commit message' '\n>  \tgit bundle verify bundle\n>  '\n>  \n> +test_expect_success 'failed bundle creation does not leave cruft' '\n> +\t# This fails because the bundle would be empty.\n> +\ttest_must_fail git bundle create fail.bundle master..master &&\n> +\ttest_path_is_missing fail.bundle.lock\n> +'\n> +\n>  test_done\n> -- \n> 2.19.1.1636.gc7a073d580\n> \n> "},{"id":"363543","messageId":"xmqqk1lci3p3.fsf@gitster-ct.c.googlers.com","threadId":"49820","inReplyTo":"20181116094358.GA6054@sigill.intra.peff.net","subject":"Re: [PATCH] bundle: dup() output descriptor closer to point-of-use","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-17T07:05:44Z","receivedAt":"2018-11-17T07:05:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Actually, I realized there's an even simpler way to do this. Here it is.\n>\n> -- >8 --\n> Subject: [PATCH] bundle: dup() output descriptor closer to point-of-use\n>\n> When writing a bundle to a file, the bundle code actually creates\n> \"your.bundle.lock\" using our lockfile interface. We feed that output\n> descriptor to a child git-pack-objects via run-command, which has the\n> quirk that it closes the output descriptor in the parent.\n>\n> To avoid confusing the lockfile code (which still thinks the descriptor\n> is valid), we dup() it, and operate on the duplicate.\n\nYes...\n\n> We can solve this by moving the dup() much closer to start_command(),\n> shrinking the window in which we have the second descriptor open. It's\n> easy to place this in such a way that no die() is possible. We could\n> still die due to a signal in the exact wrong moment, but we already\n> tolerate races there (e.g., a signal could come before we manage to put\n> the file on the cleanup list in the first place).\n>\n> As a bonus, this shields create_bundle() itself from the duplicate-fd\n> trick, and we can simplify its error handling (note that the lock\n> rollback now happens unconditionally, but that's OK; it's a noop if we\n> didn't open the lock in the first place).\n\n... I found this way too clever for me, but by following the\ncodeflow, it was easy to convince myself that this does the right\nthing.  Almost magical ;-)\n\nWill queue, with a Tested-by: with Dscho's name on it.\n\nTAhanks, both.\n"}]}