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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 23, 2015, 23:57 UTC
Message-ID: <xmqq61bxoxs8.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1422043442-30676-6-git-send-email-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 16 quoted lines

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