From: Stefan Beller Date: Wed, 22 Apr 2015 19:09:43 GMT Subject: Re: [PATCHv2] refs.c: enable large transactions Message-ID: In-Reply-To: <5537AC00.5060208@alum.mit.edu> On Wed, Apr 22, 2015 at 7:11 AM, Michael Haggerty 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.