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 <p...@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 <jrnie...@gmail.com>
