summaryrefslogtreecommitdiff
path: root/builtin/unpack-file.c
diff options
context:
space:
mode:
authorJeff King <peff@peff.net>2017-09-13 17:16:03 (GMT)
committerJunio C Hamano <gitster@pobox.com>2017-09-14 06:17:59 (GMT)
commit06f46f237afa823c0a2775e60ed8fbd80e7c751f (patch)
tree2f2a7fc1ad3329b7c2f7869700b0097b2d14b218 /builtin/unpack-file.c
parent68a423ab3e5c160e1162382d2ef0831039b298d4 (diff)
downloadgit-06f46f237afa823c0a2775e60ed8fbd80e7c751f.zip
git-06f46f237afa823c0a2775e60ed8fbd80e7c751f.tar.gz
git-06f46f237afa823c0a2775e60ed8fbd80e7c751f.tar.bz2
avoid "write_in_full(fd, buf, len) != len" pattern
The return value of write_in_full() is either "-1", or the requested number of bytes[1]. If we make a partial write before seeing an error, we still return -1, not a partial value. This goes back to f6aa66cb95 (write_in_full: really write in full or return error on disk full., 2007-01-11). So checking anything except "was the return value negative" is pointless. And there are a couple of reasons not to do so: 1. It can do a funny signed/unsigned comparison. If your "len" is signed (e.g., a size_t) then the compiler will promote the "-1" to its unsigned variant. This works out for "!= len" (unless you really were trying to write the maximum size_t bytes), but is a bug if you check "< len" (an example of which was fixed recently in config.c). We should avoid promoting the mental model that you need to check the length at all, so that new sites are not tempted to copy us. 2. Checking for a negative value is shorter to type, especially when the length is an expression. 3. Linus says so. In d34cf19b89 (Clean up write_in_full() users, 2007-01-11), right after the write_in_full() semantics were changed, he wrote: I really wish every "write_in_full()" user would just check against "<0" now, but this fixes the nasty and stupid ones. Appeals to authority aside, this makes it clear that writing it this way does not have an intentional benefit. It's a historical curiosity that we never bothered to clean up (and which was undoubtedly cargo-culted into new sites). So let's convert these obviously-correct cases (this includes write_str_in_full(), which is just a wrapper for write_in_full()). [1] A careful reader may notice there is one way that write_in_full() can return a different value. If we ask write() to write N bytes and get a return value that is _larger_ than N, we could return a larger total. But besides the fact that this would imply a totally broken version of write(), it would already invoke undefined behavior. Our internal remaining counter is an unsigned size_t, which means that subtracting too many byte will wrap it around to a very large number. So we'll instantly begin reading off the end of the buffer, trying to write gigabytes (or petabytes) of data. Signed-off-by: Jeff King <peff@peff.net> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Diffstat (limited to 'builtin/unpack-file.c')
-rw-r--r--builtin/unpack-file.c2
1 files changed, 1 insertions, 1 deletions
diff --git a/builtin/unpack-file.c b/builtin/unpack-file.c
index 73f1334..672a54f 100644
--- a/builtin/unpack-file.c
+++ b/builtin/unpack-file.c
@@ -15,7 +15,7 @@ static char *create_temp_file(unsigned char *sha1)
xsnprintf(path, sizeof(path), ".merge_file_XXXXXX");
fd = xmkstemp(path);
- if (write_in_full(fd, buf, size) != size)
+ if (write_in_full(fd, buf, size) < 0)
die_errno("unable to write temp-file");
close(fd);
return path;