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>

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index cabdc55e0933adb09d459a5a0747c718fd2c7d1e..01dea59f5851158c8dff6a055f5cd43d763e84fc 100644 (file)
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -742,7 +742,7 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,
                size_t n;
                if (feed(feed_state, &buf, &n))
                        break;
-               if (write_in_full(proc.in, buf, n) != n)
+               if (write_in_full(proc.in, buf, n) < 0)
                        break;
        }
        close(proc.in);

diff --git a/builtin/rerere.c b/builtin/rerere.c
index ffb66e29073c07c43197ae34f2c31d3b9b610903..0bc40298c2417a6d1f25aafb4b3577243e985c57 100644 (file)
--- a/builtin/rerere.c
+++ b/builtin/rerere.c
@@ -18,7 +18,7 @@ static int outf(void *dummy, mmbuffer_t *ptr, int nbuf)
 {
        int i;
        for (i = 0; i < nbuf; i++)
-               if (write_in_full(1, ptr[i].ptr, ptr[i].size) != ptr[i].size)
+               if (write_in_full(1, ptr[i].ptr, ptr[i].size) < 0)
                        return -1;
        return 0;
 }

diff --git a/builtin/unpack-file.c b/builtin/unpack-file.c
index 73f133419167840d150300faa696bc32027cd02f..672a54fcdf62f147b0497c690cb7daed07c7534b 100644 (file)
--- 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;

diff --git a/config.c b/config.c
index 6219086a2ba46a36f3284b66dfc96ed78d51e630..90b699575b22512eb9b8f59083917b2bcbfbef0a 100644 (file)
--- 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;
                }

diff --git a/diff.c b/diff.c
index 9c382580306e340ed6333f96bc4919c4c507a7b9..6f58bca2f9256ae4ff413db5a13eb249866945e5 100644 (file)
--- a/diff.c
+++ b/diff.c
@@ -2975,7 +2975,7 @@ static void prep_temp_blob(const char *path, struct diff_tempfile *temp,
                blob = buf.buf;
                size = buf.len;
        }
-       if (write_in_full(fd, blob, size) != size)
+       if (write_in_full(fd, blob, size) < 0)
                die_errno("unable to write temp-file");
        close_tempfile(&temp->tempfile);
        temp->name = get_tempfile_path(&temp->tempfile);

diff --git a/fast-import.c b/fast-import.c
index a959161b4631e134602aa9b5502105ee5c5bcfb8..395524039b97d3132e4f37f5a066c3f2f4d3a0b6 100644 (file)
--- a/fast-import.c
+++ b/fast-import.c
@@ -2951,7 +2951,7 @@ static void parse_reset_branch(const char *arg)
 
 static void cat_blob_write(const char *buf, unsigned long size)
 {
-       if (write_in_full(cat_blob_fd, buf, size) != size)
+       if (write_in_full(cat_blob_fd, buf, size) < 0)
                die_errno("Write to frontend failed");
 }
 

diff --git a/http-backend.c b/http-backend.c
index 519025d2c3d944afb6fdd6ca1f3b091827a67272..5daff8ccaba3d9986e1d4073d0e142c90c01665d 100644 (file)
--- a/http-backend.c
+++ b/http-backend.c
@@ -357,7 +357,7 @@ static void inflate_request(const char *prog_name, int out, int buffer_input)
                                die("zlib error inflating request, result %d", ret);
 
                        n = stream.total_out - cnt;
-                       if (write_in_full(out, out_buf, n) != n)
+                       if (write_in_full(out, out_buf, n) < 0)
                                die("%s aborted reading request", prog_name);
                        cnt += n;
 
@@ -378,7 +378,7 @@ static void copy_request(const char *prog_name, int out)
        ssize_t n = read_request(0, &buf);
        if (n < 0)
                die_errno("error reading request body");
-       if (write_in_full(out, buf, n) != n)
+       if (write_in_full(out, buf, n) < 0)
                die("%s aborted reading request", prog_name);
        close(out);
        free(buf);

diff --git a/ll-merge.c b/ll-merge.c
index 9fb855a90030e07e28c1ae9d0994006da22cd4ed..a6ad2ec12dc9c1ece81f120196c81fd259e40eb6 100644 (file)
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -154,7 +154,7 @@ static void create_temp(mmfile_t *src, char *path, size_t len)
 
        xsnprintf(path, len, ".merge_file_XXXXXX");
        fd = xmkstemp(path);
