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

From: Linus Torvalds <torvalds@osdl.org>
Date: Aug 26, 2006, 22:13 UTC
Message-ID: <Pine.LNX.4.64.0608261509290.11811@g5.osdl.org>
In-Reply-To: <20060826141709.GC11601@diku.dk>

On Sat, 26 Aug 2006, Jonas Fonseca wrote:

Show 15 quoted lines

> 
> diff --git a/dir.c b/dir.c
> index d53d48f..ff8a2fb 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -122,11 +122,11 @@ static int add_excludes_from_file_1(cons
>  	size = lseek(fd, 0, SEEK_END);
>  	if (size < 0)
>  		goto err;
> -	lseek(fd, 0, SEEK_SET);
>  	if (size == 0) {
>  		close(fd);
>  		return 0;
>  	}
> +	lseek(fd, 0, SEEK_SET);

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.

Show 7 quoted lines

> @@ -146,7 +146,7 @@ static int add_excludes_from_file_1(cons
>  	return 0;
>  
>   err:
> -	if (0 <= fd)
> +	if (0 >= fd)
>  		close(fd);

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