Re: [PATCH] Remove the close_ref function.
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 8, 2014, 21:50 UTC
- Message-ID
- <xmqqeh173301.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1396991830-20938-2-git-send-email-sahlberg@google.com>
Ronnie Sahlberg <sahlberg@google.com> writes:
Show 7 quoted lines
> @@ -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...