Stefan Beller <sbeller@xxxxxxxxxx> writes:
> On Fri, Jan 23, 2015 at 4:39 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> Stefan Beller <sbeller@xxxxxxxxxx> writes:
>>
>>> On Fri, Jan 23, 2015 at 3:57 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>>>> Stefan Beller <sbeller@xxxxxxxxxx> 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?
What is the point of that hashcmp() in the first place? My
understanding is that
(1) lock->old_sha1 is to hold the original SHA-1 in the ref we took
the lock on.
(2) when not forcing, and when the two SHA-1 are the same, we
bypass and return early because overwriting the ref with the
same value is no-op.
Now, in that codepath, when the code is renaming into some ref, the
ref either would have no original SHA-1 (i.e. we are renaming to a
non-existing name) or have unrelated SHA-1 (i.e. we are overwriting
an existing one). For some (unknown to me) reason, however, the
code pretends that lock->old_sha1 has the new SHA-1 already before
we start to do the write or commit.
And because both write and commit tries to pretend to be no-op when
the caller tries to update a ref with the same SHA-1, but in this
codepath it does want the write to happen, it needs to set the
force_write bit set, which look like an unnecessary workaround.
Let me rephrase.
A natural way to write that caller, I think, would be more like
this:
... lock is given by the caller, probably with ->old_sha1
... filled in; it is not likely to be orig_sha1, as it is
... either null (if locking to create a new ref) or some
... unrelated value read from the ref being overwritten by
... this rename_ref() operation.
write_ref_sha1(lock, orig_sha1, logmsg);
# which may do the "don't write the same value" optmization
# if we are renaming another ref that happens to have the
# same value, in which case it is OK. Otherwise this will
# overwrite without being forced.
... *IF* and only if there is some reason lock->old_sha1
... needs to match what is in the filesystem ref right now,
... then do
hashcpy(lock->old_sha1, orig_sha1);
... but probably there is no reason to do so.
The two lines hashcpy() and ->force_write = 1 that appear before the
write_ref_sha1() do not conceptually make sense. The former does
not make sense because ->old_sha1 is supposed to be the original
value that is already stored in the ref, to allow us optimize "write
the same value" case, and you are breaking that invariant by doing
hashcpy() before write_ref_sha1(). The lock->old_sha1 value does
not have anything to do with the (original) value of the ref we took
the lock on.
And ->force_write = 1 becomes necessary only because the effect of
this nonsensical hashcpy() is to make the !hashcmp() used by the
short-cut logic trigger. Since the code needs to override that
logic, you need to say "force" before calling write_ref_sha1(). If
you didn't do the hashcpy(), you wouldn't have to say "force", no?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html