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

From: Michael Haggerty <mhagger@alum.mit.edu>
To: Stefan Beller <sbeller@google.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	"git@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: [PATCHv2] refs.c: enable large transactions
Date: Wed, 22 Apr 2015 22:12:17 +0200	[thread overview]
Message-ID: <553800A1.3010903@alum.mit.edu> (raw)
In-Reply-To: <CAGZ79kbb=UUVq8np-ndiMS_id5GaBHpDafDe9ZB3TvsK3xYe_g@mail.gmail.com>

On 04/22/2015 09:09 PM, Stefan Beller wrote:
> 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?

At the point the lockfile is created, we have all the information we
need to write the new SHA-1 to it and close it immediately. It seems
more straightforward to do it that way than the way it is done in the
current code, where the locking and writing are separated in time and
space and now there is the small extra complication of
maybe-closing-maybe-not. But getting to the final destination requires
more refactoring than would be prudent for the upcoming release.

In other words, I think your fix is OK but that the whole area of code
has still not reached its final form. I am working on a patch series
that does what I have in mind, but it's not ready yet. As I remember I
got stuck when I realized that the reflog for HEAD is updated somewhere
out of the blue without proper locking and I haven't gotten around to
sorting it out yet.

> [...]
>> 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.

OK.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu