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

From: Linus Torvalds <torvalds@osdl.org>
Date: 2006-08-27 08:13:00
On Sat, 26 Aug 2006, Jonas Fonseca wrote:
> 
> 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.

> @@ -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
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Received on Sun Aug 27 08:13:54 2006

This archive was generated by hypermail 2.1.8 : 2006-08-27 08:14:31 EST