[PATCH] Support empty blob in fsck --lost-found

[PATCH] Support empty blob in fsck --lost-found

[BJ Hargrave]

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 <bj@bjhargrave.com>
---
 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

Re: [PATCH] Support empty blob in fsck --lost-found

[Sverre Rabbelier]

Heya,

On Sun, Sep 11, 2011 at 17:40, BJ Hargrave <bj@bjhargrave.com> 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

Re: [PATCH] Support empty blob in fsck --lost-found

[BJ Hargrave]

On Sep 11, 2011, at 12:03 , Sverre Rabbelier wrote:

> Heya,
> 
> On Sun, Sep 11, 2011 at 17:40, BJ Hargrave <bj@bjhargrave.com> 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

Re: [PATCH] Support empty blob in fsck --lost-found

[Sverre Rabbelier]

Heya,

On Sun, Sep 11, 2011 at 18:20, BJ Hargrave <bj@bjhargrave.com> 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

Re: [PATCH] Support empty blob in fsck --lost-found

[Junio C Hamano]

BJ Hargrave <bj@bjhargrave.com> 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);

Re: [PATCH] Support empty blob in fsck --lost-found

[BJ Hargrave]

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

Re: [PATCH] Support empty blob in fsck --lost-found

[Junio C Hamano]

BJ Hargrave <bj@bjhargrave.com> 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...