{"thread":{"id":"16389","subject":"[PATCH] Fix handle leak in builtin-pack-objects","startedAt":"2008-11-19T11:13:06Z","lastAt":"2008-12-10T09:36:05Z","messageCount":23,"participants":["Alex Riesen","Johannes Sixt","Nicolas Pitre","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"96134","messageId":"81b0412b0811190313p643c0cb4vad620ea942aeea93@mail.gmail.com","threadId":"16389","inReplyTo":null,"subject":"[PATCH] Fix handle leak in builtin-pack-objects","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-11-19T11:13:06Z","receivedAt":"2008-11-19T11:13:06Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"The opened packs seem to stay open forever.\n\nSigned-off-by: Alex Riesen \n---\n\nI'm very unsure about the solution, though: it is really horrible code\nto debug...\n\n builtin-pack-objects.c | 1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\n\nFrom d0fbfd05e5c1f41f58281d85d68c705c5f036e7d Mon Sep 17 00:00:00 2001\nFrom: Alex Riesen \nDate: Wed, 19 Nov 2008 11:04:48 +0100\nSubject: [PATCH] Fix handle leak in builtin-pack-objects\n\nThe opened packs seem to stay open forever.\n\nSigned-off-by: Alex Riesen \n---\n builtin-pack-objects.c | 1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 67eefa2..e68e997 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -532,6 +532,7 @@ static void write_pack_file(void)\n \n \t\t\tidx_tmp_name = write_idx_file(NULL, written_list,\n \t\t\t\t\t\t nr_written, sha1);\n+\t\t\trelease_pack_memory(-1, -1);\n \n \t\t\tsnprintf(tmpname, sizeof(tmpname), \"%s-%s.pack\",\n \t\t\t\t base_name, sha1_to_hex(sha1));\n-- \n1.6.0.4.644.gb619a\n\n"},{"id":"96139","messageId":"4923FE58.3090503@viscovery.net","threadId":"16389","inReplyTo":"81b0412b0811190313p643c0cb4vad620ea942aeea93@mail.gmail.com","subject":"Re: [PATCH] Fix handle leak in builtin-pack-objects","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-11-19T11:54:00Z","receivedAt":"2008-11-19T11:54:00Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Alex Riesen schrieb:\n> The opened packs seem to stay open forever.\n\nIn my MinGW port I have the patch below that avoids that t5303 fails\nbecause of a pack file that remains open. (Open files cannot be replaced\non Windows.) I had hoped that your patch would help, but it does not.\nSomething else still keeps the pack file open. Can anything be done about\nthat?\n\n-- Hannes\n\nFrom: Johannes Sixt \nDate: Mon, 17 Nov 2008 09:25:19 +0100\nSubject: [PATCH] t5303: Do not overwrite an existing pack\n\nThis test corrupts a pack file, then repacks the objects. The consequence\nis that the repacked pack file has the same name as the original file\n(that has been corrupted).\n\nDuring its operation, git-pack-objects opens the corrupted file and keeps\nit open at all times. On Windows, this is a problem because a file that is\nopen in any process cannot be delete or replaced, but that is what we do\nin some of the test cases, and so they fail.\n\nThe work-around is to write the repacked objects to a file of a different\nname, and replace the original after git-pack-objects has terminated.\n\nSigned-off-by: Johannes Sixt \n---\n t/t5303-pack-corruption-resilience.sh | 7 +++++--\n 1 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5303-pack-corruption-resilience.sh\nb/t/t5303-pack-corruption-resilience.sh\nindex 5132d41..41c83e3 100755\n--- a/t/t5303-pack-corruption-resilience.sh\n+++ b/t/t5303-pack-corruption-resilience.sh\n@@ -43,8 +43,11 @@ create_new_pack() {\n\n do_repack() {\n pack=`printf \"$blob_1\\n$blob_2\\n$blob_3\\n\" |\n- git pack-objects $@ .git/objects/pack/pack` &&\n- pack=\".git/objects/pack/pack-${pack}\"\n+ git pack-objects $@ .git/objects/pack/packtmp` &&\n+ packtmp=\".git/objects/pack/packtmp-${pack}\" &&\n+ pack=\".git/objects/pack/pack-${pack}\" &&\n+ mv \"${packtmp}.pack\" \"${pack}.pack\" &&\n+ mv \"${packtmp}.idx\" \"${pack}.idx\"\n }\n\n do_corrupt_object() {\n-- \n1.6.0.4.1683.g35125\n"},{"id":"96141","messageId":"81b0412b0811190413s29644092y86d1c8840e10c28b@mail.gmail.com","threadId":"16389","inReplyTo":"4923FE58.3090503@viscovery.net","subject":"Re: [PATCH] Fix handle leak in builtin-pack-objects","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-11-19T12:13:46Z","receivedAt":"2008-11-19T12:13:46Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2008/11/19 Johannes Sixt :\n> Alex Riesen schrieb:\n>> The opened packs seem to stay open forever.\n>\n> In my MinGW port I have the patch below that avoids that t5303 fails\n> because of a pack file that remains open. (Open files cannot be replaced\n> on Windows.) I had hoped that your patch would help, but it does not.\n> Something else still keeps the pack file open. Can anything be done about\n> that?\n>\n\nDo this _and_ the other handle-leak-patch together help?\n(I think the second should)\n\n> From: Johannes Sixt \n> Date: Mon, 17 Nov 2008 09:25:19 +0100\n\nBTW, you better report such things. Even though it is typical\nfor this platform brokenness, leaking open files is never good.\n"},{"id":"96144","messageId":"492405DF.5060600@viscovery.net","threadId":"16389","inReplyTo":"81b0412b0811190413s29644092y86d1c8840e10c28b@mail.gmail.com","subject":"Re: [PATCH] Fix handle leak in builtin-pack-objects","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-11-19T12:26:07Z","receivedAt":"2008-11-19T12:26:07Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Alex Riesen schrieb:\n> 2008/11/19 Johannes Sixt :\n>> Alex Riesen schrieb:\n>>> The opened packs seem to stay open forever.\n>> In my MinGW port I have the patch below that avoids that t5303 fails\n>> because of a pack file that remains open. (Open files cannot be replaced\n>> on Windows.) I had hoped that your patch would help, but it does not.\n>> Something else still keeps the pack file open. Can anything be done about\n>> that?\n>>\n> \n> Do this _and_ the other handle-leak-patch together help?\n> (I think the second should)\n\nNo, that doesn't help, either. :-(\n\n-- Hannes\n"},{"id":"96145","messageId":"alpine.LFD.2.00.0811190740570.27509@xanadu.home","threadId":"16389","inReplyTo":"81b0412b0811190313p643c0cb4vad620ea942aeea93@mail.gmail.com","subject":"Re: [PATCH] Fix handle leak in builtin-pack-objects","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-11-19T12:45:16Z","receivedAt":"2008-11-19T12:45:16Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 19 Nov 2008, Alex Riesen wrote:\n\n> The opened packs seem to stay open forever.\n> \n> Signed-off-by: Alex Riesen \n> ---\n> \n> I'm very unsure about the solution, though: it is really horrible code\n> to debug...\n\nWhat is the actual problem? Pack windows are left open on purpose.\n\n\nNicolas\n"},{"id":"96147","messageId":"alpine.LFD.2.00.0811190753420.27509@xanadu.home","threadId":"16389","inReplyTo":"4923FE58.3090503@viscovery.net","subject":"Re: [PATCH] Fix handle leak in builtin-pack-objects","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-11-19T12:55:03Z","receivedAt":"2008-11-19T12:55:03Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 19 Nov 2008, Johannes Sixt wrote:\n\n> Alex Riesen schrieb:\n> > The opened packs seem to stay open forever.\n> \n> In my MinGW port I have the patch below that avoids that t5303 fails\n> because of a pack file that remains open. (Open files cannot be replaced\n> on Windows.) I had hoped that your patch would help, but it does not.\n> Something else still keeps the pack file open. Can anything be done about\n> that?\n> \n> -- Hannes\n> \n> From: Johannes Sixt \n> Date: Mon, 17 Nov 2008 09:25:19 +0100\n> Subject: [PATCH] t5303: Do not overwrite an existing pack\n> \n> This test corrupts a pack file, then repacks the objects. The consequence\n> is that the repacked pack file has the same name as the original file\n> (that has been corrupted).\n> \n> During its operation, git-pack-objects opens the corrupted file and keeps\n> it open at all times. On Windows, this is a problem because a file that is\n> open in any process cannot be delete or replaced, but that is what we do\n> in some of the test cases, and so they fail.\n> \n> The work-around is to write the repacked objects to a file of a different\n> name, and replace the original after git-pack-objects has terminated.\n> \n> Signed-off-by: Johannes Sixt \n\nAcked-by: Nicolas Pitre \n\n\n> ---\n> t/t5303-pack-corruption-resilience.sh | 7 +++++--\n> 1 files changed, 5 insertions(+), 2 deletions(-)\n> \n> diff --git a/t/t5303-pack-corruption-resilience.sh\n> b/t/t5303-pack-corruption-resilience.sh\n> index 5132d41..41c83e3 100755\n> --- a/t/t5303-pack-corruption-resilience.sh\n> +++ b/t/t5303-pack-corruption-resilience.sh\n> @@ -43,8 +43,11 @@ create_new_pack() {\n> \n> do_repack() {\n> pack=`printf \"$blob_1\\n$blob_2\\n$blob_3\\n\" |\n> - git pack-objects $@ .git/objects/pack/pack` &&\n> - pack=\".git/objects/pack/pack-${pack}\"\n> + git pack-objects $@ .git/objects/pack/packtmp` &&\n> + packtmp=\".git/objects/pack/packtmp-${pack}\" &&\n> + pack=\".git/objects/pack/pack-${pack}\" &&\n> + mv \"${packtmp}.pack\" \"${pack}.pack\" &&\n> + mv \"${packtmp}.idx\" \"${pack}.idx\"\n> }\n> \n> do_corrupt_object() {\n> -- \n> 1.6.0.4.1683.g35125\n> \n> \n\n\nNicolas\n"},{"id":"96150","messageId":"49240F6D.3030203@viscovery.net","threadId":"16389","inReplyTo":"alpine.LFD.2.00.0811190753420.27509@xanadu.home","subject":"Re: [PATCH] Fix handle leak in builtin-pack-objects","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-11-19T13:06:53Z","receivedAt":"2008-11-19T13:06:53Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Nicolas Pitre schrieb:\n> On Wed, 19 Nov 2008, Johannes Sixt wrote:\n> \n>> Alex Riesen schrieb:\n>>> The opened packs seem to stay open forever.\n>> In my MinGW port I have the patch below that avoids that t5303 fails\n>> because of a pack file that remains open. (Open files cannot be replaced\n>> on Windows.) I had hoped that your patch would help, but it does not.\n>> Something else still keeps the pack file open. Can anything be done about\n>> that?\n>>\n>> -- Hannes\n>>\n>> From: Johannes Sixt \n>> Date: Mon, 17 Nov 2008 09:25:19 +0100\n>> Subject: [PATCH] t5303: Do not overwrite an existing pack\n>>\n...\n> Acked-by: Nicolas Pitre \n\nThanks, but I should have mentioned that at this time this patch was just\nmeant for exposition, not inclusion. [It's just one of many, many patches\nneeded to make the test suite pass on MinGW.]\n\nI'd prefer a solution to the problem that the pack file remains open. Do\nyou have an idea where git-pack-objects keeps the pack file open, even\nwith Alex's two patches applied?\n\n-- Hannes\n"},{"id":"96154","messageId":"81b0412b0811190530m79a11b9dx3771b9679e53e9e@mail.gmail.com","threadId":"16389","inReplyTo":"alpine.LFD.2.00.0811190740570.27509@xanadu.home","subject":"Re: [PATCH] Fix handle leak in builtin-pack-objects","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-11-19T13:30:52Z","receivedAt":"2008-11-19T13:30:52Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2008/11/19 Nicolas Pitre :\n> On Wed, 19 Nov 2008, Alex Riesen wrote:\n>\n>> The opened packs seem to stay open forever.\n>>\n>> Signed-off-by: Alex Riesen \n>> ---\n>>\n>> I'm very unsure about the solution, though: it is really horrible code\n>> to debug...\n>\n> What is the actual problem? Pack windows are left open on purpose.\n>\n\n* expecting success: do_repack &&\n git prune-packed -v &&\n git verify-pack ${pack}.pack &&\n git cat-file blob $blob_1 > /dev/null &&\n git cat-file blob $blob_2 > /dev/null &&\n git cat-file blob $blob_3 > /dev/null\nCounting objects: 3, done.\nerror: unknown object type 0 at offset 2032 in\n.git/objects/pack/pack-8d1e04fc992cfbdb3ab72cf7abbf08cce8b1900.pack\nCompressing objects: 100% (2/2), done.\nfatal: unable to rename temporary pack file\n.git/objects/pack/tmp_pack_s9Gijm->.git/objects/pack/pack-b8d1e04fc992cfbdbab72cf7abbf08cce8b1900.pack:\nPermission denied\n* FAIL 10: ... and then a repack \"clears\" the corruption\n do_repack &&\n git prune-packed -v &&\n git verify-pack ${pack}.pack &&\n git cat-file blob $blob_1 > /dev/null &&\n git cat-file blob $blob_2 > /dev/null &&\n git cat-file blob $blob_3 > /dev/null\n"},{"id":"96155","messageId":"81b0412b0811190534r4f71f981s53de415f79e56e25@mail.gmail.com","threadId":"16389","inReplyTo":"alpine.LFD.2.00.0811190753420.27509@xanadu.home","subject":"Re: [PATCH] Fix handle leak in builtin-pack-objects","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-11-19T13:34:30Z","receivedAt":"2008-11-19T13:34:30Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2008/11/19 Nicolas Pitre :\n> On Wed, 19 Nov 2008, Johannes Sixt wrote:\n>> The work-around is to write the repacked objects to a file of a different\n>> name, and replace the original after git-pack-objects has terminated.\n>>\n>> Signed-off-by: Johannes Sixt \n>\n> Acked-by: Nicolas Pitre \n>\n\nAre you sure? Will it work in a real repository? Were noone does\nrename the previous pack files into packtmp-something?\n"},{"id":"96157","messageId":"49241AEF.1080808@viscovery.net","threadId":"16389","inReplyTo":"81b0412b0811190534r4f71f981s53de415f79e56e25@mail.gmail.com","subject":"Re: [PATCH] Fix handle leak in builtin-pack-objects","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-11-19T13:55:59Z","receivedAt":"2008-11-19T13:55:59Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Alex Riesen schrieb:\n> 2008/11/19 Nicolas Pitre :\n>> On Wed, 19 Nov 2008, Johannes Sixt wrote:\n>>> The work-around is to write the repacked objects to a file of a different\n>>> name, and replace the original after git-pack-objects has terminated.\n>>>\n>>> Signed-off-by: Johannes Sixt \n>> Acked-by: Nicolas Pitre \n> \n> Are you sure? Will it work in a real repository? Were noone does\n> rename the previous pack files into packtmp-something?\n\nOh, the patch only works around the failure in the test case. In a real\nrepository there is usually no problem because the destination pack file\ndoes not exist.\n\nThe unusual case is where you do this:\n\n $ git rev-list -10 HEAD | git pack-objects foobar\n\ntwice in a row: In this case the second invocation fails on Windows\nbecause the destination pack file already exists *and* is open. But not\neven git-repack does this even if it is called twice. OTOH, the test case\n*does* exactly this.\n\n-- Hannes\n"},{"id":"96158","messageId":"81b0412b0811190617t4a9df151m886700672a21fad8@mail.gmail.com","threadId":"16389","inReplyTo":"49241AEF.1080808@viscovery.net","subject":"Re: [PATCH] Fix handle leak in builtin-pack-objects","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-11-19T14:17:39Z","receivedAt":"2008-11-19T14:17:39Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2008/11/19 Johannes Sixt :\n> Alex Riesen schrieb:\n>> 2008/11/19 Nicolas Pitre :\n>>> On Wed, 19 Nov 2008, Johannes Sixt wrote:\n>>>> The work-around is to write the repacked objects to a file of a different\n>>>> name, and replace the original after git-pack-objects has terminated.\n>>>>\n>>>> Signed-off-by: Johannes Sixt \n>>> Acked-by: Nicolas Pitre \n>>\n>> Are you sure? Will it work in a real repository? Were noone does\n>> rename the previous pack files into packtmp-something?\n>\n> Oh, the patch only works around the failure in the test case. In a real\n> repository there is usually no problem because the destination pack file\n> does not exist.\n>\n> The unusual case is where you do this:\n>\n> $ git rev-list -10 HEAD | git pack-objects foobar\n>\n> twice in a row: In this case the second invocation fails on Windows\n> because the destination pack file already exists *and* is open. But not\n> even git-repack does this even if it is called twice. OTOH, the test case\n> *does* exactly this.\n>\n\nStill bad...\n\nBTW, the patch _does_ fix the \"unusual case\" for me. IOW, I plainly copied\nyour rev-list|pack-objects into command line. As expected, it fails for Junio's\nmaster (after the second run) and works with the patch:\n\n $ git rev-list -10 HEAD | ./git pack-objects .git/objects/pack/foobar\n Counting objects: 10, done.\n Compressing objects: 100% (9/9), done.\n 82864ec14cd06e6089543a1419762e4cd40f7988\n Writing objects: 100% (10/10), done.\n Total 10 (delta 1), reused 10 (delta 1)\n\n $ git rev-list -10 HEAD | ./git pack-objects .git/objects/pack/foobar\n Counting objects: 10, done.\n Compressing objects: 100% (9/9), done.\n fatal: unable to rename temporary pack file: Permission denied\n\n $ git cherry-pick fix-fd-leak\n Finished one cherry-pick.\n [master]: created e629465: \"Fix handle leak in builtin-pack-objects\"\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\n $ git rev-list -10 HEAD | ./git pack-objects .git/objects/pack/foobar\n Counting objects: 10, done.\n Compressing objects: 100% (9/9), done.\n 0e43d919a2a8336fd740d8d9b8f9f78d49e855e5\n Writing objects: 100% (10/10), done.\n Total 10 (delta 1), reused 9 (delta 1)\n\n $ git rev-list -10 HEAD | ./git pack-objects .git/objects/pack/foobar\n Counting objects: 10, done.\n Compressing objects: 100% (9/9), done.\n 0e43d919a2a8336fd740d8d9b8f9f78d49e855e5\n Writing objects: 100% (10/10), done.\n Total 10 (delta 1), reused 10 (delta 1)\n\n $ git rev-list -10 HEAD | ./git pack-objects .git/objects/pack/foobar\n Counting objects: 10, done.\n Compressing objects: 100% (9/9), done.\n 0e43d919a2a8336fd740d8d9b8f9f78d49e855e5\n Writing objects: 100% (10/10), done.\n Total 10 (delta 1), reused 10 (delta 1)\n"},{"id":"96159","messageId":"alpine.LFD.2.00.0811190917020.27509@xanadu.home","threadId":"16389","inReplyTo":"49240F6D.3030203@viscovery.net","subject":"Re: [PATCH] Fix handle leak in builtin-pack-objects","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-11-19T14:31:39Z","receivedAt":"2008-11-19T14:31:39Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 19 Nov 2008, Johannes Sixt wrote:\n\n> Nicolas Pitre schrieb:\n> > On Wed, 19 Nov 2008, Johannes Sixt wrote:\n> > \n> >> Alex Riesen schrieb:\n> >>> The opened packs seem to stay open forever.\n> >> In my MinGW port I have the patch below that avoids that t5303 fails\n> >> because of a pack file that remains open. (Open files cannot be replaced\n> >> on Windows.) I had hoped that your patch would help, but it does not.\n> >> Something else still keeps the pack file open. Can anything be done about\n> >> that?\n> >>\n> >> -- Hannes\n> >>\n> >> From: Johannes Sixt \n> >> Date: Mon, 17 Nov 2008 09:25:19 +0100\n> >> Subject: [PATCH] t5303: Do not overwrite an existing pack\n> >>\n> ...\n> > Acked-by: Nicolas Pitre \n> \n> Thanks, but I should have mentioned that at this time this patch was just\n> meant for exposition, not inclusion.\n\nWell, I'd include it right away since it is fundamentally the right \nthing to do.\n\n> I'd prefer a solution to the problem that the pack file remains open. Do\n> you have an idea where git-pack-objects keeps the pack file open, even\n> with Alex's two patches applied?\n\nThat's not the issue.\n\nThe test is using pack-objects to overwrite a pack file, but to do so \npack-objects has to open and read from the pack file about to be \noverwritten. This is just wrong even if it happens to work by luck on \nLinux.\n\nIt is on purpose that the pack files are kept open. This is to minimize \nthe number of open() and mmap()/read() operations between successive \nobject reads. Those are not leaked file handles since they get closed \nwhen new packs are opened and the number of concurrent opened packs \nreaches a certain limit.\n\n\nNicolas\n"},{"id":"96161","messageId":"alpine.LFD.2.00.0811190940480.27509@xanadu.home","threadId":"16389","inReplyTo":"49241AEF.1080808@viscovery.net","subject":"Re: [PATCH] Fix handle leak in builtin-pack-objects","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-11-19T14:42:43Z","receivedAt":"2008-11-19T14:42:43Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 19 Nov 2008, Johannes Sixt wrote:\n\n> Alex Riesen schrieb:\n> > 2008/11/19 Nicolas Pitre :\n> >> On Wed, 19 Nov 2008, Johannes Sixt wrote:\n> >>> The work-around is to write the repacked objects to a file of a different\n> >>> name, and replace the original after git-pack-objects has terminated.\n> >>>\n> >>> Signed-off-by: Johannes Sixt \n> >> Acked-by: Nicolas Pitre \n> > \n> > Are you sure? Will it work in a real repository? Were noone does\n> > rename the previous pack files into packtmp-something?\n> \n> Oh, the patch only works around the failure in the test case. In a real\n> repository there is usually no problem because the destination pack file\n> does not exist.\n> \n> The unusual case is where you do this:\n> \n> $ git rev-list -10 HEAD | git pack-objects foobar\n> \n> twice in a row: In this case the second invocation fails on Windows\n> because the destination pack file already exists *and* is open. But not\n> even git-repack does this even if it is called twice. OTOH, the test case\n> *does* exactly this.\n\nOK.... Well, despite my earlier assertion, I think the above should be a \nvalid operation.\n\nI'm looking at it now. I'm therefore revoking my earlier ACK as well \n(better keep that test case alive).\n\n\nNicolas\n"},{"id":"96163","messageId":"49242821.50604@viscovery.net","threadId":"16389","inReplyTo":"alpine.LFD.2.00.0811190940480.27509@xanadu.home","subject":"Re: [PATCH] Fix handle leak in builtin-pack-objects","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-11-19T14:52:17Z","receivedAt":"2008-11-19T14:52:17Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Nicolas Pitre schrieb:\n> On Wed, 19 Nov 2008, Johannes Sixt wrote:\n>> The unusual case is where you do this:\n>>\n>> $ git rev-list -10 HEAD | git pack-objects foobar\n>>\n>> twice in a row: In this case the second invocation fails on Windows\n>> because the destination pack file already exists *and* is open. But not\n>> even git-repack does this even if it is called twice. OTOH, the test case\n>> *does* exactly this.\n> \n> OK.... Well, despite my earlier assertion, I think the above should be a \n> valid operation.\n> \n> I'm looking at it now. I'm therefore revoking my earlier ACK as well \n> (better keep that test case alive).\n\nHold on a moment: When I tested the above sequence, I was fooled by a flaw\nin mingw_rename() (it doesn't replace read-only files). With that fixed,\nit works as expected in repeated invocations (note that foobar is outside\nthe .git/objects/pack directory).\n\nIf I use .git/objects/pack/foobar instead, then I get the failures on\nWindows, and I won't argue that this should be \"fixed\". ;)\n\n-- Hannes\n"},{"id":"96172","messageId":"49243DF7.7000604@viscovery.net","threadId":"16389","inReplyTo":"49242821.50604@viscovery.net","subject":"[PATCH] compat/mingw.c: Teach mingw_rename() to replace read-only files","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-11-19T16:25:27Z","receivedAt":"2008-11-19T16:25:27Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"From: Johannes Sixt \n\nOn POSIX, rename() can replace files that are not writable. On Windows,\nhowever, read-only files cannot be replaced without additional efforts:\nWe have to make the destination writable first.\n\nSince the situations where the destination is read-only are rare, we do not\nmake the destination writable on every invocation, but only if the first\ntry to rename a file failed with an \"access denied\" error.\n\nSigned-off-by: Johannes Sixt \n---\nJohannes Sixt schrieb:\n> Nicolas Pitre schrieb:\n>> On Wed, 19 Nov 2008, Johannes Sixt wrote:\n>>> The unusual case is where you do this:\n>>>\n>>> $ git rev-list -10 HEAD | git pack-objects foobar\n>>>\n>>> twice in a row: In this case the second invocation fails on Windows\n>>> because the destination pack file already exists *and* is open. But not\n>>> even git-repack does this even if it is called twice. OTOH, the test case\n>>> *does* exactly this.\n>> OK.... Well, despite my earlier assertion, I think the above should be a \n>> valid operation.\n\nWould you please clarify which operation you exactly mean? As is written\nabove, or with .git/objects/pack/foobar instead like in the test case?\n\n>> I'm looking at it now. I'm therefore revoking my earlier ACK as well \n>> (better keep that test case alive).\n> \n> Hold on a moment: When I tested the above sequence, I was fooled by a flaw\n> in mingw_rename() (it doesn't replace read-only files). With that fixed...\n\nHere is this fix.\n\n-- Hannes\n\n compat/mingw.c | 15 ++++++++++++---\n 1 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex b534a8a..3dbe6a7 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -819,6 +819,8 @@ int mingw_connect(int sockfd, struct sockaddr *sa, size_t sz)\n #undef rename\n int mingw_rename(const char *pold, const char *pnew)\n {\n+\tDWORD attrs;\n+\n \t/*\n \t * Try native rename() first to get errno right.\n \t * It is based on MoveFile(), which cannot overwrite existing files.\n@@ -830,12 +832,19 @@ int mingw_rename(const char *pold, const char *pnew)\n \tif (MoveFileEx(pold, pnew, MOVEFILE_REPLACE_EXISTING))\n \t\treturn 0;\n \t/* TODO: translate more errors */\n-\tif (GetLastError() == ERROR_ACCESS_DENIED) {\n-\t\tDWORD attrs = GetFileAttributes(pnew);\n-\t\tif (attrs != INVALID_FILE_ATTRIBUTES && (attrs & FILE_ATTRIBUTE_DIRECTORY)) {\n+\tif (GetLastError() == ERROR_ACCESS_DENIED &&\n+\t (attrs = GetFileAttributes(pnew)) != INVALID_FILE_ATTRIBUTES) {\n+\t\tif (attrs & FILE_ATTRIBUTE_DIRECTORY) {\n \t\t\terrno = EISDIR;\n \t\t\treturn -1;\n \t\t}\n+\t\tif ((attrs & FILE_ATTRIBUTE_READONLY) &&\n+\t\t SetFileAttributes(pnew, attrs & ~FILE_ATTRIBUTE_READONLY)) {\n+\t\t\tif (MoveFileEx(pold, pnew, MOVEFILE_REPLACE_EXISTING))\n+\t\t\t\treturn 0;\n+\t\t\t/* revert file attributes on failure */\n+\t\t\tSetFileAttributes(pnew, attrs);\n+\t\t}\n \t}\n \terrno = EACCES;\n \treturn -1;\n-- \n1.6.0.4.1694.g07ac\n"},{"id":"96543","messageId":"81b0412b0811260518o52adb107tbddafb324e7fd97b@mail.gmail.com","threadId":"16389","inReplyTo":"alpine.LFD.2.00.0811190940480.27509@xanadu.home","subject":"Re: [PATCH] Fix handle leak in builtin-pack-objects","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-11-26T13:18:57Z","receivedAt":"2008-11-26T13:18:57Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2008/11/19 Nicolas Pitre :\n> On Wed, 19 Nov 2008, Johannes Sixt wrote:\n>> Alex Riesen schrieb:\n>> > 2008/11/19 Nicolas Pitre :\n>> >> On Wed, 19 Nov 2008, Johannes Sixt wrote:\n>> >>> The work-around is to write the repacked objects to a file of a different\n>> >>> name, and replace the original after git-pack-objects has terminated.\n>> >>>\n>> >>> Signed-off-by: Johannes Sixt \n>> >> Acked-by: Nicolas Pitre \n>> >\n>> > Are you sure? Will it work in a real repository? Were noone does\n>> > rename the previous pack files into packtmp-something?\n>>\n>> Oh, the patch only works around the failure in the test case. In a real\n>> repository there is usually no problem because the destination pack file\n>> does not exist.\n>>\n>> The unusual case is where you do this:\n>>\n>> $ git rev-list -10 HEAD | git pack-objects foobar\n>>\n>> twice in a row: In this case the second invocation fails on Windows\n>> because the destination pack file already exists *and* is open. But not\n>> even git-repack does this even if it is called twice. OTOH, the test case\n>> *does* exactly this.\n>\n> OK.... Well, despite my earlier assertion, I think the above should be a\n> valid operation.\n>\n> I'm looking at it now. I'm therefore revoking my earlier ACK as well\n> (better keep that test case alive).\n>\n\nAny news here?\n"},{"id":"96549","messageId":"alpine.LFD.2.00.0811260931030.14328@xanadu.home","threadId":"16389","inReplyTo":"81b0412b0811260518o52adb107tbddafb324e7fd97b@mail.gmail.com","subject":"Re: [PATCH] Fix handle leak in builtin-pack-objects","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-11-26T14:33:51Z","receivedAt":"2008-11-26T14:33:51Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 26 Nov 2008, Alex Riesen wrote:\n\n> 2008/11/19 Nicolas Pitre :\n> > On Wed, 19 Nov 2008, Johannes Sixt wrote:\n> >> Alex Riesen schrieb:\n> >> > 2008/11/19 Nicolas Pitre :\n> >> >> On Wed, 19 Nov 2008, Johannes Sixt wrote:\n> >> >>> The work-around is to write the repacked objects to a file of a different\n> >> >>> name, and replace the original after git-pack-objects has terminated.\n> >> >>>\n> >> >>> Signed-off-by: Johannes Sixt \n> >> >> Acked-by: Nicolas Pitre \n> >> >\n> >> > Are you sure? Will it work in a real repository? Were noone does\n> >> > rename the previous pack files into packtmp-something?\n> >>\n> >> Oh, the patch only works around the failure in the test case. In a real\n> >> repository there is usually no problem because the destination pack file\n> >> does not exist.\n> >>\n> >> The unusual case is where you do this:\n> >>\n> >> $ git rev-list -10 HEAD | git pack-objects foobar\n> >>\n> >> twice in a row: In this case the second invocation fails on Windows\n> >> because the destination pack file already exists *and* is open. But not\n> >> even git-repack does this even if it is called twice. OTOH, the test case\n> >> *does* exactly this.\n> >\n> > OK.... Well, despite my earlier assertion, I think the above should be a\n> > valid operation.\n> >\n> > I'm looking at it now. I'm therefore revoking my earlier ACK as well\n> > (better keep that test case alive).\n> >\n> \n> Any news here?\n\nYes: my hard disk crashed a couple hours after I mentioned this, so most \nof my time has been spent on recovery since then. I'll come back to it \neventually.\n\n\nNicolas\n"},{"id":"96567","messageId":"20081126184327.GA4399@blimp.localdomain","threadId":"16389","inReplyTo":"alpine.LFD.2.00.0811260931030.14328@xanadu.home","subject":"Re: [PATCH] Fix handle leak in builtin-pack-objects","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-11-26T18:43:27Z","receivedAt":"2008-11-26T18:43:27Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Nicolas Pitre, Wed, Nov 26, 2008 15:33:51 +0100:\n> On Wed, 26 Nov 2008, Alex Riesen wrote:\n> > 2008/11/19 Nicolas Pitre :\n> > > I'm looking at it now. I'm therefore revoking my earlier ACK as well\n> > > (better keep that test case alive).\n> > >\n> > \n> > Any news here?\n> \n> Yes: my hard disk crashed a couple hours after I mentioned this, so most \n> of my time has been spent on recovery since then. I'll come back to it \n> eventually.\n\nHeh... Good luck!\n"},{"id":"97449","messageId":"alpine.LFD.2.00.0812091414030.14328@xanadu.home","threadId":"16389","inReplyTo":"alpine.LFD.2.00.0811260931030.14328@xanadu.home","subject":"[PATCH] make sure packs to be replaced are closed beforehand","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-12-09T19:26:52Z","receivedAt":"2008-12-09T19:26:52Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"Especially on Windows where an opened file cannot be replaced, make\nsure pack-objects always close packs it is about to replace. Even on\nnon Windows systems, this could save potential bad results if ever\nobjects were to be read from the new pack file using offset from the old\nindex.\n\nThis should fix t5303 on Windows.\n\nSigned-off-by: Nicolas Pitre \n---\n\nOn Wed, 26 Nov 2008, Nicolas Pitre wrote:\n> On Wed, 26 Nov 2008, Alex Riesen wrote:\n> > 2008/11/19 Nicolas Pitre :\n> > > On Wed, 19 Nov 2008, Johannes Sixt wrote:\n> > >> Oh, the patch only works around the failure in the test case. In a real\n> > >> repository there is usually no problem because the destination pack file\n> > >> does not exist.\n> > >>\n> > >> The unusual case is where you do this:\n> > >>\n> > >> $ git rev-list -10 HEAD | git pack-objects foobar\n> > >>\n> > >> twice in a row: In this case the second invocation fails on Windows\n> > >> because the destination pack file already exists *and* is open. But not\n> > >> even git-repack does this even if it is called twice. OTOH, the test case\n> > >> *does* exactly this.\n> > >\n> > > OK.... Well, despite my earlier assertion, I think the above should be a\n> > > valid operation.\n> > >\n> > > I'm looking at it now. I'm therefore revoking my earlier ACK as well\n> > > (better keep that test case alive).\n> > >\n> > \n> > Any news here?\n> \n> Yes: my hard disk crashed a couple hours after I mentioned this, so most \n> of my time has been spent on recovery since then. I'll come back to it \n> eventually.\n\nOK, here it is at last. Please confirm it works on Windows before Junio \nmerges it.\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 67eefa2..cedef52 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -535,6 +535,7 @@ static void write_pack_file(void)\n \n \t\t\tsnprintf(tmpname, sizeof(tmpname), \"%s-%s.pack\",\n \t\t\t\t base_name, sha1_to_hex(sha1));\n+\t\t\tfree_pack_by_name(tmpname);\n \t\t\tif (adjust_perm(pack_tmp_name, mode))\n \t\t\t\tdie(\"unable to make temporary pack file readable: %s\",\n \t\t\t\t strerror(errno));\ndiff --git a/cache.h b/cache.h\nindex f15b3fc..231c06d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -820,6 +820,7 @@ extern int open_pack_index(struct packed_git *);\n extern unsigned char* use_pack(struct packed_git *, struct pack_window **, off_t, unsigned int *);\n extern void close_pack_windows(struct packed_git *);\n extern void unuse_pack(struct pack_window **);\n+extern void free_pack_by_name(const char *);\n extern struct packed_git *add_packed_git(const char *, int, int);\n extern const unsigned char *nth_packed_object_sha1(struct packed_git *, uint32_t);\n extern off_t nth_packed_object_offset(const struct packed_git *, uint32_t);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 6c0e251..0e021c5 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -673,6 +673,37 @@ void unuse_pack(struct pack_window **w_cursor)\n }\n \n /*\n+ * This is used by git-repack in case a newly created pack happens to\n+ * contain the same set of objects as an existing one. In that case\n+ * the resulting file might be different even if its name would be the\n+ * same. It is best to close any reference to the old pack before it is\n+ * replaced on disk. Of course no index pointers nor windows for given pack\n+ * must subsist at this point. If ever objects from this pack are requested\n+ * again, the new version of the pack will be reinitialized through\n+ * reprepare_packed_git().\n+ */\n+void free_pack_by_name(const char *pack_name)\n+{\n+\tstruct packed_git *p, **pp = &packed_git;\n+\n+\twhile (*pp) {\n+\t\tp = *pp;\n+\t\tif (strcmp(pack_name, p->pack_name) == 0) {\n+\t\t\tclose_pack_windows(p);\n+\t\t\tif (p->pack_fd != -1)\n+\t\t\t\tclose(p->pack_fd);\n+\t\t\tif (p->index_data)\n+\t\t\t\tmunmap((void *)p->index_data, p->index_size);\n+\t\t\tfree(p->bad_object_sha1);\n+\t\t\t*pp = p->next;\n+\t\t\tfree(p);\n+\t\t\treturn;\n+\t\t}\n+\t\tpp = &p->next;\n+\t}\n+}\n+\n+/*\n * Do not call this directly as this leaks p->pack_fd on error return;\n * call open_packed_git() instead.\n */\n"},{"id":"97450","messageId":"20081209193330.GA8877@blimp.localdomain","threadId":"16389","inReplyTo":"alpine.LFD.2.00.0812091414030.14328@xanadu.home","subject":"Re: [PATCH] make sure packs to be replaced are closed beforehand","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-12-09T19:33:30Z","receivedAt":"2008-12-09T19:33:30Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"Nicolas Pitre, Tue, Dec 09, 2008 20:26:52 +0100:\n> Especially on Windows where an opened file cannot be replaced, make\n> sure pack-objects always close packs it is about to replace. Even on\n> non Windows systems, this could save potential bad results if ever\n> objects were to be read from the new pack file using offset from the old\n> index.\n> \n> This should fix t5303 on Windows.\n> \n> Signed-off-by: Nicolas Pitre \n> ---\n...\n> OK, here it is at last. Please confirm it works on Windows before Junio \n> merges it.\n> \n\nWill do in about 16 hours.\n"},{"id":"97469","messageId":"493F71B7.60804@viscovery.net","threadId":"16389","inReplyTo":"alpine.LFD.2.00.0812091414030.14328@xanadu.home","subject":"Re: [PATCH] make sure packs to be replaced are closed beforehand","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-12-10T07:37:27Z","receivedAt":"2008-12-10T07:37:27Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Nicolas Pitre schrieb:\n> Especially on Windows where an opened file cannot be replaced, make\n> sure pack-objects always close packs it is about to replace. Even on\n> non Windows systems, this could save potential bad results if ever\n> objects were to be read from the new pack file using offset from the old\n> index.\n> \n> This should fix t5303 on Windows.\n...\n> OK, here it is at last. Please confirm it works on Windows before Junio \n> merges it.\n\nI can confirm that this patch fixes t5303 on Windows (MinGW).\n\n-- Hannes\n"},{"id":"97471","messageId":"7vabb43065.fsf@gitster.siamese.dyndns.org","threadId":"16389","inReplyTo":"493F71B7.60804@viscovery.net","subject":"Re: [PATCH] make sure packs to be replaced are closed beforehand","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-12-10T08:27:14Z","receivedAt":"2008-12-10T08:27:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Nicolas Pitre schrieb:\n>> Especially on Windows where an opened file cannot be replaced, make\n>> sure pack-objects always close packs it is about to replace. Even on\n>> non Windows systems, this could save potential bad results if ever\n>> objects were to be read from the new pack file using offset from the old\n>> index.\n>> \n>> This should fix t5303 on Windows.\n> ...\n>> OK, here it is at last. Please confirm it works on Windows before Junio \n>> merges it.\n>\n> I can confirm that this patch fixes t5303 on Windows (MinGW).\n\nThanks; it is a bit too late for tonight, but it will appear in tomorrow's\n'master'.\n"},{"id":"97479","messageId":"81b0412b0812100136j6ae4f3few2ab00a475643b538@mail.gmail.com","threadId":"16389","inReplyTo":"493F71B7.60804@viscovery.net","subject":"Re: [PATCH] make sure packs to be replaced are closed beforehand","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2008-12-10T09:36:05Z","receivedAt":"2008-12-10T09:36:05Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"2008/12/10 Johannes Sixt :\n> Nicolas Pitre schrieb:\n>> Especially on Windows where an opened file cannot be replaced, make\n>> sure pack-objects always close packs it is about to replace. Even on\n>> non Windows systems, this could save potential bad results if ever\n>> objects were to be read from the new pack file using offset from the old\n>> index.\n>>\n>> This should fix t5303 on Windows.\n> ...\n>> OK, here it is at last. Please confirm it works on Windows before Junio\n>> merges it.\n>\n> I can confirm that this patch fixes t5303 on Windows (MinGW).\n>\n\nAnd it does that for me, FWIW.\n"}]}