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

On Wed, Apr 22, 2015 at 7:11 AM, Michael Haggerty [off-list ref] wrote:

quoted

+     if (lock->lk->fd == -1)
+             reopen_lock_file(lock->lk);

You should check that reopen_lock_file() was successful.

ok

quoted

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