Re: [PATCHv2] refs.c: enable large transactions

[Stefan Beller <sbeller@google.com>]

On Wed, Apr 22, 2015 at 7:11 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>> +     if (lock->lk->fd == -1)
>> +             reopen_lock_file(lock->lk);
>
> You should check that reopen_lock_file() was successful.

ok


>> @@ -3762,6 +3779,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>>                                   update->refname);
>>                       goto cleanup;
>>               }
>> +             if (remaining_fds > 0)
>> +                     remaining_fds--;
>> +             else
>> +                     close_lock_file(update->lock->lk);
>
> I consider this code a stopgap, and simplicity is more important than
> optimization.

Can you explain a bit why you think this is a stopgap?

Looking at the patch this looks simple to me, as there are no
huge pain points involved. (Compared to [1] as we'd change a lot in
that series)

Also this is pretty good on performance.
The small cases do not have an additional unneeded close and reopen,
but only the
larger cases do.

[1] http://git.661346.n2.nabble.com/PATCHv1-0-6-Fix-bug-in-large-transactions-tt7624363.html#a7624368




> But just for the sake of discussion, if we planned to keep
> this code around, it could be improved by not wasting open file
> descriptors for references that are only being verified or deleted, like so:

I'll pick that up for the resend.