Clean up sha1 file writing

Clean up sha1 file writing

[Linus Torvalds]

This cleans up and future-proofs the sha1 file writing in sha1_file.c.

In particular, instead of doing a simple "write()" call and just verifying 
that it succeeds (or - as in one place - just assuming it does), it uses 
"write_buffer()" to write data to the file descriptor while correctly 
checking for partial writes, EINTR etc.

It also splits up write_sha1_to_fd() to be a lot more readable: if we need 
to re-create the compressed object, we do so in a separate helper 
function, making the logic a whole lot more modular and obvious.

Signed-off-by: Linus Torvalds <torvalds@osdl.org>
---

This shouldn't change any behaviour, and it's obviously touching some core 
code, so maybe it's not worth it. On the other hand, from a longer-term 
maintenance standpoint and from a "be much more careful when doing file 
writes" standpoint, I think it's worth it.

The re-write is "obviously correct" (famous last words) and is mostly 
just moving code around and getting rid of a few temporaries that become 
unnecessary as a result.

The patch looks a bit messy: the changes aren't actually that big, but the 
split-up and the resulting re-indentation makes the patch fairly 
unreadable, so the cleanups are more obvious when you look at the 
before-and-after side by side rather than when looking at the unified 
diff..)

diff --git a/sha1_file.c b/sha1_file.c
index 2230010..c2fe7c2 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1399,6 +1399,25 @@ int move_temp_to_file(const char *tmpfil
 	return 0;
 }
 
