threads / patch / 28357

patchSupport empty blob in fsck --lost-found

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

## tl;dr

7 messages between Sep 11, 2011 and Sep 12, 2011. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

BJ Hargrave· Sep 11, 2011, 15:40 UTC · lore

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(-)
Show changes to 2 files +12 −8

builtin/fsck.c, t/t1420-lost-found.sh

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· Sep 11, 2011, 16:03 UTC · re: BJ Hargrave · lore

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

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
BJ Hargrave· Sep 11, 2011, 16:20 UTC · re: Sverre Rabbelier · lore

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

On Sep 11, 2011, at 12:03 , Sverre Rabbelier wrote:
Show 11 quoted lines
> 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
Sverre Rabbelier· Sep 11, 2011, 16:21 UTC · re: BJ Hargrave · lore

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

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
Junio C Hamano· Sep 11, 2011, 20:43 UTC · re: BJ Hargrave · lore

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

BJ Hargrave <bj@bjhargrave.com> writes:
Show 15 quoted lines
> 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· Sep 11, 2011, 21:43 UTC · re: Junio C Hamano · lore

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

On Sep 11, 2011, at 16:43 , Junio C Hamano wrote:
Show 10 quoted lines
> 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· Sep 12, 2011, 01:10 UTC · re: BJ Hargrave · lore

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

BJ Hargrave <bj@bjhargrave.com> writes:
Show 14 quoted lines
> 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...

← back to recent threads