On Wed, Sep 27, 2017 at 03:59:15PM +0900, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> writes:
>
> > diff --git a/csum-file.c b/csum-file.c
> > index a172199e44..2adae04073 100644
> > --- a/csum-file.c
> > +++ b/csum-file.c
> > @@ -19,7 +19,7 @@ static void flush(struct sha1file *f, const void *buf, unsigned int count)
> >
> > if (ret < 0)
> > die_errno("%s: sha1 file read error", f->name);
> > - if (ret < count)
> > + if (ret != count)
> > die("%s: sha1 file truncated", f->name);
>
> I personally find that this "ret < count" that comes after checking
> if ret is negative expresses what it is checking in a more natural
> way than "ret must be exactly count".
>
> It is not a big deal, as either one needs to be read with an
> understanding that read_in_full() would read at most "count" bytes
> to see if the right condition is being checked to declare
> "truncated" anyway. But I somehow find
>
> ret = read up to count
> if (ret < 0)
> read failed
> if (ret < count)
> we failed to read as much as expected
>
> a bit more natural.
I actually do, too, and I wouldn't terribly mind to drop these. Mostly I
didn't want people blindly copying only the second half. The fact that
the second condition is OK only because the first condition is present
is somewhat subtle.
I also wondered if this would shut up -Wsign-compare. But it doesn't
seem to complain about the originals, which kind of surprises me.
-Peff
