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

From: Stefan Beller <sbeller@google.com>
To: Michael Haggerty <mhagger@alum.mit.edu>
Cc: Jeff King <peff@peff.net>,
	"git@vger.kernel.org" <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>,
	Loic Dachary <loic@dachary.org>
Subject: Re: [PATCHv2 5/5] refs.c: enable large transactions
Date: Thu, 22 Jan 2015 11:24:57 -0800	[thread overview]
Message-ID: <CAGZ79kbrnu7ZH7T6WR5j0_nhS+Ann0LcEmXfQYMRqdSzK0xW9Q@mail.gmail.com> (raw)
In-Reply-To: <54C12644.1000406@alum.mit.edu>

On Thu, Jan 22, 2015 at 8:33 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 01/22/2015 02:10 PM, Jeff King wrote:
>> On Thu, Jan 22, 2015 at 12:24:23PM +0100, Michael Haggerty wrote:
>>
>>> I can't figure out where to apply this series or where to fetch it from,
>>> so I can't see these changes in context, so maybe I'm misunderstanding
>>> something. It looks like this code is doing
>>>
>>>     open(), close(), open(), fdopen(), write(), fclose(), rename()
>>>
>>> on each lockfile. But don't we have enough information to write the
>>> SHA-1 into the lockfile the first time we touch it? I.e., couldn't we
>>> reduce this to
>>>
>>>     open(), fdopen(), write(), fclose(), rename()
>>>
>>> , where the first four calls all happen in the initial loop? If a
>>> problem is discovered when writing a later reference, we would roll back
>>> the transaction anyway.
>>>
>>> I understand that this would require a bigger rewrite, so maybe it is
>>> not worth it.
>>
>> I had a nagging feeling on the multiple-open thing, too, and would much
>> prefer to just write out the contents early (since we know what they
>> are). It looks like we would just need to split write_ref_sha1() into
>> its two halves:
>>
>>   1. Write out the lockfile
>>
>>   2. Commit the change
>>
>> And then call them at the appropriate spots from ref_transaction_commit().
>>
>> I guess that is maybe a step backwards for abstracted ref backends,
>> though.
>
> Nah, the implementation of ref_transaction_commit() will have to differ
> between backends anyway. I don't think this would be a step backwards.
>
> Michael
>

I also dislike the double open/close thing, but I just wanted to come up with
a quick and unobtrusive fix which doesn't rewrite the whole refs backend as
we have some code churn in the refs lately.

Michael, I forgot your short term intentions on the refs backend, so I tried to
be shy with that bug fix. What huge changes are you planning in the next few
weeks w.r.t. the refs handling? I would look more into that if there are no code
conflicts likely to arise.

Thanks,
Stefan