From: Stefan Beller <sbeller@google.com>
Date: Sat, 24 Jan 2015 01:04:14 GMT
Subject: Re: [PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1
Message-ID: <CAGZ79kZ0EBRgSuc4Wupaa9t85xcF79SizvGbjD9OC3+Jq=XbFA@mail.gmail.com>
In-Reply-To: <xmqqppa5nhab.fsf@gitster.dls.corp.google.com>

On Fri, Jan 23, 2015 at 4:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> 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?
>
> These two lines in pre-context of the hunk:
>
>>>>       lock->force_write = 1;
>>>>       hashcpy(lock->old_sha1, orig_sha1);
>

So these 2 lines (specially the force_write=1 line) is just there to trigger
the valid early exit path as you sent in the other mail :

> if (!lock->force_write && !hashcmp(lock->old_sha1, sha1))
>             return 0;

when we have the same sha1?
and you're saying that's a problem because hard to understand?

I am confused as well by now.