# [PATCH] Support empty blob in fsck --lost-found 7 messages from 2011-09-11 to 2011-09-12. Participants: BJ Hargrave, Sverre Rabbelier, Junio C Hamano. Thread: https://gitlist.dev/t/28357 ## BJ Hargrave, 2011-09-11 15:40 Subject: [PATCH] Support empty blob in fsck --lost-found Message-ID: URL: https://gitlist.dev/e/A3964281-B24B-46C0-AE73-0CCB4C12556F%40bjhargrave.com ``` fsck --lost-found died when attempting to write out the empty blob. Avoid calling fwrite when the blob size is zero since the call to fwrite returns 0 objects written which fails the check and caused fsck to die. Signed-off-by: BJ Hargrave --- builtin/fsck.c | 7 ++++--- t/t1420-lost-found.sh | 13 ++++++++----- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/builtin/fsck.c b/builtin/fsck.c index 5ae0366..ad6d713 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -232,9 +232,10 @@ static void check_unreachable_object(struct object *obj) char *buf = read_sha1_file(obj->sha1, &type, &size); if (buf) { - if (fwrite(buf, size, 1, f) != 1) - die_errno("Could not write '%s'", - filename); + if (size > 0) + if (fwrite(buf, size, 1, f) != 1) + die_errno("Could not write '%s'", + filename); free(buf); } } else diff --git a/t/t1420-lost-found.sh b/t/t1420-lost-found.sh index dc9e402..02323c9 100755 --- a/t/t1420-lost-found.sh +++ b/t/t1420-lost-found.sh @@ -8,7 +8,7 @@ test_description='Test fsck --lost-found' test_expect_success setup ' git config core.logAllRefUpdates 0 && - : > file1 && + echo x > file1 && git add file1 && test_tick && git commit -m initial && @@ -18,18 +18,21 @@ test_expect_success setup ' test_tick && git commit -m second && echo 3 > file3 && - git add file3 + : > file4 && + git add file3 file4 ' test_expect_success 'lost and found something' ' git rev-parse HEAD > lost-commit && - git rev-parse :file3 > lost-other && + git rev-parse :file3 > lost-other3 && + git rev-parse :file4 > lost-other4 && test_tick && git reset --hard HEAD^ && git fsck --lost-found && - test 2 = $(ls .git/lost-found/*/* | wc -l) && + test 3 = $(ls .git/lost-found/*/* | wc -l) && test -f .git/lost-found/commit/$(cat lost-commit) && - test -f .git/lost-found/other/$(cat lost-other) + test -f .git/lost-found/other/$(cat lost-other3) && + test -f .git/lost-found/other/$(cat lost-other4) ' test_done -- 1.7.6.2 ``` ## Sverre Rabbelier, 2011-09-11 16:03 Subject: Re: [PATCH] Support empty blob in fsck --lost-found Message-ID: URL: https://gitlist.dev/e/CAGdFq_hqfqdFyLY%3DKdA_QW5kH8Kjhx8Y18mHEga_Pdv8yzB2wg%40mail.gmail.com In-Reply-To: ``` Heya, On Sun, Sep 11, 2011 at 17:40, BJ Hargrave wrote: > fsck --lost-found died when attempting to write out the empty blob. > Avoid calling fwrite when the blob size is zero since the call to > fwrite returns 0 objects written which fails the check and caused > fsck to die. Now we don't die at all if a 0-byte file couldn't be written. Shouldn't we check errno or something? -- Cheers, Sverre Rabbelier ``` ## BJ Hargrave, 2011-09-11 16:20 Subject: Re: [PATCH] Support empty blob in fsck --lost-found Message-ID: URL: https://gitlist.dev/e/E6A02216-CA67-4B66-AA8F-6DDE8AF7DF3A%40bjhargrave.com In-Reply-To: ``` On Sep 11, 2011, at 12:03 , Sverre Rabbelier wrote: > Heya, > > On Sun, Sep 11, 2011 at 17:40, BJ Hargrave wrote: >> fsck --lost-found died when attempting to write out the empty blob. >> Avoid calling fwrite when the blob size is zero since the call to >> fwrite returns 0 objects written which fails the check and caused >> fsck to die. > > Now we don't die at all if a 0-byte file couldn't be written. > Shouldn't we check errno or something? > You 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. > -- > Cheers, > > Sverre Rabbelier -- BJ Hargrave ``` ## Sverre Rabbelier, 2011-09-11 16:21 Subject: Re: [PATCH] Support empty blob in fsck --lost-found Message-ID: URL: https://gitlist.dev/e/CAGdFq_iLnCtXWJsiYLatzyi3bjz1drv1kmM54y8mMkNaU8b33A%40mail.gmail.com In-Reply-To: ``` Heya, On Sun, Sep 11, 2011 at 18:20, BJ Hargrave wrote: > You 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. Works for me :). -- Cheers, Sverre Rabbelier ``` ## Junio C Hamano, 2011-09-11 20:43 Subject: Re: [PATCH] Support empty blob in fsck --lost-found Message-ID: <7vty8iolnj.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vty8iolnj.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` BJ Hargrave writes: > diff --git a/builtin/fsck.c b/builtin/fsck.c > index 5ae0366..ad6d713 100644 > --- a/builtin/fsck.c > +++ b/builtin/fsck.c > @@ -232,9 +232,10 @@ static void check_unreachable_object(struct object *obj) > char *buf = read_sha1_file(obj->sha1, > &type, &size); > if (buf) { > - if (fwrite(buf, size, 1, f) != 1) > - die_errno("Could not write '%s'", > - filename); > + if (size > 0) > + if (fwrite(buf, size, 1, f) != 1) > + die_errno("Could not write '%s'", > + filename); Funny. I am sure we fixed a similar breakage elsewhere a few years ago, by swapping the size and nmemb to the calls (i.e. instead of writing one block of "size" bytes, you could write "size" blocks of 1-byte) and making sure fwrite() reports the number of items. IOW if (buf && fwrite(buf, 1, size, f) != size) die_errno("Could not write '%s'", filename); ``` ## BJ Hargrave, 2011-09-11 21:43 Subject: Re: [PATCH] Support empty blob in fsck --lost-found Message-ID: <348F09EE-5EE2-4F3E-B1B1-6FD34BDBD117@bjhargrave.com> URL: https://gitlist.dev/e/348F09EE-5EE2-4F3E-B1B1-6FD34BDBD117%40bjhargrave.com In-Reply-To: <7vty8iolnj.fsf@alter.siamese.dyndns.org> ``` On Sep 11, 2011, at 16:43 , Junio C Hamano wrote: > Funny. > > I am sure we fixed a similar breakage elsewhere a few years ago, by > swapping the size and nmemb to the calls (i.e. instead of writing one > block of "size" bytes, you could write "size" blocks of 1-byte) and making > sure fwrite() reports the number of items. IOW > > if (buf && fwrite(buf, 1, size, f) != size) > die_errno("Could not write '%s'", filename); > Do you want me to resubmit the patch using this technique instead of the size > 0 check? -- BJ ``` ## Junio C Hamano, 2011-09-12 01:10 Subject: Re: [PATCH] Support empty blob in fsck --lost-found Message-ID: <7vmxeabm5q.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vmxeabm5q.fsf%40alter.siamese.dyndns.org In-Reply-To: <348F09EE-5EE2-4F3E-B1B1-6FD34BDBD117@bjhargrave.com> ``` BJ Hargrave writes: > On Sep 11, 2011, at 16:43 , Junio C Hamano wrote: > >> Funny. >> >> I am sure we fixed a similar breakage elsewhere a few years ago, by >> swapping the size and nmemb to the calls (i.e. instead of writing one >> block of "size" bytes, you could write "size" blocks of 1-byte) and making >> sure fwrite() reports the number of items. IOW >> >> if (buf && fwrite(buf, 1, size, f) != size) >> die_errno("Could not write '%s'", filename); >> > > Do you want me to resubmit the patch using this technique instead of the size > 0 check? Not really. I am not sure when/why we would try to write an empty blob out to begin with... ```