patchdir: do all size checks before seeking back and fix file closing

Signed-off-by: Jonas Fonseca <fonseca@diku.dk>
---
 dir.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Jonas Fonseca wrote:

Could you explain that piece? You now only close "fd" if it's zero (stdin) or negative (invalid). The old code (close if its >=0) make more sense.

-Mitch

Mitchell Blank Jr <mitch@sfgoth.com> wrote Sat, Aug 26, 2006:

Ah, yes, sorry for the noise. I read the old code as (fd <= 0).

On Sat, 26 Aug 2006, Jonas Fonseca wrote:

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.

That's wrong. 

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

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

instead.

Junio: I realize that you claim that you learnt that syntax from an 
authorative source, but he was _wrong_. Doing the constant first is more 
likely to cause bugs, rather than less. Compilers will warn about the

	if (x = 0)
		..

kind of bug, and putting the constant first just confuses humans.

It's more important to _not_ confuse humans than it is to try to avoid an uncommon error that compilers can and do warn about anyway.

			Linus

Linus Torvalds wrote:

Well, perhaps except checking if variable is in given range, e.g.

        if (0 <= x && x <= 5)
 

Linus Torvalds <torvalds@osdl.org> writes:

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.

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 >).

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

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 ;-).

Junio C Hamano <junkio@cox.net> writes:

s/comparison/constant/; I cannot spell.

On Sat, 26 Aug 2006, Junio C Hamano wrote:

Ahh. A number of people do the "0 == x" thing, because they want to be caught if they use "=" instead of "==" by mistake. I thought it was the same thing.

To anybody who has _ever_ done any math at all, it makes no sense at all. 

You _always_ put constants on the right-hand side (or, possibly last on the left-hand side, in order to make the right-hand side be "0").

Similarly, if you say it out loud, you'd always say "if 'x' is larger than or equal to zero", not "if zero is smaller or less than 'x'". That's because "zero" obviously never varies, so you'd never talk about "zero" being compared to anything else.

The only exception would be the mathematical "0 < x < 10" kind of thing, which some languages (not C, of course) allows in that form. I can imagine that people would just do that as "0 < x && x < 10" just to keep the C form as close to the mathematical form, although I would at least personally do it as

	if (x > 0 &&
	    x < 10)

especially if I ever needed to write it that way on multiple lines due to some of the expressions being more complicated.

		Linus

Linus Torvalds <torvalds@osdl.org> writes:

The rest is repeating what you said 15 months ago, so I did not quote that part, but interested parties can follow this thread:

http://thread.gmane.org/gmane.comp.version-control.git/3907/focus=4126

Signed-off-by: Jonas Fonseca <fonseca@diku.dk>
---

 dir.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

Junio C Hamano <junkio@cox.net> wrote Sat, Aug 26, 2006:

I take it that you want this change, so here's a little addition to the "use X instead of Y" series.