-       if (write_in_full(fd, src->ptr, src->size) != src->size)
+       if (write_in_full(fd, src->ptr, src->size) < 0)
                die_errno("unable to write temp-file");
        close(fd);
 }

diff --git a/read-cache.c b/read-cache.c
index acfb028f480b5545aa82d5c0533fe8bdfc581dfd..5e6d24d44450d9408b461f05cf55b51035ae11dc 100644 (file)
--- a/read-cache.c
+++ b/read-cache.c
@@ -1921,7 +1921,7 @@ static int ce_write_flush(git_SHA_CTX *context, int fd)
        unsigned int buffered = write_buffer_len;
        if (buffered) {
                git_SHA1_Update(context, write_buffer, buffered);
-               if (write_in_full(fd, write_buffer, buffered) != buffered)
+               if (write_in_full(fd, write_buffer, buffered) < 0)
                        return -1;
                write_buffer_len = 0;
        }
@@ -1970,7 +1970,7 @@ static int ce_flush(git_SHA_CTX *context, int fd, unsigned char *sha1)
 
        /* Flush first if not enough space for SHA1 signature */
        if (left + 20 > WRITE_BUFFER_SIZE) {
-               if (write_in_full(fd, write_buffer, left) != left)
+               if (write_in_full(fd, write_buffer, left) < 0)
                        return -1;
                left = 0;
        }
@@ -1979,7 +1979,7 @@ static int ce_flush(git_SHA_CTX *context, int fd, unsigned char *sha1)
        git_SHA1_Final(write_buffer + left, context);
        hashcpy(sha1, write_buffer + left);
        left += 20;
-       return (write_in_full(fd, write_buffer, left) != left) ? -1 : 0;
+       return (write_in_full(fd, write_buffer, left) < 0) ? -1 : 0;
 }
 
 static void ce_smudge_racily_clean_entry(struct cache_entry *ce)

diff --git a/refs.c b/refs.c
index ea2b9f84f8d3fab5452105f10f4492d672cd1d90..a5abca04b7888e1813942136955f0f7dd25efaa9 100644 (file)
--- a/refs.c
+++ b/refs.c
@@ -592,7 +592,7 @@ static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
                }
        }
 
-       if (write_in_full(fd, buf.buf, buf.len) != buf.len) {
+       if (write_in_full(fd, buf.buf, buf.len) < 0) {
                strbuf_addf(err, "could not write to '%s'", filename);
                rollback_lock_file(&lock);
                goto done;

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0404f2c2333c0fadeb57d225deedc6f0d4afadad..924e8537f3e4a6cf3cd581f121c37688150531d0 100644 (file)
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2118,8 +2118,8 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
                return -1;
        }
        fd = get_lock_file_fd(lock->lk);
-       if (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||
-           write_in_full(fd, &term, 1) != 1 ||
+       if (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) < 0 ||
+           write_in_full(fd, &term, 1) < 0 ||
            close_ref(lock) < 0) {
                strbuf_addf(err,
                            "couldn't write '%s'", get_lock_file_path(lock->lk));
@@ -3338,8 +3338,8 @@ static int files_reflog_expire(struct ref_store *ref_store,
                                        strerror(errno));
                } else if (update &&
                           (write_in_full(get_lock_file_fd(lock->lk),
-                               oid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) != GIT_SHA1_HEXSZ ||
-                           write_str_in_full(get_lock_file_fd(lock->lk), "\n") != 1 ||
+                               oid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) < 0 ||
+                           write_str_in_full(get_lock_file_fd(lock->lk), "\n") < 0 ||
                            close_ref(lock) < 0)) {
                        status |= error("couldn't write %s",
                                        get_lock_file_path(lock->lk));

diff --git a/rerere.c b/rerere.c
index 70634d456cc0b70cdbac1579b82016761252c7d9..51376cf6da3b8ab8f8e0c4f20c307fef9dec8392 100644 (file)
--- a/rerere.c
+++ b/rerere.c
@@ -258,7 +258,7 @@ static int write_rr(struct string_list *rr, int out_fd)
                                    rerere_id_hex(id),
                                    rr->items[i].string, 0);
 
-               if (write_in_full(out_fd, buf.buf, buf.len) != buf.len)
+               if (write_in_full(out_fd, buf.buf, buf.len) < 0)
                        die("unable to write rerere record");
 
                strbuf_release(&buf);

