Re: [PATCH 1/2] Don't leak file descriptors from unavailable pack files.

From: Junio C Hamano <junkio@cox.net>
Date: Feb 2, 2007, 08:37 UTC
Message-ID: <7vbqkdhs4d.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <20070202080003.GA21094@spearce.org>

"Shawn O. Pearce" <spearce@spearce.org> writes:

> If open_packed_git failed it may have been because the packfile
> actually exists and is readable, but some sort of verification
> did not pass.  In this case open_packed_git left pack_fd filled
> in, as the file descriptor is valid.

Show 5 quoted lines

>  			if (p->pack_fd == -1 && open_packed_git(p)) {
> +				if (p->pack_fd != -1) {
> +					close(p->pack_fd);
> +					p->pack_fd = -1;
> +				}

I agree leaking fd is not nice, but I wonder if that should be dealt with by the caller.

Originally it did not matter as open_packed_git() died, but shouldn't it be closing the fd and marking p->pack_fd with -1, as you made it return instead of die?