Re: [PATCH v8 35/44] refs.c: make delete_ref use a transaction

From: Jonathan Nieder <jrnieder@gmail.com>
Date: May 21, 2014, 23:22 UTC
Message-ID: <20140521232211.GK12314@google.com>
In-Reply-To: <1400174999-26786-36-git-send-email-sahlberg@google.com>

Ronnie Sahlberg wrote:

> --- a/refs.c
> +++ b/refs.c

[...]

Show 8 quoted lines

> @@ -2515,24 +2510,18 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
>  
>  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;
>

[...]

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

What should happen when is_null_sha1(sha1)? In that case the caller has asked us to check that the ref doesn't exist before deleting it, which doesn't make a lot of sense, but the old code did exactly that if I read it correctly. The new code seems to disable verification instead.

Do we know that no callers call delete_ref with such an argument? Would it make sense to just die with a "BUG:" message to make the contract more clear?

[...]

> -	unlink_or_warn(git_path("logs/%s", lock->ref_name));

When does the reflog get deleted in the new code?

Thanks, Jonathan