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

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

[Linus Torvalds]

From: Linus Torvalds <torvalds@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 <torvalds@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 their values match, use that instead.
+# $head is the refname from the command line.
+# If a ref with the same name as $head exists at the remote
+# and their values match, use that.
 #
 # Otherwise find a random ref that matches $headrev.
 find_matching_ref='
-	sub abbr {
-		my $ref = shift;
-		if ($ref =~ s|^refs/heads/|| || $ref =~ s|^refs/tags/|tags/|) {
-			return $ref;
-		} else {
-			return $ref;
-		}
-	}
-
-	my ($tagged, $branch, $found);
+	my ($exact,$found);
 	while (<STDIN>) {
-		my ($sha1, $ref, $deref) = /^(\S+)\s+(\S+?)(\^\{\})?$/;
+		my ($sha1, $ref, $deref) = /^(\S+)\s+([^^]+)(\S*)$/;
 		next unless ($sha1 eq $ARGV[1]);
-		$found = abbr($ref);
-		if ($deref && $ref eq "tags/$ARGV[2]") {
-			$tagged = $found;
-			last;
+		if ($ref eq $ARGV[0]) {
+			$exact = $ref;
 		}
-		if ($ref =~ m|/\Q$ARGV[0]\E$|) {
-			$exact = $found;
+		if ($sha1 eq $ARGV[0]) {
+			$found = $sha1;
 		}
 	}
-	if ($tagged) {
-		print "$tagged\n";
-	} elsif ($exact) {
+	if ($exact) {
 		print "$exact\n";
 	} elsif ($found) {
 		print "$found\n";
 	}
 '
 
-ref=$(git ls-remote "$url" | @@PERL@@ -e "$find_matching_ref" "$head" "$headrev" "$tag_name")
+ref=$(git ls-remote "$url" | @@PERL@@ -e "$find_matching_ref" "$head" "$headrev")
+
+if test -z "$ref"
+then
+	echo "warn: No match for $prettyhead found at $url" >&2
+	echo "warn: Are you sure you pushed '$prettyhead' there?" >&2
+	status=1
+fi
 
 url=$(git ls-remote --get-url "$url")
 
@@ -116,7 +116,7 @@ git show -s --format='The following changes since commit %H:
 
 are available in the git repository at:
 ' $merge_base &&
-echo "  $url${ref+ $ref}" &&
+echo "  $url $prettyhead" &&
 git show -s --format='
 for you to fetch changes up to %H:
 
@@ -129,34 +129,10 @@ then
 	echo "(from the branch description for $branch_name local branch)"
 	echo
 	git config "branch.$branch_name.description"
-fi &&
-
-if test -n "$tag_name"
-then
-	if test -z "$ref" || test "$ref" != "tags/$tag_name"
-	then
-		echo >&2 "warn: You locally have $tag_name but it does not (yet)"
-		echo >&2 "warn: appear to be at $url"
-		echo >&2 "warn: Do you want to push it there, perhaps?"
-	fi
-	git cat-file tag "$tag_name" |
-	sed -n -e '1,/^$/d' -e '/^-----BEGIN PGP /q' -e p
-	echo
-fi &&
-
-if test -n "$branch_name" || test -n "$tag_name"
-then
 	echo "----------------------------------------------------------------"
 fi &&
 
 git shortlog ^$baserev $headrev &&
 git diff -M --stat --summary $patch $merge_base..$headrev || status=1
 
-if test -z "$ref"
-then
-	echo "warn: No branch of $url is at:" >&2
-	git show -s --format='warn:   %h: %s' $headrev >&2
-	echo "warn: Are you sure you pushed '$head' there?" >&2
-	status=1
-fi
 exit $status
-- 
1.9.rc0.7.gd9bb4be.dirty

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

[Junio C Hamano]

Linus Torvalds <torvalds@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).

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

[Linus Torvalds]

On Wed, Jan 22, 2014 at 1:46 PM, Junio C Hamano <gitster@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

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

[Linus Torvalds]

On Wed, Jan 22, 2014 at 2:03 PM, Linus Torvalds
<torvalds@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

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

[Junio C Hamano]

Linus Torvalds <torvalds@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.

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

[Linus Torvalds]

On Wed, Jan 22, 2014 at 2:14 PM, Junio C Hamano <gitster@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

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

[Junio C Hamano]

Junio C Hamano <gitster@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 ;-).