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

[Jonathan Nieder <jrnieder@gmail.com>]

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 <peff@peff.net>
> ---
>  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 <jrnieder@gmail.com>