Re: [PATCH] dir: do all size checks before seeking back and fix file closing

From: Junio C Hamano <junkio@cox.net>
Date: Aug 27, 2006, 00:07 UTC
Message-ID: <7v4pvz11o6.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.64.0608261509290.11811@g5.osdl.org>

Linus Torvalds <torvalds@osdl.org> writes:

> I really think you'd be better off rewriting that to use "fstat()" 
> instead. I don't know why it uses two lseek's, but it's wrong, and looks 
> like some bad habit Junio picked up at some point.

I think the code was written to avoid getting confused by unseekable input (pipes) but was done in early morning before the first shot of caffeine.

> Now, admittedly it's wrong because another bad habit Junio picked up 
> (doing comparisons with constants in the wrong order)

I think you misunderstand the rationale used to encourage the comparison used there. It does not have anything to do with having comparison on the left.

The comparison order is done in textual order. You list smaller things on the left and then larger things on the right (iow, you almost never use >= or >).

> Junio: I realize that you claim that you learnt that syntax from an 
> authorative source, but he was _wrong_....

This does not come from any authoritative source, but I picked it up because I felt it made a lot of sense.

> ... Doing the constant first is more likely to cause bugs,
> rather than less.

That's a funny thing to say, because I was about to send out a comment that touches this exact topic.

I spotted the bug the patch was trying to introduce right away _because_ the original comparison was written in textual order.

The patch changed the comparison operator which first confused me for a handful seconds, and then after I swapped everything in my head to read as

	if (fd <= 0)
        	close(fd);

it became blatantly obvious it was a bogus change. In the message I was about to send out, I would have said "a fine example that using texual order comparison consistently avoids bugs". So it is really relative to what you are used to.

Get used to it, please ;-).