Re: [PATCH] Remove the close_ref function.

> 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...
>