{"thread":{"id":"28357","subject":"[PATCH] Support empty blob in fsck --lost-found","startedAt":"2011-09-11T15:40:28Z","lastAt":"2011-09-12T01:10:41Z","messageCount":7,"participants":["BJ Hargrave","Sverre Rabbelier","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"175295","messageId":"A3964281-B24B-46C0-AE73-0CCB4C12556F@bjhargrave.com","threadId":"28357","inReplyTo":null,"subject":"[PATCH] Support empty blob in fsck --lost-found","fromName":"BJ Hargrave","fromEmail":"bj@bjhargrave.com","sentAt":"2011-09-11T15:40:28Z","receivedAt":"2011-09-11T15:40:28Z","isPatch":true,"sender":{"key":"bj@bjhargrave.com","avatar":"https://gravatar.com/avatar/48e60c01177c0e8d3e60c996d54fbe36cf70058efcdd020375b4055e34fc05d7?d=mp&s=160"},"body":"fsck --lost-found died when attempting to write out the empty blob.\nAvoid calling fwrite when the blob size is zero since the call to\nfwrite returns 0 objects written which fails the check and caused\nfsck to die.\n\nSigned-off-by: BJ Hargrave <bj@bjhargrave.com>\n---\n builtin/fsck.c        |    7 ++++---\n t/t1420-lost-found.sh |   13 ++++++++-----\n 2 files changed, 12 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 5ae0366..ad6d713 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -232,9 +232,10 @@ static void check_unreachable_object(struct object *obj)\n \t\t\t\tchar *buf = read_sha1_file(obj->sha1,\n \t\t\t\t\t\t&type, &size);\n \t\t\t\tif (buf) {\n-\t\t\t\t\tif (fwrite(buf, size, 1, f) != 1)\n-\t\t\t\t\t\tdie_errno(\"Could not write '%s'\",\n-\t\t\t\t\t\t\t  filename);\n+\t\t\t\t\tif (size > 0)\n+\t\t\t\t\t\tif (fwrite(buf, size, 1, f) != 1)\n+\t\t\t\t\t\t\tdie_errno(\"Could not write '%s'\",\n+\t\t\t\t\t\t\t\t  filename);\n \t\t\t\t\tfree(buf);\n \t\t\t\t}\n \t\t\t} else\ndiff --git a/t/t1420-lost-found.sh b/t/t1420-lost-found.sh\nindex dc9e402..02323c9 100755\n--- a/t/t1420-lost-found.sh\n+++ b/t/t1420-lost-found.sh\n@@ -8,7 +8,7 @@ test_description='Test fsck --lost-found'\n \n test_expect_success setup '\n \tgit config core.logAllRefUpdates 0 &&\n-\t: > file1 &&\n+\techo x > file1 &&\n \tgit add file1 &&\n \ttest_tick &&\n \tgit commit -m initial &&\n@@ -18,18 +18,21 @@ test_expect_success setup '\n \ttest_tick &&\n \tgit commit -m second &&\n \techo 3 > file3 &&\n-\tgit add file3\n+\t: > file4 &&\n+\tgit add file3 file4\n '\n \n test_expect_success 'lost and found something' '\n \tgit rev-parse HEAD > lost-commit &&\n-\tgit rev-parse :file3 > lost-other &&\n+\tgit rev-parse :file3 > lost-other3 &&\n+\tgit rev-parse :file4 > lost-other4 &&\n \ttest_tick &&\n \tgit reset --hard HEAD^ &&\n \tgit fsck --lost-found &&\n-\ttest 2 = $(ls .git/lost-found/*/* | wc -l) &&\n+\ttest 3 = $(ls .git/lost-found/*/* | wc -l) &&\n \ttest -f .git/lost-found/commit/$(cat lost-commit) &&\n-\ttest -f .git/lost-found/other/$(cat lost-other)\n+\ttest -f .git/lost-found/other/$(cat lost-other3) &&\n+\ttest -f .git/lost-found/other/$(cat lost-other4)\n '\n \n test_done\n-- \n1.7.6.2\n"},{"id":"175297","messageId":"CAGdFq_hqfqdFyLY=KdA_QW5kH8Kjhx8Y18mHEga_Pdv8yzB2wg@mail.gmail.com","threadId":"28357","inReplyTo":"A3964281-B24B-46C0-AE73-0CCB4C12556F@bjhargrave.com","subject":"Re: [PATCH] Support empty blob in fsck --lost-found","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-09-11T16:03:21Z","receivedAt":"2011-09-11T16:03:21Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Sep 11, 2011 at 17:40, BJ Hargrave <bj@bjhargrave.com> wrote:\n> fsck --lost-found died when attempting to write out the empty blob.\n> Avoid calling fwrite when the blob size is zero since the call to\n> fwrite returns 0 objects written which fails the check and caused\n> fsck to die.\n\nNow we don't die at all if a 0-byte file couldn't be written.\nShouldn't we check errno or something?\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"175298","messageId":"E6A02216-CA67-4B66-AA8F-6DDE8AF7DF3A@bjhargrave.com","threadId":"28357","inReplyTo":"CAGdFq_hqfqdFyLY=KdA_QW5kH8Kjhx8Y18mHEga_Pdv8yzB2wg@mail.gmail.com","subject":"Re: [PATCH] Support empty blob in fsck --lost-found","fromName":"BJ Hargrave","fromEmail":"bj@bjhargrave.com","sentAt":"2011-09-11T16:20:06Z","receivedAt":"2011-09-11T16:20:06Z","isPatch":true,"sender":{"key":"bj@bjhargrave.com","avatar":"https://gravatar.com/avatar/48e60c01177c0e8d3e60c996d54fbe36cf70058efcdd020375b4055e34fc05d7?d=mp&s=160"},"body":"On Sep 11, 2011, at 12:03 , Sverre Rabbelier wrote:\n\n> Heya,\n> \n> On Sun, Sep 11, 2011 at 17:40, BJ Hargrave <bj@bjhargrave.com> wrote:\n>> fsck --lost-found died when attempting to write out the empty blob.\n>> Avoid calling fwrite when the blob size is zero since the call to\n>> fwrite returns 0 objects written which fails the check and caused\n>> fsck to die.\n> \n> Now we don't die at all if a 0-byte file couldn't be written.\n> Shouldn't we check errno or something?\n> \n\nYou don't need to write anything to the 0-byte file. Just create it and close it and there are checks already that verify the fopen and fclose do not fail. So I don't think we are missing any error conditions here.\n\n> -- \n> Cheers,\n> \n> Sverre Rabbelier\n\n-- \n\nBJ Hargrave\n"},{"id":"175299","messageId":"CAGdFq_iLnCtXWJsiYLatzyi3bjz1drv1kmM54y8mMkNaU8b33A@mail.gmail.com","threadId":"28357","inReplyTo":"E6A02216-CA67-4B66-AA8F-6DDE8AF7DF3A@bjhargrave.com","subject":"Re: [PATCH] Support empty blob in fsck --lost-found","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2011-09-11T16:21:31Z","receivedAt":"2011-09-11T16:21:31Z","isPatch":true,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sun, Sep 11, 2011 at 18:20, BJ Hargrave <bj@bjhargrave.com> wrote:\n> You don't need to write anything to the 0-byte file. Just create it and\n>  close it and there are checks already that verify the fopen and fclose\n> do not fail. So I don't think we are missing any error conditions here.\n\nWorks for me :).\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"175312","messageId":"7vty8iolnj.fsf@alter.siamese.dyndns.org","threadId":"28357","inReplyTo":"A3964281-B24B-46C0-AE73-0CCB4C12556F@bjhargrave.com","subject":"Re: [PATCH] Support empty blob in fsck --lost-found","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-11T20:43:12Z","receivedAt":"2011-09-11T20:43:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"BJ Hargrave <bj@bjhargrave.com> writes:\n\n> diff --git a/builtin/fsck.c b/builtin/fsck.c\n> index 5ae0366..ad6d713 100644\n> --- a/builtin/fsck.c\n> +++ b/builtin/fsck.c\n> @@ -232,9 +232,10 @@ static void check_unreachable_object(struct object *obj)\n>  \t\t\t\tchar *buf = read_sha1_file(obj->sha1,\n>  \t\t\t\t\t\t&type, &size);\n>  \t\t\t\tif (buf) {\n> -\t\t\t\t\tif (fwrite(buf, size, 1, f) != 1)\n> -\t\t\t\t\t\tdie_errno(\"Could not write '%s'\",\n> -\t\t\t\t\t\t\t  filename);\n> +\t\t\t\t\tif (size > 0)\n> +\t\t\t\t\t\tif (fwrite(buf, size, 1, f) != 1)\n> +\t\t\t\t\t\t\tdie_errno(\"Could not write '%s'\",\n> +\t\t\t\t\t\t\t\t  filename);\n\nFunny.\n\nI am sure we fixed a similar breakage elsewhere a few years ago, by\nswapping the size and nmemb to the calls (i.e. instead of writing one\nblock of \"size\" bytes, you could write \"size\" blocks of 1-byte) and making\nsure fwrite() reports the number of items. IOW\n\n\tif (buf && fwrite(buf, 1, size, f) != size)\n\t\tdie_errno(\"Could not write '%s'\", filename);\n"},{"id":"175318","messageId":"348F09EE-5EE2-4F3E-B1B1-6FD34BDBD117@bjhargrave.com","threadId":"28357","inReplyTo":"7vty8iolnj.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Support empty blob in fsck --lost-found","fromName":"BJ Hargrave","fromEmail":"bj@bjhargrave.com","sentAt":"2011-09-11T21:43:32Z","receivedAt":"2011-09-11T21:43:32Z","isPatch":true,"sender":{"key":"bj@bjhargrave.com","avatar":"https://gravatar.com/avatar/48e60c01177c0e8d3e60c996d54fbe36cf70058efcdd020375b4055e34fc05d7?d=mp&s=160"},"body":"\nOn Sep 11, 2011, at 16:43 , Junio C Hamano wrote:\n\n> Funny.\n> \n> I am sure we fixed a similar breakage elsewhere a few years ago, by\n> swapping the size and nmemb to the calls (i.e. instead of writing one\n> block of \"size\" bytes, you could write \"size\" blocks of 1-byte) and making\n> sure fwrite() reports the number of items. IOW\n> \n> \tif (buf && fwrite(buf, 1, size, f) != size)\n> \t\tdie_errno(\"Could not write '%s'\", filename);\n> \n\nDo you want me to resubmit the patch using this technique instead of the size > 0 check?\n-- \n\nBJ\n"},{"id":"175323","messageId":"7vmxeabm5q.fsf@alter.siamese.dyndns.org","threadId":"28357","inReplyTo":"348F09EE-5EE2-4F3E-B1B1-6FD34BDBD117@bjhargrave.com","subject":"Re: [PATCH] Support empty blob in fsck --lost-found","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-12T01:10:41Z","receivedAt":"2011-09-12T01:10:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"BJ Hargrave <bj@bjhargrave.com> writes:\n\n> On Sep 11, 2011, at 16:43 , Junio C Hamano wrote:\n>\n>> Funny.\n>> \n>> I am sure we fixed a similar breakage elsewhere a few years ago, by\n>> swapping the size and nmemb to the calls (i.e. instead of writing one\n>> block of \"size\" bytes, you could write \"size\" blocks of 1-byte) and making\n>> sure fwrite() reports the number of items. IOW\n>> \n>> \tif (buf && fwrite(buf, 1, size, f) != size)\n>> \t\tdie_errno(\"Could not write '%s'\", filename);\n>> \n>\n> Do you want me to resubmit the patch using this technique instead of the size > 0 check?\n\nNot really.\n\nI am not sure when/why we would try to write an empty blob out to begin\nwith...\n"}]}