On Sun, 16 Mar 2008, Junio C Hamano wrote: > I was looking at t5505 tests and noticed something funny. > > This is a design level question, so I am cc'ing Daniel whose remote.c is > heavily involved in the new implementation. > > What should this config do: > > [remote "origin"] > url = ../one/.git > fetch = +refs/heads/*:refs/remotes/origin/* > fetch = refs/heads/master:refs/heads/upstream > > when the other repository (../one/.git) has branches "master" and "side2"? > > I am not sure if the original implementation used to copy master to both > refs/remotes/origin/master and refs/heads/upstream, but I think that is > what the users would expect. > > I think the current one excludes any source that has an explicit > destination from the wildcard matches. It is probably Ok as long as we > reject if the same source has more than one destinations (or matches more > than one wildcards, for that matter) like this as a configuration error: > > [remote "origin"] > url = ../one/.git > fetch = refs/heads/master:refs/heads/upstream > fetch = refs/heads/master:refs/heads/another > > If it doesn't, it does feel somewhat inconsistent. > > Fortunately or unfortunately, Documentation/pull-fetch-param.txt does not > talk about wildcard refspecs (not even the syntax, let alone the > semantics), so we can define whatever we want right now, and I think both > > (1) allow duplicated destinations, including wildcard matches; and > > (2) refuse duplicated destinations for explicit ones, and more than > one wildcard patterns that match the same ref, but omit explicitly > specified ones from wildcard matches; > > are viable options. I suspect the current code does not do either. We > should pick one semantics, make sure the implementation matches that, and > document it. Actually, I think the current code is close to (2). get_fetch_map() returns everything, ref_remove_duplicates() removes any exact matches and gives errors if there's the same destination for two different sources. (Upon further consideration, there's one slight issue: [remote "origin"] fetch = refs/heads/*:refs/remotes/origin/* fetch = +refs/heads/pu:refs/remotes/origin/pu is not quite the same as: [remote "origin"] fetch = +refs/heads/pu:refs/remotes/origin/pu fetch = refs/heads/*:refs/remotes/origin/* in whether pu will be forced; the forcing flag on the first matching refspec is what matters.) In any case, the implementation should be easy enough with any of these combinations. > Another topic is what the semantics should be for mirroring configuration, > like this: > > [remote "origin"] > url = ../one/.git > fetch = refs/*:refs/* > > or > > [remote "origin"] > url = ../one/.git > fetch = refs/*:refs/remotes/one/* > > The issues are: > > (1) get_fetch_map() currently insists on refname to be check_ref_format() > clean; it even rejects CHECK_REF_FORMAT_ONELEVEL, which means that > refs/stash would not be considered Ok and the code will die(). Yes, that's probably wrong. We probably do want to reject people whose servers send us "refs/heads/../../heads/master", but not "refs/stash". > (2) "git remote prune" seems to cull refs/remotes/one/HEAD if exists. > > Currently we do not have a way to determine where HEAD at the remote > points at at the protocol level (I've sent a patch to the list earlier for > the necessary protocol extension on the upload-pack side, but receiver > side never got implemented in remotes.c). So we cannot propagate > refs/HEAD information correctly right now, but when we accept the protocol > extension to do so, issue (1) will matter also for HEAD. There's the issue that "HEAD" isn't "refs/HEAD". I'm not at all sure how the user should communicate the desire to update things to match the remote HEAD. FWIW, I was considering moving the code to guess where the remote HEAD points from builtin-clone to remotes.c, until I realized that it's not clear what configuration should control this. I think it'd be necessary to have a special option to say "write HEAD here", but I may be wrong. The implementation of whatever handles it is slightly non-trivial, since struct ref can't currently represent a symref, but that shouldn't be a major issue. -Daniel *This .sig left intentionally blank* -- 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
