Re: [PATCHv3 5/6] refs.c: remove unlock_ref and commit_ref from write_ref_sha1

[Junio C Hamano <gitster@pobox.com>]

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.  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?