Re: [PATCH v2 3/7] prefer "!=" when checking read_in_full() result

From: Jeff King <peff@peff.net>
Date: Sep 27, 2017, 07:09 UTC
Message-ID: <20170927070959.snjdfrunaybo2buc@sigill.intra.peff.net>
In-Reply-To: <xmqqr2us4ofw.fsf@gitster.mtv.corp.google.com>

On Wed, Sep 27, 2017 at 03:59:15PM +0900, Junio C Hamano wrote:

Show 30 quoted lines

> Jeff King <peff@peff.net> 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