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

[Junio C Hamano <gitster@pobox.com>]

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 ;-)

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?

>  refs.c               | 7 +++++++
>  refs/files-backend.c | 6 ++++--
>  refs/refs-internal.h | 3 +++
>  3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 6c1e899..8ab9862 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1529,3 +1529,10 @@ int initial_ref_transaction_commit(struct ref_transaction *transaction,
>  
>  	return refs->be->initial_transaction_commit(refs, transaction, err);
>  }
> +
> +int delete_refs(struct string_list *refnames, unsigned int flags)
> +{
> +	struct ref_store *refs = get_ref_store(NULL);
> +
> +	return refs->be->delete_refs(refs, refnames, flags);
> +}
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 253899f..5681141 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2455,10 +2455,11 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
>  	return 0;
>  }
>  
> -int delete_refs(struct string_list *refnames, unsigned int flags)
> +static int files_delete_refs(struct ref_store *ref_store,
> +			     struct string_list *refnames, unsigned int flags)
>  {
>  	struct files_ref_store *refs =
> -		get_files_ref_store(NULL, "delete_refs");
> +		files_downcast(ref_store, 0, "delete_refs");
>  	struct strbuf err = STRBUF_INIT;
>  	int i, result = 0;
>  
> @@ -4070,6 +4071,7 @@ struct ref_storage_be refs_be_files = {
>  	files_pack_refs,
>  	files_peel_ref,
>  	files_create_symref,
> +	files_delete_refs,
>  
>  	files_ref_iterator_begin,
>  	files_read_raw_ref,
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index e462b54..f944b7a 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -485,6 +485,8 @@ typedef int create_symref_fn(struct ref_store *ref_store,
>  			     const char *ref_target,
>  			     const char *refs_heads_master,
>  			     const char *logmsg);
> +typedef int delete_refs_fn(struct ref_store *ref_store,
> +			   struct string_list *refnames, unsigned int flags);
>  
>  /*
>   * Iterate over the references in the specified ref_store that are
> @@ -582,6 +584,7 @@ struct ref_storage_be {
>  	pack_refs_fn *pack_refs;
>  	peel_ref_fn *peel_ref;
>  	create_symref_fn *create_symref;
> +	delete_refs_fn *delete_refs;
>  
>  	ref_iterator_begin_fn *iterator_begin;
>  	read_raw_ref_fn *read_raw_ref;