Stefan Beller <sbel...@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. 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.
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?
--
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
