On Tue, Apr 8, 2014 at 2:50 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Ronnie Sahlberg <sahlberg@xxxxxxxxxx> 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...
>
Good point. Thanks.
Please consider this patch abandoned.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html