On Thu, May 22, 2014 at 8:32 AM, Ronnie Sahlberg <sahlb...@google.com> wrote:
> On Wed, May 21, 2014 at 4:22 PM, Jonathan Nieder <jrnie...@gmail.com> wrote:
>> 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.
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.
>
>>
>> [...]
>>> - 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
