Michael Haggerty <mhagger@xxxxxxxxxxxx> writes: > On 05/02/2012 12:19 AM, Junio C Hamano wrote: > >> The specific failure of "refs/stash" is fixed with this patch, but I think >> this call to check_ref_format() in the filter_refs() should be removed. >> The function is trying to see what we _asked_ against what the remote side >> said they have, and if we tried to recover objects from a broken remote by >> doing: >> >> git fetch $somewhere refs/heads/a..b:refs/heads/a_dot_dot_b >> >> and the remote side advertised that it has such a ref (note that a..b is >> an illegal path component), we should be able to do so without a misguided >> call to check_refname_format() getting in the way. > > I agree; if the remote reference name never gets used as a local > reference name, there is no reason to call check_ref_format() on it at > all. The local reference name that is derived from the remote > reference name (even if it is derived via an identity operation) > *should* be checked using check_ref_format(); presumably that is > already the case? We should make sure the ref the refspec mapping told us to update is a valid ref locally (didn't I say that), but I do not know offhand if we do so using check_ref_format() and/or if the check is not overly loose. Honestly, I didn't expect _you_ to be _asking_ that question; as you are the person who has been advocating tightening of the function, you would know the callers of the function better than anybody, no? >> It is the same story if the name advertised by a broken remote were >> "refs/head/../heads/master". As long as the RHS of the refspec >> (i.e. refs/heads/a_dot_dot_b) is kosher, we must allow such a request to >> succeed, so that people can interoperate in less than perfect world. > > A slightly more awkward question is if the broken remote advertises > many references including "refs/head/../heads/master" and we fetch > refspec "+refs/*:refs/*". In this case it is pretty clear that we > should fetch all of the valid references; otherwise, working around > the problem would be quite awkward. If we ignore what will be stored in FETCH_HEAD, we could either (1) fetch histories leading to all the valid commits, but not store incorrectly formatted refs, or (2) fetch histories leading to commits that will be stored in only the valid _refs_. But because we cannot ignore FETCH_HEAD, we have to do (1). That is, history leading to any ref the remote advertises that matches the LHS of the refspec has to be fetched, and listed in the resulting FETCH_HEAD. Some of them may map to invalid refs on the local side, and we do not store it inside our refs/, so that they do not pollute our local ref namespace. > But what to do about > "refs/head/../heads/master"? I think we should emit a warning naming > the reference that will not be fetched "because it is formatted > incorrectly", and not include it in the "want" lines. We have to ask for it, so we include it "want". Remember, $A without colon is a short-hand for $A: (fetch but not store locally), and as long as $A (the LHS of the refspec) matches the refs advertised by the remote side, we fetch it and list it in FETCH_HEAD. As the user is _not_ asking to store it locally in our refs/ namespace, we won't store it. But if the request were $A:$A for any invalid refname $A, the story is the same. We fetch it because LHS of refspec matches what they advertise, and we list it in FETCH_HEAD. We do _not_ store it in our local refs/ space, because RHS of refspec is invalid. -- 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