Re: [PATCH v6 1/7] refs.c: add err arguments to reflog functions
- From
Michael Haggerty <mhagger@alum.mit.edu>
- Date
- Jul 8, 2015, 10:59 UTC
- Message-ID
- <559D0281.6040908@alum.mit.edu>
- In-Reply-To
- <1436308882.5521.15.camel@twopensource.com>
On 07/08/2015 12:41 AM, David Turner wrote:
Show 17 quoted lines
> 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