summaryrefslogtreecommitdiff
path: root/config.c
diff options
context:
space:
mode:
authorJeff King <peff@peff.net>2017-09-13 13:16:03 -0400
committerJunio C Hamano <gitster@pobox.com>2017-09-14 15:17:59 +0900
commit06f46f237afa823c0a2775e60ed8fbd80e7c751f (patch)
tree2f2a7fc1ad3329b7c2f7869700b0097b2d14b218 /config.c
parent68a423ab3e5c160e1162382d2ef0831039b298d4 (diff)
downloadgit-06f46f237afa823c0a2775e60ed8fbd80e7c751f.tar.gz
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 'config.c')
-rw-r--r--config.c4
1 files changed, 2 insertions, 2 deletions
diff --git a/config.c b/config.c
index 6219086a2b..90b699575b 100644
--- a/config.c
+++ b/config.c
@@ -2565,7 +2565,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
copy_end - copy_begin) < 0)
goto write_err_out;
if (new_line &&
- write_str_in_full(fd, "\n") != 1)
+ write_str_in_full(fd, "\n") < 0)
goto write_err_out;
}
copy_begin = store.offset[i];
@@ -2796,7 +2796,7 @@ int git_config_rename_section_in_file(const char *config_filename,
if (remove)
continue;
length = strlen(output);
- if (write_in_full(out_fd, output, length) != length) {
+ if (write_in_full(out_fd, output, length) < 0) {
ret = write_error(get_lock_file_path(lock));
goto out;
}