On Fri, Jan 23, 2015 at 4:38 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Stefan Beller <sbeller@xxxxxxxxxx> writes:
>
>> On Fri, Jan 23, 2015 at 4:14 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>>
>> yeah that's the goal. Though as we're in one transaction, as soon
>> as we have an early exit, the transaction will abort.
>
> An early exit I am talking about is this:
>
> static int write_ref_sha1(struct ref_lock *lock,
> const unsigned char *sha1, const char *logmsg)
> {
> static char term = '\n';
> struct object *o;
>
> if (!lock) {
> errno = EINVAL;
> return -1;
> }
> if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
> return 0;
>
> It returns 0 and then the transaction side has this call in a loop:
>
> if (!is_null_sha1(update->new_sha1)) {
> if (write_ref_sha1(update->lock, update->new_sha1,
> update->msg)) {
> strbuf_addf(err, "Cannot write to the ref lock '%s'.",
> update->refname);
> ret = TRANSACTION_GENERIC_ERROR;
> goto cleanup;
> }
> }
And just after this code path there is the new
+ /* Do not keep all lock files open at the same time. */
+ close_ref(update->lock);
So in case we go the zero return path of write_ref_sha1 the loop looks like
/* Acquire all locks while verifying old values */
for (all updates) {
write_ref_sha1(...)
close_ref(...)
}
In case we do go the non zero return path, it is
if (write_ref_sha1(update->lock, update->new_sha1, ..)
goto cleanup;
...
cleanup:
for (i = 0; i < n; i++)
if (updates[i]->lock)
unlock_ref(updates[i]->lock);
I do not see the problem in the code itself, but rather in understanding
the code. I will send a follow up patch which makes it easier to follow
by removing the early exit with no problem away.
>
>>> If so, shouldn't the write function at least close the file
>>> descriptor even when it knows that the $ref.lock already has the
>>> correct object name? The call to close_ref() is never made when the
>>> early-return codepath is taken as far as I can see.
>>
>> The goto cleanup; will take care of unlocking (and closing fds of) all refs
>
> Yes, if write_ref_sha1() returns with non-zero signaling an error,
> then the goto will trigger.
>
> But if it short-cuts and returns zero, that goto will not be
> reached.
Yes, if the goto is not reached we just drop down to
close_ref(update->lock); which should take care of
the open fd.
Thanks,
Stefan
--
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