+static int write_buffer(int fd, const void *buf, size_t len)
+{
+	while (len) {
+		ssize_t size;
+
+		size = write(fd, buf, len);
+		if (!size)
+			return error("file write: disk full");
+		if (size < 0) {
+			if (errno == EINTR || errno == EAGAIN)
+				continue;
+			return error("file write error (%s)", strerror(errno));
+		}
+		len -= size;
+		buf += size;
+	}
+	return 0;
+}
+
 int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned char *returnsha1)
 {
 	int size;
@@ -1465,8 +1484,8 @@ int write_sha1_file(void *buf, unsigned 
 	deflateEnd(&stream);
 	size = stream.total_out;
 
-	if (write(fd, compressed, size) != size)
-		die("unable to write file");
+	if (write_buffer(fd, compressed, size) < 0)
+		die("unable to write sha1 file");
 	fchmod(fd, 0444);
 	close(fd);
 	free(compressed);
@@ -1474,73 +1493,70 @@ int write_sha1_file(void *buf, unsigned 
 	return move_temp_to_file(tmpfile, filename);
 }
 
-int write_sha1_to_fd(int fd, const unsigned char *sha1)
+/*
+ * We need to unpack and recompress the object for writing
+ * it out to a different file.
+ */
+static void *repack_object(const unsigned char *sha1, unsigned long *objsize)
 {
-	ssize_t size;
-	unsigned long objsize;
-	int posn = 0;
-	void *map = map_sha1_file_internal(sha1, &objsize);
-	void *buf = map;
-	void *temp_obj = NULL;
+	size_t size;
 	z_stream stream;
+	unsigned char *unpacked;
+	unsigned long len;
+	char type[20];
+	char hdr[50];
+	int hdrlen;
+	void *buf;
 
-	if (!buf) {
-		unsigned char *unpacked;
-		unsigned long len;
-		char type[20];
-		char hdr[50];
-		int hdrlen;
-		// need to unpack and recompress it by itself
-		unpacked = read_packed_sha1(sha1, type, &len);
+	// need to unpack and recompress it by itself
+	unpacked = read_packed_sha1(sha1, type, &len);
 
-		hdrlen = sprintf(hdr, "%s %lu", type, len) + 1;
+	hdrlen = sprintf(hdr, "%s %lu", type, len) + 1;
 
-		/* Set it up */
-		memset(&stream, 0, sizeof(stream));
-		deflateInit(&stream, Z_BEST_COMPRESSION);
-		size = deflateBound(&stream, len + hdrlen);
-		temp_obj = buf = xmalloc(size);
+	/* Set it up */
+	memset(&stream, 0, sizeof(stream));
+	deflateInit(&stream, Z_BEST_COMPRESSION);
+	size = deflateBound(&stream, len + hdrlen);
+	buf = xmalloc(size);
 
-		/* Compress it */
-		stream.next_out = buf;
-		stream.avail_out = size;
+	/* Compress it */
+	stream.next_out = buf;
+	stream.avail_out = size;
 		
-		/* First header.. */
-		stream.next_in = (void *)hdr;
-		stream.avail_in = hdrlen;
-		while (deflate(&stream, 0) == Z_OK)
-			/* nothing */;
+	/* First header.. */
+	stream.next_in = (void *)hdr;
+	stream.avail_in = hdrlen;
+	while (deflate(&stream, 0) == Z_OK)
+		/* nothing */;
 
-		/* Then the data itself.. */
-		stream.next_in = unpacked;
-		stream.avail_in = len;
-		while (deflate(&stream, Z_FINISH) == Z_OK)
-			/* nothing */;
-		deflateEnd(&stream);
-		free(unpacked);
-		
-		objsize = stream.total_out;
-	}
+	/* Then the data itself.. */
+	stream.next_in = unpacked;
+	stream.avail_in = len;
+	while (deflate(&stream, Z_FINISH) == Z_OK)
+		/* nothing */;
+	deflateEnd(&stream);
+	free(unpacked);
 
-	do {
-		size = write(fd, buf + posn, objsize - posn);
-		if (size <= 0) {
-			if (!size) {
-				fprintf(stderr, "write closed\n");
-			} else {
-				perror("write ");
-			}
-			return -1;
-		}
-		posn += size;
-	} while (posn < objsize);
+	*objsize = stream.total_out;
+	return buf;
+}
 
-	if (map)
-		munmap(map, objsize);
-	if (temp_obj)
-		free(temp_obj);
+int write_sha1_to_fd(int fd, const unsigned char *sha1)
+{
+	int retval;
+	unsigned long objsize;
+	void *buf = map_sha1_file_internal(sha1, &objsize);
 
-	return 0;
+	if (buf) {
+		retval = write_buffer(fd, buf, objsize);
+		munmap(buf, objsize);
+		return retval;
+	}
+
+	buf = repack_object(sha1, &objsize);
+	retval = write_buffer(fd, buf, objsize);    
+	free(buf);
+	return retval;
 }
 
 int write_sha1_from_fd(const unsigned char *sha1, int fd, char *buffer,
@@ -1579,7 +1595,8 @@ int write_sha1_from_fd(const unsigned ch
 				SHA1_Update(&c, discard, sizeof(discard) -
 					    stream.avail_out);
 			} while (stream.avail_in && ret == Z_OK);
-			write(local, buffer, *bufposn - stream.avail_in);
+			if (write_buffer(local, buffer, *bufposn - stream.avail_in) < 0)
+				die("unable to write sha1 file");
 			memmove(buffer, buffer + *bufposn - stream.avail_in,
 				stream.avail_in);
 			*bufposn = stream.avail_in;

Re: Clean up sha1 file writing

[Matthias Lederhofer]

> checking for partial writes
Just out of interest: is this to be safe on any OS or should this
be checked always?

> +		size = write(fd, buf, len);
> +		if (!size)
> +			return error("file write: disk full");
Shouldn't write to a full disk return -1 with ENOSPC?

Re: Clean up sha1 file writing

[Linus Torvalds]

On Wed, 24 May 2006, Matthias Lederhofer wrote:

> > checking for partial writes
>
> Just out of interest: is this to be safe on any OS or should this
> be checked always?

Any POSIX-conformant OS/filesystem should always do a full write for a 
regular file, unless a serious error happens.

HOWEVER. 

In practice, you can get partial writes at least over NFS (hey, it may not 
be posix, but it's _common_) when the filesystem has been mounted soft 
(and/or interruptible). And obviously if your file descriptor isn't a 
regular file, you can easily get partial writes.

Doing the loop is always safe, so it's worth doing it that way.

> > +		size = write(fd, buf, len);
> > +		if (!size)
> > +			return error("file write: disk full");
>
> Shouldn't write to a full disk return -1 with ENOSPC?

In that case, the "size < 0" check will catch it. The "return zero for 
full" case is an alternate error return (it happens for block device files 
at the end, it could happen for other things too). So the "returns zero 
means full" is the portable/safe thing to do.

		Linus

Re: Clean up sha1 file writing

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> ... On the other hand, from a longer-term 
> maintenance standpoint and from a "be much more careful when doing file 
> writes" standpoint, I think it's worth it.
>
> The re-write is "obviously correct" (famous last words) and is mostly 
> just moving code around and getting rid of a few temporaries that become 
> unnecessary as a result.
>
> The patch looks a bit messy: the changes aren't actually that big, but the 
> split-up and the resulting re-indentation makes the patch fairly 
> unreadable, so the cleanups are more obvious when you look at the 
> before-and-after side by side rather than when looking at the unified 
> diff..)

I usually work in text-only terminal, but with the above
warning, I did this:

	git cat-file -p HEAD^:sha1_file.c >/var/tmp/1
        xxdiff /var/tmp/1 sha1_file.c
        
with ignorespace and stuff enabled.  It was very pleasant to
read the changes that way, especially around write_sha1_to_fd()
vs repack_object().  xxdiff is my new friend.

Re: Clean up sha1 file writing

[Linus Torvalds]

On Wed, 24 May 2006, Junio C Hamano wrote:
>
> It was very pleasant to read the changes that way, especially around 
> write_sha1_to_fd() vs repack_object().  xxdiff is my new friend.

I think "kompare" (the KDE diff tool) is nicer.

I'm tolf xxdiff integrates with some of the other SCM tools well (svn and 
tla), so it people use xxdiff, maybe it could support the git way of doing 
things too..

		Linus

Re: Clean up sha1 file writing

[Junio C Hamano]

Linus Torvalds <torvalds@osdl.org> writes:

> On Wed, 24 May 2006, Junio C Hamano wrote:
>>
>> It was very pleasant to read the changes that way, especially around 
>> write_sha1_to_fd() vs repack_object().  xxdiff is my new friend.
>
> I think "kompare" (the KDE diff tool) is nicer.

I'd love to give it a whirl, but aptitude says it will consume
73.5MB diskspace to install it, with download size 22.4MB, which
makes me go ... hmmmm (my machines are currently KDE free so the
above counts slurping in the kdelibs essentials).  

Re: Clean up sha1 file writing

[Sean]

On Wed, 24 May 2006 17:19:29 -0700
Junio C Hamano <junkio@cox.net> wrote:

> Linus Torvalds <torvalds@osdl.org> writes:
> 
> > On Wed, 24 May 2006, Junio C Hamano wrote:
> >>
> >> It was very pleasant to read the changes that way, especially around 
> >> write_sha1_to_fd() vs repack_object().  xxdiff is my new friend.
> >
> > I think "kompare" (the KDE diff tool) is nicer.
> 
> I'd love to give it a whirl, but aptitude says it will consume
> 73.5MB diskspace to install it, with download size 22.4MB, which
> makes me go ... hmmmm (my machines are currently KDE free so the
> above counts slurping in the kdelibs essentials).  
> 

"meld" works great too, and lets you do hunk-level merging etc; very
handy.  The bash <( ... )  feature is quite handy too, it will create
a temp file for you and clean it up afterward.

$ meld sha1_file.c <(git cat-file -p c2f493a4ae:sha1_file.c)

Sean

[PATCH] cat-file: document -p option

[Jeff King]

---
On Wed, May 24, 2006 at 08:31:08PM -0400, Sean wrote:
> $ meld sha1_file.c <(git cat-file -p c2f493a4ae:sha1_file.c)

I had never seen the -p option, and it wasn't in the Documentation. Let
me know if I've gotten its function completely wrong. :)

-Peff

 Documentation/git-cat-file.txt |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt
index 504eb1b..5e9cbf8 100644
--- a/Documentation/git-cat-file.txt
+++ b/Documentation/git-cat-file.txt
@@ -8,12 +8,12 @@ git-cat-file - Provide content or type i
 
 SYNOPSIS
 --------
-'git-cat-file' [-t | -s | -e | <type>] <object>
+'git-cat-file' [-t | -s | -e | -p | <type>] <object>
 
 DESCRIPTION
 -----------
 Provides content or type of objects in the repository. The type
-is required unless '-t' is used to find the object type,
+is required unless '-t' or '-p' is used to find the object type,
 or '-s' is used to find the object size.
 
 OPTIONS
@@ -33,6 +33,9 @@ OPTIONS
 	Suppress all output; instead exit with zero status if <object>
 	exists and is a valid object.
 
+-p::
+	Pretty-print the contents of <object> based on its type.
+
 <type>::
 	Typically this matches the real type of <object> but asking
 	for a type that can trivially be dereferenced from the given
@@ -49,6 +52,8 @@ If '-s' is specified, the size of the <o
 
 If '-e' is specified, no output.
 
+If '-p' is specified, the contents of <object> are pretty-printed.
+
 Otherwise the raw (though uncompressed) contents of the <object> will
 be returned.
 
-- 
1.3.3.g4d548

Re: Clean up sha1 file writing

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

>> I think "kompare" (the KDE diff tool) is nicer.
>
> I'd love to give it a whirl, but aptitude says it will consume
> 73.5MB diskspace to install it, with download size 22.4MB, which
> makes me go ... hmmmm (my machines are currently KDE free so the
> above counts slurping in the kdelibs essentials).  

It indeed is nicer ;-).  I got a good laugh watching it smoothly
move the inserted hunk as I scrolled down.

