Re: [PATCH v4 13/21] refs: resolve symbolic refs first

[Michael Haggerty <mhagger@alum.mit.edu>]

On 02/05/2016 08:44 PM, David Turner wrote:
> Before committing ref updates, split symbolic ref updates into two
> parts: an update to the underlying ref, and a log-only update to the
> symbolic ref.  This ensures that both references are locked correctly
> while their reflogs are updated.
> 
> It is still possible to confuse git by concurrent updates, since the
> splitting of symbolic refs does not happen under lock. So a symbolic ref
> could be replaced by a plain ref in the middle of this operation, which
> would lead to reflog discontinuities and missed old-ref checks.

This patch is doing too much at once for my little brain to follow.

My first hangup is the change to setting RESOLVE_REF_NO_RECURSE
unconditionally in lock_ref_sha1_basic(). I count five callers of that
function and see no justification for why the change is OK in the
context of each caller. Here are some thoughts:

* The call from files_create_symref() sets REF_NODEREF, so it is
unaffected by this change.

* The call from files_transaction_commit() is preceded by a call to
dereference_symrefs(), which I assume effectively replaces the need for
RESOLVE_REF_NO_RECURSE.

* There are two calls from files_rename_ref(). Why is it OK to do
without RESOLVE_REF_NO_RECURSE there?

  * For the oldrefname call, I suppose the justification is the "(flag &
REF_ISSYMREF)" check earlier in the function. (But does this introduce a
significant TOCTOU race?)

  * For the newrefname call, I suppose it's because the code a little
higher up tries to delete any existing reference with that name. It
looks to me like the old code was slightly broken: if newrefname was an
unborn symbolic reference, then: read_ref_full() would fail;
delete_ref() would be skipped; lock_ref_sha1_basic() would lock the
*referred-to* reference; the referred-to reference would be overwritten
instead of newrefname. So it could be that here REF_NODEREF indirectly
fixes a bug?

* The last call, from files_reflog_expire(), is also questionable before
your patch. If refname is a symref, then the function is expiring the
reflog of the symref. But (before this patch) it locks not the symref
but its referent. This was discussed in some length before on the
mailing list [1], and the conclusion was that the current behavior is
wrong, but for backwards compatibility reasons it would be safest to
change it to locking *both* the symref and its referent.

If possible, it would be better to split this patch up into several: the
first few would each add the REF_NODEREF flag at one callsite, with a
careful justification of why that is OK. Once all the callsites (except
the one in files_transaction_commit()) have been changed, then the last
patch could add the dereference_symrefs() machinery and change the last
callsite.

(I'm not certain that those steps are actually doable independently,
given that REF_NODEREF has other effects besides setting
RESOLVE_REF_NO_RECURSE.)

I'm not just being pedantic here. The patch as written is really too big
to review effectively.

Michael

[1]
http://thread.gmane.org/gmane.comp.version-control.git/263552/focus=263555

-- 
Michael Haggerty
mhagger@alum.mit.edu