From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Thu, 09 Jun 2016 15:14:41 GMT
Subject: Re: [PATCH 34/38] refs: add method for delete_refs
Message-ID: <575987E1.9060200@alum.mit.edu>
In-Reply-To: <xmqqporsrj21.fsf@gitster.mtv.corp.google.com>

On 06/07/2016 07:43 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> From: David Turner <dturner@twopensource.com>
>>
>> In the file-based backend, delete_refs has some special optimization
>> to deal with packed refs.  In other backends, we might be able to make
>> ref deletion faster by putting all deletions into a single
>> transaction.  So we need a special backend function for this.
>>
>> Signed-off-by: David Turner <dturner@twopensource.com>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
>> I think that we could get away without this method if we make
>> ref_transactions a bit smarter (for example, by supporting best-effort
>> updates that can fail without causing the entire transaction to be
>> aborted). But that would be a significant detour, so let's leave it
>> here for now.
> 
> Hmm, I actually was wondering why 'pack without' was there while
> reading 24/38; IIUC, that is one of the "special optimization" that
> is very much tied to the files backend, and it may make sense to
> hide it behind delete_refs() as its implementation detail.
> 
> Which is exactly what this step is about, so I am happy ;-)

In the future I think this optimization will be even better hidden,
namely within the transaction_commit method of a packed_refs_store class :-)

> Unlike other changes like the ones that did read_raw_ref(),
> verify_refname_available(), etc., the title does not follow the
> pattern "refs: make X() virtual", even though as far as I can see
> the intent is the same as others.  Perhaps a minor retitle is in
> order?

OK.

Michael