Re: [PATCH v6 1/7] refs.c: add err arguments to reflog functions

[Michael Haggerty <mhagger@alum.mit.edu>]

On 07/08/2015 12:41 AM, David Turner wrote:
> On Mon, 2015-07-06 at 17:53 +0200, Michael Haggerty wrote:
>> On 06/29/2015 10:17 PM, David Turner wrote:
>>> [...]
>>> @@ -3317,7 +3322,8 @@ static int commit_ref_update(struct ref_lock *lock,
>>>  					      head_sha1, &head_flag);
>>>  		if (head_ref && (head_flag & REF_ISSYMREF) &&
>>>  		    !strcmp(head_ref, lock->ref_name))
>>> -			log_ref_write("HEAD", lock->old_oid.hash, sha1, logmsg);
>>> +			log_ref_write("HEAD", lock->old_oid.hash, sha1, logmsg,
>>> +				      err);
>>
>> Here you don't check for errors from log_ref_write(). So it might or
>> might not have written something to err. If it has, is that error
>> handled correctly?
> 
> That was the case before this patch too. But I guess I might as well
> check.

This isn't about fixing a pre-existing bug. Your patch introduced a
regression.

Before this patch, commit_ref_update() didn't really need to know
whether log_ref_write() failed, because it didn't take any further
remedial action anyway. log_ref_write() would have already written an
error message to stderr, and that was all the error handling that was
needed.

But now, log_ref_write() *doesn't* write its error message to stderr; it
only stores it to `err`. So now it is the caller's responsibility to
check whether there was an error, and if so write `err` to stderr.
Otherwise the error message will get lost entirely, I think.

I think your v7 of this patch goes too far, by turning a failure to
write to the reflog into a failure of the whole transaction. The problem
is that this failure comes too late, in the commit phase of the
transaction. Aborting at this late stage can leave some references
changed and others rolled back, violating the promise of atomicity.

The old code reported a failure to write the reflog to stderr but didn't
fail the transaction. I think that behavior is more appropriate. The
reflog is of lower importance than the references themselves. Junio, do
you agree?

If so, it might be that adding err arguments to the reflog functions
does not bring any benefits.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu