From: Stefan Beller <sbeller@google.com>
Date: Sat, 24 Jan 2015 00:22:57 GMT
Subject: Re: [PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1
Message-ID: <CAGZ79kYPCFBNNkYV0C_XeCEaqfz=Mh3Zy=L-vAWsmZBFgebqNQ@mail.gmail.com>
In-Reply-To: <xmqq61bxoxs8.fsf@gitster.dls.corp.google.com>

On Fri, Jan 23, 2015 at 3:57 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> -static int commit_ref(struct ref_lock *lock)
>> +static int commit_ref(struct ref_lock *lock, const unsigned char *sha1)
>>  {
>> +     if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
>> +             return 0;
>>       if (commit_lock_file(lock->lk))
>>               return -1;
>>       return 0;
>> @@ -2879,10 +2882,13 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
>>       }
>>       lock->force_write = 1;
>>       hashcpy(lock->old_sha1, orig_sha1);
>> -     if (write_ref_sha1(lock, orig_sha1, logmsg)) {
>> +     if (write_ref_sha1(lock, orig_sha1, logmsg)
>> +         || commit_ref(lock, orig_sha1)) {
>> +             unlock_ref(lock);
>
> This is not a new problem, but the two lines in pre-context of this
> patch look strange.

Which (not new) problem are you talking about here? Do you have
a reference?

> Regardless of what this particular caller does, I am not sure if the
> early-return codepath in commit_ref() is correct.  From the callers'
> point of view, it sometimes unlocks the ref (i.e. when a different
> SHA-1 is written or force_write is set) and sometimes keeps the ref
> locked (i.e. when early-return is taken).  Shouldn't these two cases
> behave identically?  Or am I wrong to assume that the early return
> using "hashcmp(lock->old_sha1, sha1)" is a mere optimization?
>

I assumed it was not just an optimization as the test suite fails if I
touch that line. I'll look into cleaning it up in a more obvious fashion.