[RFC PATCH] Make 'git request-pull' more strict about matching local/remote branches

2014-01-22 Thread Linus Torvalds 

From: Linus Torvalds torva...@linux-foundation.org
Date: Wed, 22 Jan 2014 12:32:30 -0800
Subject: [PATCH] Make 'git request-pull' more strict about matching
 local/remote branches

The current 'request-pull' will try to find matching commit on the given 
remote, and rewrite the please pull line to match that remote ref.

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).

To avoid that recurring problem, this changes git request-pull so that 
it matches the ref name to be pulled against the *local* repository, and 
then warns if the remote repository does not have that exact same branch 
or tag name and content.

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).

Signed-off-by: Linus Torvalds torva...@linux-foundation.org
---

This should fix the problem we've had multiple times with kernel 
maintainers, where git request-pull ends up leaving the target branch 
name blank, because people either forgot to push it, or (more commonly) 
people pushed it just before doing the pull request, and it hadn't 
actually had time to mirror out to the public site.

Now, git request-pull will *warn* about the fact that the matching ref 
isn't found on the remote (and the new matching code is stricter at that), 
but it will never try to re-write the branch name that it asks the other 
end to pull.

So if the remote branch doesn't exist, you'll get a warning, but the pull 
request will still have the branch you specified.

The whole checking thing is both simplified (removing more lines than it 
adds) and made more strict.

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).

Thus the RFC part. Because the currect git request-pull behavior has been 
horrible.

 git-request-pull.sh | 110 
 1 file changed, 43 insertions(+), 67 deletions(-)

diff --git a/git-request-pull.sh b/git-request-pull.sh
index fe21d5db631c..659a412155d8 100755
--- a/git-request-pull.sh
+++ b/git-request-pull.sh
@@ -35,20 +35,7 @@ do
shift
 done
 
-base=$1 url=$2 head=${3-HEAD} status=0 branch_name=
-
-headref=$(git symbolic-ref -q $head)
-if git show-ref -q --verify $headref
-then
-   branch_name=${headref#refs/heads/}
-   if test z$branch_name = z$headref ||
-   ! git config branch.$branch_name.description /dev/null
-   then
-   branch_name=
-   fi
-fi
-
-tag_name=$(git describe --exact $head^0 2/dev/null)
+base=$1 url=$2 status=0
 
 test -n $base  test -n $url || usage
 
@@ -58,55 +45,68 @@ then
 die fatal: Not a valid revision: $base
 fi
 
+#
+# $3 must be a symbolic ref, a unique ref, or
+# a SHA object expression
+#
+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
+
+# Was it a branch with a description?
+branch_name=${head#refs/heads/}
+if test z$branch_name = z$headref ||
+   ! git config branch.$branch_name.description /dev/null
 then
-die fatal: Not a valid revision: $head
+   branch_name=
 fi
 
+prettyhead=${head#refs/}
+prettyhead=${prettyhead#heads/}
+
 merge_base=$(git merge-base $baserev $headrev) ||
 die fatal: No commits in common between $base and $head
 
-# $head is the token given from the command line, and $tag_name, if
-# exists, is the tag we are going to show the commit information for.
-# If that tag exists at the remote and it points at the commit, use it.
-# Otherwise, if a branch with the same name as $head exists at the remote
-# and

Re: [RFC PATCH] Make 'git request-pull' more strict about matching local/remote branches

2014-01-22 Thread Junio C Hamano 
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

2014-01-22 Thread Linus Torvalds 
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

2014-01-22 Thread Linus Torvalds 
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

2014-01-22 Thread Junio C Hamano 
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

2014-01-22 Thread Linus Torvalds 
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

2014-01-22 Thread Junio C Hamano 
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