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

On 02/05/2016 08:44 PM, David Turner wrote:

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