threads / discuss / 11244

'git fast-export' is crashing on the gcc repo

Subject: 'git fast-export' is crashing on the gcc repo

## tl;dr

10 messages between Dec 11, 2007 and Dec 12, 2007.

replies: 9people: 5as markdown or json

Nicolas Pitre· Dec 11, 2007, 20:27 UTC · lore
Simply doing something like:
$ git fast-export --all > /dev/null
results in:
fatal: Could not write blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
Since this is extremely enlightening, I patched it as follows:
diff --git a/builtin-fast-export.c b/builtin-fast-export.c
index 2136aad..5c7bfe0 100755
--- a/builtin-fast-export.c
+++ b/builtin-fast-export.c
@@ -104,7 +104,8 @@ static void handle_object(const unsigned char *sha1)
 
 	printf("blob\nmark :%d\ndata %lu\n", last_idnum, size);
 	if (fwrite(buf, size, 1, stdout) != 1)
-		die ("Could not write blob %s", sha1_to_hex(sha1));
+		die ("Could not write blob %s: %s",
+		     sha1_to_hex(sha1), strerror(errno));
 	printf("\n");
 
 	show_progress();

And then running it again produced:

fatal: Could not write blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391: Inappropriate ioctl for device

adding to today's confusion.


Nicolas
Marco Costalba· Dec 11, 2007, 20:35 UTC · re: Nicolas Pitre · lore

Re: 'git fast-export' is crashing on the gcc repo

On Dec 11, 2007 9:27 PM, Nicolas Pitre <nico@cam.org> wrote:
Show 7 quoted lines
>
> And then running it again produced:
>
> fatal: Could not write blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391: Inappropriate ioctl for device
>
> adding to today's confusion.
>
GCC repo is turning out to be a very good test case indeed ;-)
Alex Riesen· Dec 11, 2007, 22:01 UTC · re: Nicolas Pitre · lore

[PATCH] Fix git-fast-export for zero-sized blobs

Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
---
 builtin-fast-export.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Nicolas Pitre, Tue, Dec 11, 2007 21:27:33 +0100:
Show 8 quoted lines
> Simply doing something like:
> 
> $ git fast-export --all > /dev/null
> 
> results in:
> 
> fatal: Could not write blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
> 
I get this for my git repo:
	$ git fast-export --all >/dev/null
	fatal: Could not write blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
	$ git cat-file blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 |wc -c
	0
Writing a zero-size blob will surely returns 0.
diff --git a/builtin-fast-export.c b/builtin-fast-export.c
index 2136aad..ef27eee 100755
--- a/builtin-fast-export.c
+++ b/builtin-fast-export.c
@@ -103,7 +103,7 @@ static void handle_object(const unsigned char *sha1)
 	mark_object(object);
 
 	printf("blob\nmark :%d\ndata %lu\n", last_idnum, size);
-	if (fwrite(buf, size, 1, stdout) != 1)
+	if (size && fwrite(buf, size, 1, stdout) != 1)
 		die ("Could not write blob %s", sha1_to_hex(sha1));
 	printf("\n");
 

-- 
1.5.3.7.1177.gb22b
Nicolas Pitre· Dec 11, 2007, 22:20 UTC · re: Alex Riesen · lore

Re: [PATCH] Fix git-fast-export for zero-sized blobs

On Tue, 11 Dec 2007, Alex Riesen wrote:
Show 23 quoted lines
> Signed-off-by: Alex Riesen <raa.lkml@gmail.com>
> ---
>  builtin-fast-export.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> Nicolas Pitre, Tue, Dec 11, 2007 21:27:33 +0100:
> > Simply doing something like:
> > 
> > $ git fast-export --all > /dev/null
> > 
> > results in:
> > 
> > fatal: Could not write blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
> > 
> 
> I get this for my git repo:
> 
> 	$ git fast-export --all >/dev/null
> 	fatal: Could not write blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
> 	$ git cat-file blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 |wc -c
> 	0
> 
> Writing a zero-size blob will surely returns 0.
Indeed.
/me looking at too many piece of code at the same time
Nicolas
Nicolas Pitre· Dec 11, 2007, 22:06 UTC · re: Nicolas Pitre · lore

Re: 'git fast-export' is crashing on the gcc repo

On Tue, 11 Dec 2007, Nicolas Pitre wrote:
Show 30 quoted lines
> Simply doing something like:
> 
> $ git fast-export --all > /dev/null
> 
> results in:
> 
> fatal: Could not write blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
> 
> Since this is extremely enlightening, I patched it as follows:
> 
> diff --git a/builtin-fast-export.c b/builtin-fast-export.c
> index 2136aad..5c7bfe0 100755
> --- a/builtin-fast-export.c
> +++ b/builtin-fast-export.c
> @@ -104,7 +104,8 @@ static void handle_object(const unsigned char *sha1)
>  
>  	printf("blob\nmark :%d\ndata %lu\n", last_idnum, size);
>  	if (fwrite(buf, size, 1, stdout) != 1)
> -		die ("Could not write blob %s", sha1_to_hex(sha1));
> +		die ("Could not write blob %s: %s",
> +		     sha1_to_hex(sha1), strerror(errno));
>  	printf("\n");
>  
>  	show_progress();
> 
> And then running it again produced:
> 
> fatal: Could not write blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391: Inappropriate ioctl for device
> 
> adding to today's confusion.