diff --git a/shallow.c b/shallow.c
index 54359d549075b5cfe1670ca979ff3cf5eeedc9d1..e8429a9a845f98f4a73299c044463ce99ebbea93 100644 (file)
--- a/shallow.c
+++ b/shallow.c
@@ -296,7 +296,7 @@ const char *setup_temporary_shallow(const struct oid_array *extra)
        if (write_shallow_commits(&sb, 0, extra)) {
                fd = xmks_tempfile(&temporary_shallow, git_path("shallow_XXXXXX"));
 
-               if (write_in_full(fd, sb.buf, sb.len) != sb.len)
+               if (write_in_full(fd, sb.buf, sb.len) < 0)
                        die_errno("failed to write to %s",
                                  get_tempfile_path(&temporary_shallow));
                close_tempfile(&temporary_shallow);
@@ -321,7 +321,7 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,
                                       LOCK_DIE_ON_ERROR);
        check_shallow_file_for_update();
        if (write_shallow_commits(&sb, 0, extra)) {
-               if (write_in_full(fd, sb.buf, sb.len) != sb.len)
+               if (write_in_full(fd, sb.buf, sb.len) < 0)
                        die_errno("failed to write to %s",
                                  get_lock_file_path(shallow_lock));
                *alternate_shallow_file = get_lock_file_path(shallow_lock);
@@ -368,7 +368,7 @@ void prune_shallow(int show_only)
                                       LOCK_DIE_ON_ERROR);
        check_shallow_file_for_update();
        if (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {
-               if (write_in_full(fd, sb.buf, sb.len) != sb.len)
+               if (write_in_full(fd, sb.buf, sb.len) < 0)
                        die_errno("failed to write to %s",
                                  get_lock_file_path(&shallow_lock));
                commit_lock_file(&shallow_lock);

diff --git a/t/helper/test-delta.c b/t/helper/test-delta.c
index 59937dc1be1c4f0b3d80e3ef3a86e09bff3703b6..591730adc4f3940940fdb4691da0afb81648e353 100644 (file)
--- a/t/helper/test-delta.c
+++ b/t/helper/test-delta.c
@@ -69,7 +69,7 @@ int cmd_main(int argc, const char **argv)
        }
 
        fd = open (argv[4], O_WRONLY|O_CREAT|O_TRUNC, 0666);
-       if (fd < 0 || write_in_full(fd, out_buf, out_size) != out_size) {
+       if (fd < 0 || write_in_full(fd, out_buf, out_size) < 0) {
                perror(argv[4]);
                return 1;
        }

diff --git a/transport-helper.c b/transport-helper.c
index 33cff38cc0b9b4a2d05b26544c9edda9caa33e9a..2128a32f1cd11b96b63c03aa4e38cb43e08fc546 100644 (file)
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -44,8 +44,7 @@ static void sendline(struct helper_data *helper, struct strbuf *buffer)
 {
        if (debug)
                fprintf(stderr, "Debug: Remote helper: -> %s", buffer->buf);
-       if (write_in_full(helper->helper->in, buffer->buf, buffer->len)
-               != buffer->len)
+       if (write_in_full(helper->helper->in, buffer->buf, buffer->len) < 0)
                die_errno("Full write to remote helper failed");
 }
 
@@ -74,7 +73,7 @@ static void write_constant(int fd, const char *str)
 {
        if (debug)
                fprintf(stderr, "Debug: Remote helper: -> %s", str);
-       if (write_in_full(fd, str, strlen(str)) != strlen(str))
+       if (write_in_full(fd, str, strlen(str)) < 0)
                die_errno("Full write to remote helper failed");
 }
 

diff --git a/wrapper.c b/wrapper.c
index 36630e5d1855a41407e217e610aa1f293288abe1..61aba0b5c1bece4aad04bee649accae990a8ce18 100644 (file)
--- a/wrapper.c
+++ b/wrapper.c
@@ -652,7 +652,7 @@ int xsnprintf(char *dst, size_t max, const char *fmt, ...)
 void write_file_buf(const char *path, const char *buf, size_t len)
 {
        int fd = xopen(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);
-       if (write_in_full(fd, buf, len) != len)
+       if (write_in_full(fd, buf, len) < 0)
                die_errno(_("could not write to %s"), path);
        if (close(fd))
                die_errno(_("could not close %s"), path);