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

On Thu, May 22, 2014 at 8:32 AM, Ronnie Sahlberg [off-list ref] wrote:

On Wed, May 21, 2014 at 4:22 PM, Jonathan Nieder [off-list ref] wrote:

quoted

Ronnie Sahlberg wrote:

quoted

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

[...]

quoted

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

[...]

quoted

-     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?

There are no callers that pass in null_sha1 explicitely and all tests pass.
I have changed it to a die("BUG... to make it more explicit as you suggested.

Strike that.

While there are no callers I can see that deliberately send null_sha1
it can happen because resolve_ref_unsafe(reading=0) calls
handle_missing_loose_ref which will clear sha1
if the ref is missing.

This can happen for either symrefs or refs that are soft links that
point to a nonexisting ref.

quoted

[...]

quoted

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

When does the reflog get deleted in the new code?

It is deleted towards the end of ref_transaction_commit, after the ref
itself has been deleted.

Thanks!

ronnie sahlberg