Re: [RFC PATCH] Make 'git request-pull' more strict about matching local/remote branches
Linus Torvalds torva...@linux-foundation.org writes: This means that git request-pull will never rewrite the ref-name you gave it. If the local branch name is xyzzy, that is the only branch name that request-pull will ask the other side to fetch. If the remote has that branch under a different name, that's your problem and git request-pull will not try to fix it up (but git request-pull will warn about the fact that no exact matching branch is found, and you can edit the end result to then have the remote name you want if it doesn't match your local one). The new find local ref code will also complain loudly if you give an ambiguous refname (eg you have both a tag and a branch with that same name, and you don't specify heads/name or tags/name). I agree with the basic direction, especially the part we will never rewrite, is quite attractive. But this part might be a bit problematic. $3=master will almost always have refs/heads/master and refs/remotes/origin/master listed because the call to show-ref comes before rev-parse --verify, no? +head=$(git symbolic-ref -q ${3-HEAD}) +head=${head:-$(git show-ref ${3-HEAD} | cut -d' ' -f2)} +head=${head:-$(git rev-parse --quiet --verify $3)} + +# None of the above? Bad. +test -z $head die fatal: Not a valid revision: $3 + +# This also verifies that the resulting head is unique: +# git show-ref could have shown multiple matching refs.. headrev=$(git rev-parse --verify --quiet $head^0) -if test -z $headrev +test -z $headrev die fatal: Ambiguous revision: $3 ... and it would die here. $3=for-linus may be the most common case on the kernel list, and remotes/origin/for-linus may be unlikely to appear in the real life (hmph, really? perhaps people keep track of what they pushed out the last time with it, I dunno). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] Make 'git request-pull' more strict about matching local/remote branches
On Wed, Jan 22, 2014 at 1:46 PM, Junio C Hamano gits...@pobox.com wrote: The new find local ref code will also complain loudly if you give an ambiguous refname (eg you have both a tag and a branch with that same name, and you don't specify heads/name or tags/name). But this part might be a bit problematic. $3=master will almost always have refs/heads/master and refs/remotes/origin/master listed because the call to show-ref comes before rev-parse --verify, no? Hmm. Yes. It's done that way very much on purpose, to avoid the branch/tag ambiguity (which we have had problems with), but you're right, it also ends up being ambiguous wrt remote branches, which wasn't the intention, and you're right, that is not acceptable. Damn. I very much want to get the full ref-name (ie master should become refs/heads/master), and I do want to avoid the branch/tag ambiguity, but you're right, show-ref plus the subsequent rev-parse --verify comes close but not quite close enough. Any ideas? The hacky way is to do | head -1 to take the first show-ref output, and then check if you get a different result if you re-do it using show-ref --tags. But that sounds really excessively hacky. Is there a good way to do it? Linus -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] Make 'git request-pull' more strict about matching local/remote branches
On Wed, Jan 22, 2014 at 2:03 PM, Linus Torvalds torva...@linux-foundation.org wrote: Any ideas? The hacky way is to do | head -1 to take the first show-ref output, and then check if you get a different result if you re-do it using show-ref --tags. But that sounds really excessively hacky. Is there a good way to do it? Using git show-refs --tags --heads would work for the common case (since that ignores remote branches), but would then disallow remote branches entirely. That might be ok in practice, but it's definitely wrong too. I'm probably missing some obvious solution. Linus -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] Make 'git request-pull' more strict about matching local/remote branches
Linus Torvalds torva...@linux-foundation.org writes: That may be very helpful if your local tree doesn't match the layout of the remote branches, but for the common case it's been a recurring disaster, when request-pull is done against a delayed remote update, and it rewrites the target branch randomly to some other branch name that happens to have the same expected SHA1 (or more commonly, leaves it blank). Thinking about this a bit more... Comments? It passes the tests I put it through locally, but I did *not* make it pass the test-suite, since it very much does change the rules. Some of the test suite code literally tests for the old completely broken case (at least t5150, subtests 4 and 5). I looked at 5150.4 and found that what it attempts to do is halfway sensible. The contributor works on the local 'master' branch, publishes the result to 'for-linus' in its 'origin' repository, and asks his state to be pulled, with: git push origin master:for-linus git request-pull initial origin The contributor could be more explicit in his request-pull and say git request-pull initial origin master but there is no 'master' on the publishing repository in this case (or even if there is, it does not match what is being pushed out), and there is no 'for-linus' branch locally, so there is no way for him to say git request-pull initial origin for-linus unless he creates it locally first. I am starting to wonder if it is a better fix to check potentially ambiguous cases (e.g. the publishing repository does have 'master' that does not point at the commit local 'master' points at, and 'for-linus' that points at the same commit, and the user asks for 'master' to be pulled) or clearly broken cases (e.g. the user gave something other than HEAD explicitly from the command line, but we ended up computing blank) and die loudly, without breaking cases this test tries to protect. On the other hand, I tend to think what 5150.5 wants is convoluted and expects a broken behaviour. Its publishing repository has 'master' and 'for-upstream', and also has 'tags/full' that is an annotated tag that points at the commit, runs request-pull with its local 'master', and expects the resulting message to ask 'tags/full' to be pulled. If the contributor wants such a non-commit to be pulled, I think it should be made more explicit, i.e., not with git request-pull initial $origin_url but with git request-pull initial $origin_url tags/full or something. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] Make 'git request-pull' more strict about matching local/remote branches
On Wed, Jan 22, 2014 at 2:14 PM, Junio C Hamano gits...@pobox.com wrote: I looked at 5150.4 and found that what it attempts to do is halfway sensible. I agree that it is half-way sensible. The important bit being the HALF part. The half part is why we have the semantics we have. There's no question about that. The problem is, the *other* half is pure and utter crap. The half-way sensible solution then generates pure and utter garbage in the totally sensible case. And that's why I think it needs to be fixed. Not because the existing behavior can never make sense in some circumstances, but because the existing behavior can screw up really really badly in other (arguably more common, and definitely real) circumstances. For the kernel, the broken missing branch name situation has come up pretty regularly. This is definitely not a one-time event, it's more like almost every merge window somebody gets screwed by this and I have to guess what the branch name should have been. I think that we could potentially do a local:remote syntax for that half-way sensible case, so that if you do git push .. master:for-linus then you have to do git request-pull .. master:for-linus to match the fact that you renamed your local branch on the remote. Linus -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] Make 'git request-pull' more strict about matching local/remote branches
Junio C Hamano gits...@pobox.com writes: ... there is no 'for-linus' branch locally, so there is no way for him to say git request-pull initial origin for-linus unless he creates it locally first. In real life on the kernel list, for-linus may have to be a signed tag, and pushed out 1-to-1 name mapping, so in that sense, unless he creates it locally first may not be a problem. I'd throw this into No, this is not the only way to do so and there are workarounds available if we suddenly tightened the rule and broke those who relied on this behaviour. But this is not a less good way compared to the alternative of creating the same-named ref first, so we _are_ breaking people deliberately---is that worth the safety for always-push-one-to-one people? category. I'd throw the other one (i.e. 5150.5) into that is crazy enough that a short apology in the Release Notes is sufficient before breaking those who relied on that behaviour category, on the other hand ;-). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html