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

Stefan Beller <sbeller@google.com> writes:

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?