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@xxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html