Re: [PATCHv3 6/6] refs.c: enable large transactions

From: Stefan Beller <sbeller@google.com>
Date: Jan 26, 2015, 19:30 UTC
Message-ID: <CAGZ79kbQiVQJyZC8mKaSUnOpY6YJc0TYdX=msuZDXLd7DxmTmQ@mail.gmail.com>
In-Reply-To: <xmqqtwzhnhbl.fsf@gitster.dls.corp.google.com>

On Fri, Jan 23, 2015 at 4:38 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 33 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> On Fri, Jan 23, 2015 at 4:14 PM, Junio C Hamano <gitster@pobox.com> 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.

Show 13 quoted lines

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