Re: [PATCH 3/7] avoid "write_in_full(fd, buf, len) != len" pattern

Jeff King wrote:

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

[...]

  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.

Ok, you convinced me.

Should we add a comment to cache.h as well encouraging this?

[...]

[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.

This footnote just leaves me more confused, since as you mention,
write() never would return a value greater than N.  Are you saying we
need to defend against a broken platform where that isn't true?

Signed-off-by: Jeff King <redacted>
---
 builtin/receive-pack.c | 2 +-
 builtin/rerere.c       | 2 +-
 builtin/unpack-file.c  | 2 +-
 config.c               | 4 ++--
 diff.c                 | 2 +-
 fast-import.c          | 2 +-
 http-backend.c         | 4 ++--
 ll-merge.c             | 2 +-
 read-cache.c           | 6 +++---
 refs.c                 | 2 +-
 refs/files-backend.c   | 8 ++++----
 rerere.c               | 2 +-
 shallow.c              | 6 +++---
 t/helper/test-delta.c  | 2 +-
 transport-helper.c     | 5 ++---
 wrapper.c              | 2 +-
 16 files changed, 26 insertions(+), 27 deletions(-)

All of these look correctly done.

Reviewed-by: Jonathan Nieder <redacted>