Well, ignore the above. It seems that most of stdio doesn't set errno so the above is crap.

Even strace doesn't show any error.
But, somehow, the following patch fixes it:
diff --git a/builtin-fast-export.c b/builtin-fast-export.c
index 2136aad..c32a124 100755
--- a/builtin-fast-export.c
+++ b/builtin-fast-export.c
@@ -103,7 +103,7 @@ static void handle_object(const unsigned char *sha1)
 	mark_object(object);
 
 	printf("blob\nmark :%d\ndata %lu\n", last_idnum, size);
-	if (fwrite(buf, size, 1, stdout) != 1)
+	if (fwrite(buf, 1, size, stdout) != size)
 		die ("Could not write blob %s", sha1_to_hex(sha1));
 	printf("\n");
 

Go figure.


Nicolas
Alex Riesen· Dec 12, 2007, 01:38 UTC · re: Nicolas Pitre · lore

Re: 'git fast-export' is crashing on the gcc repo

Nicolas Pitre, Tue, Dec 11, 2007 23:06:42 +0100:
> 
> Well, ignore the above.  It seems that most of stdio doesn't set errno 
> so the above is crap.
>

Well, it had no reason to in this case. It's not an error. It does not even have to do a syscall.

Show 10 quoted lines
> diff --git a/builtin-fast-export.c b/builtin-fast-export.c
> index 2136aad..c32a124 100755
> --- a/builtin-fast-export.c
> +++ b/builtin-fast-export.c
> @@ -103,7 +103,7 @@ static void handle_object(const unsigned char *sha1)
>  	mark_object(object);
>  
>  	printf("blob\nmark :%d\ndata %lu\n", last_idnum, size);
> -	if (fwrite(buf, size, 1, stdout) != 1)
> +	if (fwrite(buf, 1, size, stdout) != size)
That's a probable syscall which could be spared
Nicolas Pitre· Dec 12, 2007, 01:45 UTC · re: Alex Riesen · lore

Re: 'git fast-export' is crashing on the gcc repo

On Wed, 12 Dec 2007, Alex Riesen wrote:
Show 8 quoted lines
> Nicolas Pitre, Tue, Dec 11, 2007 23:06:42 +0100:
> > 
> > Well, ignore the above.  It seems that most of stdio doesn't set errno 
> > so the above is crap.
> > 
> 
> Well, it had no reason to in this case. It's not an error.
> It does not even have to do a syscall.
Which is why I later agreed with your patch.
Nicolas
Junio C Hamano· Dec 12, 2007, 08:32 UTC · re: Nicolas Pitre · lore

Re: 'git fast-export' is crashing on the gcc repo

Nicolas Pitre <nico@cam.org> writes:
Show 12 quoted lines
> On Wed, 12 Dec 2007, Alex Riesen wrote:
>
>> Nicolas Pitre, Tue, Dec 11, 2007 23:06:42 +0100:
>> > 
>> > Well, ignore the above.  It seems that most of stdio doesn't set errno 
>> > so the above is crap.
>> > 
>> 
>> Well, it had no reason to in this case. It's not an error.
>> It does not even have to do a syscall.
>
> Which is why I later agreed with your patch.

Still, I like your swapping of size and nmemb parameters, regardless of the "don't bother calling fwrite(3) if size is zero" fix.

We are writing 1 element of size n-byte and expecting the call to return
1.  It may be argued that writing one element of size 0-byte should
always write 1 element (without actually having to go down to write(2),
obviously) successfully and returning 0 from fwrite(3) is a bug ;-)

No, I am just kidding. I checked with POSIX and it clearly says it should return 0 if size or nmemb is zero.

David Kastrup· Dec 12, 2007, 08:46 UTC · re: Junio C Hamano · lore

Re: 'git fast-export' is crashing on the gcc repo

Junio C Hamano <gitster@pobox.com> writes:
Show 17 quoted lines
> Nicolas Pitre <nico@cam.org> writes:
>
>> On Wed, 12 Dec 2007, Alex Riesen wrote:
>>
>>> Nicolas Pitre, Tue, Dec 11, 2007 23:06:42 +0100:
>>> > 
>>> > Well, ignore the above.  It seems that most of stdio doesn't set errno 
>>> > so the above is crap.
>>> > 
>>> 
>>> Well, it had no reason to in this case. It's not an error.
>>> It does not even have to do a syscall.
>>
>> Which is why I later agreed with your patch.
>
> Still, I like your swapping of size and nmemb parameters, regardless
> of the "don't bother calling fwrite(3) if size is zero" fix.

I don't. Far too obscure, looks like an unintentional wart waiting to be corrected.

I think that the explicit test is the way to go here.
-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum
Junio C Hamano· Dec 12, 2007, 08:56 UTC · re: David Kastrup · lore

Re: 'git fast-export' is crashing on the gcc repo

David Kastrup <dak@gnu.org> writes:
Show 7 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> ...
>> Still, I like your swapping of size and nmemb parameters, regardless
>> of the "don't bother calling fwrite(3) if size is zero" fix.
>
> I don't.  Far too obscure, looks like an unintentional wart waiting to
> be corrected.

Oh, I did not mean it in the sense that would be a bugfix, but in the sense that we are writing N instances of 1 byte, not 1 instance of N byte blob, and should express size and nmemb parameters to fwrite(3) as such. IOW, I would have preferred:

	if (size && fwrite(buf, 1, size, stdout) != size)
        	barf(...);

← back to recent threads