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

[Michael Haggerty]

On 06/07/2016 07:43 PM, Junio C Hamano wrote:
> Michael Haggerty  writes:
> 
>> From: David Turner 
>>
>> 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 
>> Signed-off-by: Junio C Hamano 
>> Signed-off-by: Michael Haggerty 
>> ---
>> 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Michael Haggerty  writes:

> From: David Turner 
>
> 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 
> Signed-off-by: Junio C Hamano 
> Signed-off-by: Michael Haggerty 
> ---
> 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;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html