Re: [PATCH] Remove the close_ref function.

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

Ronnie Sahlberg <sahlberg@google.com> writes:

> @@ -2824,8 +2816,7 @@ int write_ref_sha1(struct ref_lock *lock,
>  		return -1;
>  	}
>  	if (write_in_full(lock->lock_fd, sha1_to_hex(sha1), 40) != 40 ||
> -	    write_in_full(lock->lock_fd, &term, 1) != 1
> -		|| close_ref(lock) < 0) {
> +	    write_in_full(lock->lock_fd, &term, 1) != 1) {

In the original code, we try to write the new object name and the
line terminator, and also try to make sure that the file descriptor
is successfully closed here.  Only when all of that is done
successfully we go update the reflog and then after that, we commit
the lockfile by renaming.

With the updated code, these two write(2)s may succeed, but we would
not know if the close(2) would succeed, until commit_lock_file() is
called much later in this codepath.

We would end up updating the reflog even when the close(2) of the
ref fails.

To be really paranoid, we should probably be doing an fsync(2)
to make sure that the bytes written hit the disk platter, not just
close(2), and such a change may be a good step in the direction to
make the code more robust; in that light, the patch goes in the
opposite direction "what it does is not robust enough anyway, so
let's loosen it further".

Hmm...