Re: [PATCH 2/2] close_lock_file(): new function in the lockfile API

[Junio C Hamano <gitster@pobox.com>]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> My patch does this, though I understand it may take some time to review.
>
> I left the lk->fd unmodified when close() failed in case the caller
> would like to include it in an error message.

But that would bring us back to the same double-close issue,
wouldn't it?

	if (close_lock_file(lock))
		die("Oops, failed to close fd %d", lock->fd);

is not enough.  You need to do:

	if (close_lock_file(lock)) {
        	int fd = lock->fd;
                lock->fd = -1;
		die("Oops, failed to close fd %d", fd);
	}

to avoid atexit handler closing the lock->fd.

Worse yet, a careless caller may do:

	close_lock_file(lock);

	... do something that opens a new fd, perhaps for
        ... mmaping a packfile in

	rollback_lock_file(lock);

	... Oops, we cannot mmap the packfile.