On Wed, Apr 22, 2015 at 7:11 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> 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. -- 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