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

From: Ronnie Sahlberg <sahlberg@google.com>
Date: May 22, 2014, 15:32 UTC
Message-ID: <CAL=YDWmwfdb7BEoyFV4SQjC+wySDWzE1E4S3hrhsVy3UZOa5Lw@mail.gmail.com>
In-Reply-To: <20140521232211.GK12314@google.com>

On Wed, May 21, 2014 at 4:22 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 28 quoted lines

> Ronnie Sahlberg wrote:
>
>> --- a/refs.c
>> +++ b/refs.c
> [...]
>> @@ -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?

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.

Show 5 quoted lines

>
> [...]
>> -     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