Re: Clean up sha1 file writing

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> On Wed, 24 May 2006, Matthias Lederhofer wrote:
> 
>>> checking for partial writes
>> Just out of interest: is this to be safe on any OS or should this
>> be checked always?
> 
> Any POSIX-conformant OS/filesystem should always do a full write for a 
> regular file, unless a serious error happens.
> 

Actually, at least Linux won't, if the object is larger than 2 GB on a 64-bit system :)

	-hpa

Re: Clean up sha1 file writing

[Linus Torvalds]

On Fri, 2 Jun 2006, H. Peter Anvin wrote:
> Linus Torvalds wrote:
> > 
> > Any POSIX-conformant OS/filesystem should always do a full write for a
> > regular file, unless a serious error happens.
> > 
> 
> Actually, at least Linux won't, if the object is larger than 2 GB on a 64-bit
> system :)

Yeah, true.

In the end, you should always have the loop. That way, you never have to 
worry about what kind of file descriptor it is, what kind of filesystem 
you're running on, and what the limits of ssize_t might be.

		Linus

Re: Clean up sha1 file writing

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> On Fri, 2 Jun 2006, H. Peter Anvin wrote:
>> Linus Torvalds wrote:
>>> Any POSIX-conformant OS/filesystem should always do a full write for a
>>> regular file, unless a serious error happens.
>>>
>> Actually, at least Linux won't, if the object is larger than 2 GB on a 64-bit
>> system :)
> 
> Yeah, true.
> 
> In the end, you should always have the loop. That way, you never have to 
> worry about what kind of file descriptor it is, what kind of filesystem 
> you're running on, and what the limits of ssize_t might be.
> 

