Re: [PATCH 04/10] packed_delete_refs(): implement method

[Brandon Williams <bmwill@google.com>]

On 08/29, Michael Haggerty wrote:
> Implement `packed_delete_refs()` using a reference transaction. This
> means that `files_delete_refs()` can use `refs_delete_refs()` instead
> of `repack_without_refs()` to delete any packed references, decreasing
> the coupling between the classes.
> 
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs/files-backend.c  |  2 +-
>  refs/packed-backend.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 45 insertions(+), 2 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index fccbc24ac4..2c78f63494 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1157,7 +1157,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,
>  	if (packed_refs_lock(refs->packed_ref_store, 0, &err))
>  		goto error;
>  
> -	if (repack_without_refs(refs->packed_ref_store, refnames, &err)) {
> +	if (refs_delete_refs(refs->packed_ref_store, msg, refnames, flags)) {
>  		packed_refs_unlock(refs->packed_ref_store);
>  		goto error;
>  	}
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index d19b3bfba5..83a088118f 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -1082,7 +1082,50 @@ static int packed_initial_transaction_commit(struct ref_store *ref_store,
>  static int packed_delete_refs(struct ref_store *ref_store, const char *msg,
>  			     struct string_list *refnames, unsigned int flags)
>  {
> -	die("BUG: not implemented yet");
> +	struct packed_ref_store *refs =
> +		packed_downcast(ref_store, REF_STORE_WRITE, "delete_refs");
> +	struct strbuf err = STRBUF_INIT;
> +	struct ref_transaction *transaction;
> +	struct string_list_item *item;
> +	int ret;
> +
> +	(void)refs; /* We need the check above, but don't use the variable */

Can't say I've seen a line like this before, Is the intent to just mark
that this variable won't be used like you mention in the comment?

> +
> +	if (!refnames->nr)
> +		return 0;
> +
> +	/*
> +	 * Since we don't check the references' old_oids, the
> +	 * individual updates can't fail, so we can pack all of the
> +	 * updates into a single transaction.
> +	 */
> +
> +	transaction = ref_store_transaction_begin(ref_store, &err);
> +	if (!transaction)
> +		return -1;
> +
> +	for_each_string_list_item(item, refnames) {
> +		if (ref_transaction_delete(transaction, item->string, NULL,
> +					   flags, msg, &err)) {
> +			warning(_("could not delete reference %s: %s"),
> +				item->string, err.buf);
> +			strbuf_reset(&err);
> +		}
> +	}
> +
> +	ret = ref_transaction_commit(transaction, &err);
> +
> +	if (ret) {
> +		if (refnames->nr == 1)
> +			error(_("could not delete reference %s: %s"),
> +			      refnames->items[0].string, err.buf);
> +		else
> +			error(_("could not delete references: %s"), err.buf);
> +	}
> +
> +	ref_transaction_free(transaction);
> +	strbuf_release(&err);
> +	return ret;
>  }
>  
>  static int packed_pack_refs(struct ref_store *ref_store, unsigned int flags)
> -- 
> 2.14.1
> 

-- 
Brandon Williams