Re: [PATCH 34/38] refs: add method for delete_refs

On 06/07/2016 07:43 PM, Junio C Hamano wrote:

Michael Haggerty [off-list ref] writes:

quoted

From: David Turner <redacted>

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 <redacted>
Signed-off-by: Junio C Hamano <redacted>
Signed-off-by: Michael Haggerty <redacted>
---
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