Subject: Re: [PATCH v20 39/48] refs.c: make delete_ref use a transaction

From: Michael Haggerty <[email-masked]>
Date: Tue, 08 Jul 2014 15:52:20 +0200

On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
> Change delete_ref to use a ref transaction for the deletion. At the same time
> since we no longer have any callers of repack_without_ref we can now delete
> this function.
> 
> Change delete_ref to return 0 on success and 1 on failure instead of the
> previous 0 on success either 1 or -1 on failure.
> 
> Reviewed-by: Jonathan Nieder <[email-masked]>
> Signed-off-by: Ronnie Sahlberg <[email-masked]>
> ---
>  refs.c | 34 +++++++++++++---------------------
>  1 file changed, 13 insertions(+), 21 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 3d070d5..92a06d4 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2544,11 +2544,6 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)
>  	return ret;
>  }
>  
> -static int repack_without_ref(const char *refname)
> -{
> -	return repack_without_refs(&refname, 1, NULL);
> -}
> -
>  static int delete_ref_loose(struct ref_lock *lock, int flag)
>  {
>  	if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
> @@ -2566,24 +2561,21 @@ static int delete_ref_loose(struct ref_lock *lock, int flag)
>  
>  int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
>  {
> -	struct ref_lock *lock;
> -	int ret = 0, flag = 0;
> +	struct ref_transaction *transaction;
> +	struct strbuf err = STRBUF_INIT;
>  
> -	lock = lock_ref_sha1_basic(refname, sha1, delopt, &flag);

The old code checked that the old value of refname was sha1, regardless
of whether sha1 was null_sha1.  Presumably callers never set sha1 to
null_sha1...

> -	if (!lock)
> +	transaction = ref_transaction_begin(&err);
> +	if (!transaction ||
> +	    ref_transaction_delete(transaction, refname, sha1, delopt,
> +				   sha1 && !is_null_sha1(sha1), &err) ||

...But the new code explicitly skips the check if sha1 is null_sha1.
This shouldn't make a practical difference, because presumably callers
never set sha1 to null_sha1.  But given that the new policy elsewhere
for "delete" updates is that it is an error for old_sha1 to equal
null_sha1, it seems to me that this extra check shouldn't be here.  So I
think this should be changed to

    ref_transaction_delete(transaction, refname, sha1, delopt,
                           !!sha1, &err) ||

> [...]

Michael

-- 
Michael Haggerty
[email-masked]