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

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

[Nicolas Pitre]

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

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

[Marco Costalba]

On Dec 11, 2007 9:27 PM, Nicolas Pitre <nico@cam.org> wrote:
>
> 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 ;-)

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

[Alex Riesen]

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.

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

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

[Nicolas Pitre]

On Tue, 11 Dec 2007, Alex Riesen wrote:

> 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

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

[Nicolas Pitre]

On Tue, 11 Dec 2007, Nicolas Pitre wrote:

> 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

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

[Alex Riesen]

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.

> 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

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

[Nicolas Pitre]

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.


Nicolas

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

[Junio C Hamano]

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.

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.

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

[David Kastrup]

Junio C Hamano <gitster@pobox.com> writes:

> 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

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

[Junio C Hamano]

David Kastrup <dak@gnu.org> writes:

> 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(...);