Or if you're getting a SIGWINCH in the middle of it.

Any POSIX system will interrupt the transfer and return a short read on 
receiving a signal.

	-hpa

Re: Clean up sha1 file writing

[Linus Torvalds]

On Sun, 4 Jun 2006, H. Peter Anvin wrote:
> 
> Or if you're getting a SIGWINCH in the middle of it.
> 
> Any POSIX system will interrupt the transfer and return a short read on
> receiving a signal.

Only for interruptible fd's, though, which normally a real "file" won't 
be.

		Linus

Re: Clean up sha1 file writing

[Linus Torvalds]

On Sun, 4 Jun 2006, Linus Torvalds wrote:
> 
> On Sun, 4 Jun 2006, H. Peter Anvin wrote:
> > 
> > Or if you're getting a SIGWINCH in the middle of it.
> > 
> > Any POSIX system will interrupt the transfer and return a short read on
> > receiving a signal.
> 
> Only for interruptible fd's, though, which normally a real "file" won't 
> be.

And _usually_ only if you actually have a SIGWINCH handler.

Although older Linux kernels were broken in this regard. They'd interrupt 
a socket/pipe read or write even for a signal that ended up being ignored.

So it's absolutely the case that having the loop is always the safer thing 
to do, and it's never the _wrong_ thing to do.

		Linus

Re: Clean up sha1 file writing

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> On Sun, 4 Jun 2006, Linus Torvalds wrote:
>> On Sun, 4 Jun 2006, H. Peter Anvin wrote:
>>> Or if you're getting a SIGWINCH in the middle of it.
>>>
>>> Any POSIX system will interrupt the transfer and return a short read on
>>> receiving a signal.
>> Only for interruptible fd's, though, which normally a real "file" won't 
>> be.
> 
> And _usually_ only if you actually have a SIGWINCH handler.
> 
> Although older Linux kernels were broken in this regard. They'd interrupt 
> a socket/pipe read or write even for a signal that ended up being ignored.
> 
> So it's absolutely the case that having the loop is always the safer thing 
> to do, and it's never the _wrong_ thing to do.
> 

Right.  I tend to think of it as implementation-defined the cases where 
it happen to be safe, with the one exception of transactions which are 
smaller than PIPE_BUF that are directed to a pipe.

	-hpa