[PATCH 0/3] request-pull: do something if $3 is passed

[PATCH 0/3] request-pull: do something if $3 is passed

[Paolo Bonzini]

From: Paolo Bonzini <pbonzini@redhat.com>

After updating to git 2.3.0, "git request-pull" is stubbornly complaining
that I lack a matching tag on the remote side unless I pass the third
argument.  But I did prepare and push a signed tag.

This looks like a bug to me; when $3 is not passed git will try to use
"HEAD" as the default but it cannot be resolved to a tag, neither locally
(patch 2) nor remotely (patch 3).

Patch 1 is a simple testcase fix.

Paolo

Paolo Bonzini (3):
  request-pull: fix expected format in tests
  request-pull: use "git tag --points-at" to detect local tags
  request-pull: find matching tag or branch name on remote side

 git-request-pull.sh     | 15 +++++++++++----
 t/t5150-request-pull.sh |  5 ++---
 2 files changed, 13 insertions(+), 7 deletions(-)

-- 
2.3.0

[PATCH 1/3] request-pull: fix expected format in tests

[Paolo Bonzini]

From: Paolo Bonzini <pbonzini@redhat.com>

"tag foo" in requests has been replaced with "tags/foo" (commit f032d66,
request-pull: do not emit "tag" before the tagname, 2011-12-19).  Adjust
the parsing script to match; since the new format does not have spaces,
doing nothing is fine.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 t/t5150-request-pull.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh
index 82c33b8..8b19279 100755
--- a/t/t5150-request-pull.sh
+++ b/t/t5150-request-pull.sh
@@ -67,11 +67,10 @@ test_expect_success 'setup: two scripts for reading pull requests' '
 
 	cat <<-\EOT >read-request.sed &&
 	#!/bin/sed -nf
-	# Note that a request could ask for "tag $tagname"
+	# Note that a request could ask for "tags/$tagname"
 	/ in the git repository at:$/!d
 	n
 	/^$/ n
-	s/ tag \([^ ]*\)$/ tag--\1/
 	s/^[ 	]*\(.*\) \([^ ]*\)/please pull\
 	\1\
 	\2/p
-- 
2.3.0

[PATCH 2/3] request-pull: use "git tag --points-at" to detect local tags

[Paolo Bonzini]

From: Paolo Bonzini <pbonzini@redhat.com>

If the third argument is not passed, "git show-ref --tags HEAD" will
never return anything and git-request-pull will never detect a tag
name.

Instead, "git tag --points-at" can find it.  Use it if "git show-ref"
fails.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 git-request-pull.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/git-request-pull.sh b/git-request-pull.sh
index d5500fd..a507006 100755
--- a/git-request-pull.sh
+++ b/git-request-pull.sh
@@ -58,6 +58,7 @@ pretty_remote=${remote#refs/}
 pretty_remote=${pretty_remote#heads/}
 head=$(git symbolic-ref -q "$local")
 head=${head:-$(git show-ref --heads --tags "$local" | cut -d' ' -f2)}
+head=${head:-$(git tag --points-at "$local" | sed 's,^,refs/tags/,')}
 head=${head:-$(git rev-parse --quiet --verify "$local")}
 
 # None of the above? Bad.
-- 
2.3.0

[PATCH 3/3] request-pull: find matching tag or branch name on remote side

[Paolo Bonzini]

From: Paolo Bonzini <pbonzini@redhat.com>

If the third argument is not passed to "git request-pull", the
find_matching_ref script will look for HEAD in the remote side
which does not work.  Instead, default to the ref names found
via "git show-ref" or "git tag".

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 git-request-pull.sh     | 14 ++++++++++----
 t/t5150-request-pull.sh |  2 +-
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/git-request-pull.sh b/git-request-pull.sh
index a507006..fcbe383 100755
--- a/git-request-pull.sh
+++ b/git-request-pull.sh
@@ -54,8 +54,6 @@ fi
 local=${3%:*}
 local=${local:-HEAD}
 remote=${3#*:}
-pretty_remote=${remote#refs/}
-pretty_remote=${pretty_remote#heads/}
 head=$(git symbolic-ref -q "$local")
 head=${head:-$(git show-ref --heads --tags "$local" | cut -d' ' -f2)}
 head=${head:-$(git tag --points-at "$local" | sed 's,^,refs/tags/,')}
@@ -64,6 +62,14 @@ head=${head:-$(git rev-parse --quiet --verify "$local")}
 # None of the above? Bad.
 test -z "$head" && die "fatal: Not a valid revision: $local"
 
+#
+# If $3 was not there, the remote name should be the same
+# as the locally detected name
+#
+remote=${remote:-$head}
+pretty_remote=${remote#refs/}
+pretty_remote=${pretty_remote#heads/}
+
 # 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)
@@ -111,12 +117,12 @@ find_matching_ref='
 	}
 '
 
-ref=$(git ls-remote "$url" | @@PERL@@ -e "$find_matching_ref" "${remote:-HEAD}" "$headrev")
+ref=$(git ls-remote "$url" | @@PERL@@ -e "$find_matching_ref" "$remote" "$headrev")
 
 if test -z "$ref"
 then
 	echo "warn: No match for commit $headrev found at $url" >&2
-	echo "warn: Are you sure you pushed '${remote:-HEAD}' there?" >&2
+	echo "warn: Are you sure you pushed '$remote' there?" >&2
 	status=1
 fi
 
diff --git a/t/t5150-request-pull.sh b/t/t5150-request-pull.sh
index 8b19279..11ba8ff 100755
--- a/t/t5150-request-pull.sh
+++ b/t/t5150-request-pull.sh
@@ -178,7 +178,7 @@ test_expect_success 'request asks HEAD to be pulled' '
 		read repository &&
 		read branch
 	} <digest &&
-	test -z "$branch"
+	test "$branch" = "tags/full"
 
 '
 
-- 
2.3.0

Re: [PATCH 0/3] request-pull: do something if $3 is passed

[Junio C Hamano]

Paolo Bonzini <bonzini@gnu.org> writes:

> From: Paolo Bonzini <pbonzini@redhat.com>
>
> After updating to git 2.3.0, "git request-pull" is stubbornly complaining
> that I lack a matching tag on the remote side unless I pass the third
> argument.  But I did prepare and push a signed tag.

A few questions.

 - what old version did you update from?  I think the "correct
   over-eager dwimming" change was from v2.0 days.

 - what exactly do you mean by "stubbornly complain"?  I think we
   say something about HEAD not matching the HEAD over there, which
   I think is bogus (we should instead say things about the branch
   you are on and the branch over there with the same name) and is
   worth fixing.

> This looks like a bug to me; when $3 is not passed git will try to use
> "HEAD" as the default but it cannot be resolved to a tag, neither locally
> (patch 2) nor remotely (patch 3).

An earlier 024d34cb (request-pull: more strictly match local/remote
branches, 2014-01-22) deliberately disabled over-eager DWIMming when
the $3-rd argument _is_ given.  It didn't say much about what should
happen when it is missing.

I am torn about your changes.

One part of me feel that not giving the $3-rd argument should behave
the same way as if you gave the name of the current branch as the
$3-rd argument.  DWIMming from local HEAD to a local branch name
(e.g. 'master') may be OK and necessary (I already said it is worth
fixing above).  But we should not be resurrecting the over-eager
DWIMming from that point---not from a local branch name to a tag
that points at it, which was what 024d34cb wanted to forbid.

On the other hand, I can also understand (not necessarily agree
with) a view that not giving the $3-rd argument is an explicit
user's wish to us to DWIM as much as we want.  But again, that
directly contradicts with the desire of 024d34cb.

So,... I dunno.

I'd be more comfortable if 2/3 and 3/3 were replaced with something
like "do not ask HEAD to be pulled, but always require a specific
ref to be pulled", by dereferencing HEAD locally to a branch name,
and behave as if that name was given to $3 from the command line,
without doing any other changes (like turning that branch name that
was implicitly given into a tag that happens to point at it).

Thanks.

>
> Patch 1 is a simple testcase fix.
>
> Paolo
>
> Paolo Bonzini (3):
>   request-pull: fix expected format in tests
>   request-pull: use "git tag --points-at" to detect local tags
>   request-pull: find matching tag or branch name on remote side
>
>  git-request-pull.sh     | 15 +++++++++++----
>  t/t5150-request-pull.sh |  5 ++---
>  2 files changed, 13 insertions(+), 7 deletions(-)

Re: [PATCH 0/3] request-pull: do something if $3 is passed

[Paolo Bonzini]

On 16/02/2015 20:47, Junio C Hamano wrote:
> Paolo Bonzini <bonzini@gnu.org> writes:
> 
>> From: Paolo Bonzini <pbonzini@redhat.com>
>>
>> After updating to git 2.3.0, "git request-pull" is stubbornly complaining
>> that I lack a matching tag on the remote side unless I pass the third
>> argument.  But I did prepare and push a signed tag.
> 
> A few questions.
> 
>  - what old version did you update from?  I think the "correct
>    over-eager dwimming" change was from v2.0 days.

I upgraded from 1.9.  My workflow is to make a signed tag, push it and
do "git request-pull origin/master <url>".

My branches have a different name locally vs. remotely (e.g.
"kvm-master" and "kvm-next" locally vs. refs/heads/master and
refs/heads/next remotely) exactly to avoid overeager matching in
git-request-pull.  I only ever want to request pulls based on signed tags.

>  - what exactly do you mean by "stubbornly complain"?  I think we
>    say something about HEAD not matching the HEAD over there, which
>    I think is bogus (we should instead say things about the branch
>    you are on and the branch over there with the same name) and is
>    worth fixing.

I tried both "git checkout kvm-next" and "git checkout tags/for-linus",
and it still complains.

What you refer to is, I think, fixed by patch 3.  The find_matching_ref
script does not work if its first argument is HEAD.  So patch 3 is
probably an improvement anyway for the "matching branch name" case, even
if my usecase involves tags rather than branches.

> An earlier 024d34cb (request-pull: more strictly match local/remote
> branches, 2014-01-22) deliberately disabled over-eager DWIMming when
> the $3-rd argument _is_ given.

I agree with that change.

> One part of me feel that not giving the $3-rd argument should behave
> the same way as if you gave the name of the current branch as the
> $3-rd argument.

This works well for workflows where you do pull requests based on
branches.  However Linus strongly encourages using signed tags.

I certainly can adjust my workflow for this.  For example I can add
something like this to my .gitconfig

	[request-pull]
		dwim = tags/for-linus

and add an alias that uses "git config request-pull.dwim" as the third
argument (other projects I work on obviously use different tag names :).

While similar, the two patches are different:

1) The usage of "git show-ref --heads --tags" looked like a feeble
attempt at DWIMming tags.  But I can see how that is supposed to work
only if $3 is specified.  Adding a usage of "git tag --points-at" would
go against the intentions of 024d34cb.  Perhaps restrict DWIMming to
signed and annotated tags only, through a new option to "git tag"?

2) Patch 3 makes sense independent of patch 2 and, as mentioned above,
it is probably a bugfix anyway.

> On the other hand, I can also understand (not necessarily agree
> with) a view that not giving the $3-rd argument is an explicit
> user's wish to us to DWIM as much as we want.  But again, that
> directly contradicts with the desire of 024d34cb.
> 
> So,... I dunno.

I don't know either.  Based on your answer, it seems like you are
focusing mostly on a branch-based workflow; the two definitely have
different requirements for DWIMming (since you cannot get a tag name via
"git symbolic-ref" for example).  On the other hand most of the
un-DWIMming changes were done by Linus who works a lot with (other
people's) signed tags...

Paolo

Re: [PATCH 0/3] request-pull: do something if $3 is passed

[Junio C Hamano]

Paolo Bonzini <bonzini@gnu.org> writes:

> On 16/02/2015 20:47, Junio C Hamano wrote:
>> Paolo Bonzini <bonzini@gnu.org> writes:
>> 
>>> From: Paolo Bonzini <pbonzini@redhat.com>
>>>
>>> After updating to git 2.3.0, "git request-pull" is stubbornly complaining
>>> that I lack a matching tag on the remote side unless I pass the third
>>> argument.  But I did prepare and push a signed tag.
>> 
>> A few questions.
>> 
>>  - what old version did you update from?  I think the "correct
>>    over-eager dwimming" change was from v2.0 days.
>
> I upgraded from 1.9.  My workflow is to make a signed tag, push it and
> do "git request-pull origin/master <url>".
>
> My branches have a different name locally vs. remotely (e.g.
> "kvm-master" and "kvm-next" locally vs. refs/heads/master and
> refs/heads/next remotely) exactly to avoid overeager matching in
> git-request-pull.  I only ever want to request pulls based on signed tags.

So I think you would want something like this:

    git tag -s for-linus kvm-next
    git push <url> kvm-next:next tags/for-linus
    git request-pull origin/master <url> for-linus

in the post 2.0 world with 024d34cb (request-pull: more strictly
match local/remote branches, 2014-01-22)?

>>  - what exactly do you mean by "stubbornly complain"?  I think we
>>    say something about HEAD not matching the HEAD over there, which
>>    I think is bogus (we should instead say things about the branch
>>    you are on and the branch over there with the same name) and is
>>    worth fixing.
>
> I tried both "git checkout kvm-next" and "git checkout tags/for-linus",
> and it still complains.

Sorry, I was asking what you mean by "complains" (i.e. the exact
error message).  I was and am guessing it is something like this: 

    warn: No match for commit 3188ab3... found at <url>
    warn: Are you sure you pushed 'HEAD' there?

Asking to pull 'HEAD' may be often a wrong thing to do, and I
wouldn't mind if this sequence:

	git checkout kvm-next
        git request-pull origin/master <url>

behaved the same way as

        git request-pull origin/master <url> kvm-next

But I do not know if the implicit HEAD should DWIM locally to this:

        git request-pull origin/master <url> for-linus

> ...  Based on your answer, it seems like you are focusing mostly
> on a branch-based workflow; ...

Not really.  I am focusing mostly on not breaking what 024d34cb0 and
dc2eacc58c fixed earlier.

> ... the two definitely have
> different requirements for DWIMming (since you cannot get a tag name via
> "git symbolic-ref" for example).  On the other hand most of the
> un-DWIMming changes were done by Linus who works a lot with (other
> people's) signed tags...

Re: [PATCH 0/3] request-pull: do something if $3 is passed

[Paolo Bonzini]

On 17/02/2015 20:57, Junio C Hamano wrote:
> Sorry, I was asking what you mean by "complains" (i.e. the exact
> error message).  I was and am guessing it is something like this: 
> 
>     warn: No match for commit 3188ab3... found at <url>
>     warn: Are you sure you pushed 'HEAD' there?

Yes, it is.

> Asking to pull 'HEAD' may be often a wrong thing to do, and I
> wouldn't mind if this sequence:
> 
> 	git checkout kvm-next
>         git request-pull origin/master <url>
> 
> behaved the same way as
> 
>         git request-pull origin/master <url> kvm-next

FWIW, that would always be wrong for my scenario.

> But I do not know if the implicit HEAD should DWIM locally to this:
> 
>         git request-pull origin/master <url> for-linus

I guess only Linus could answer that, since he wrote 024d34cb0 and he
knows the intent better than anyone else.

Paolo

Re: [PATCH 0/3] request-pull: do something if $3 is passed

[Linus Torvalds]

On Tue, Feb 17, 2015 at 12:34 PM, Paolo Bonzini <bonzini@gnu.org> wrote:
>
> I guess only Linus could answer that, since he wrote 024d34cb0 and he
> knows the intent better than anyone else.

I don't even understand your problem.

You said

  "when $3 is not passed git will try to use "HEAD" as the default but
it cannot be resolved to a tag, neither locally (patch 2) nor remotely
(patch 3)"

which makes absolutely no sense.

HEAD is not a tag. Never has been, never will be. If you want me to
pull a tag, then you damn well should say what tag you want, not just
randomly say HEAD.

So what is it you want to do? At no point is "HEAD should resolve as a
tag" sensible.

                      Linus

Re: [PATCH 0/3] request-pull: do something if $3 is passed

[Paolo Bonzini]

On 17/02/2015 21:42, Linus Torvalds wrote:
>   "when $3 is not passed git will try to use "HEAD" as the default but
> it cannot be resolved to a tag, neither locally (patch 2) nor remotely
> (patch 3)"
> 
> which makes absolutely no sense.

Indeed, that's why I wrote patches even though I did find the patches
that you wrote for 2.0.

Without $3, git tries to do things that make no sense like "git show-ref
--heads --tags HEAD"; or that make little sense when requesting a pull,
like looking for HEAD in the output of "git ls-remote".  But from the
release notes of 2.0 it looks like it's intended and the script is just
taking shortcuts.

> HEAD is not a tag. Never has been, never will be. If you want me to
> pull a tag, then you damn well should say what tag you want, not just
> randomly say HEAD.

Ok, in 1.9.x I used to not say anything; if the new workflow is to
always specify a tag, that's okay.

> So what is it you want to do? At no point is "HEAD should resolve as a
> tag" sensible.

I wanted git to find the matching tag on the remote side when I use "git
request-pull origin/master URL" with no third parameter, since I never
request pulls except with a single signed tag.  But I'll adjust my aliases.

Paolo

Re: [PATCH 0/3] request-pull: do something if $3 is passed

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> HEAD is not a tag. Never has been, never will be. If you want me to
> pull a tag, then you damn well should say what tag you want, not just
> randomly say HEAD.
>
> So what is it you want to do? At no point is "HEAD should resolve as a
> tag" sensible.

"HEAD should resolve as a tag" is not sensible, but "HEAD should
locally DWIM to something sensible" is still possible, no?

We could for example make the rule for unset $3 case like this,
instead of the current "missing $3 is a request to pull HEAD":

    If you have one and only one signed tag that happens to point at
    the commit sitting at HEAD, behave as if that tag was given as
    the third argument from the command line.

    Otherwise, if you are on a branch, behave as if that branch was
    given as the third argument from the command line.

    If you are not on any branch, error out.

Re: [PATCH 0/3] request-pull: do something if $3 is passed

[Linus Torvalds]

On Tue, Feb 17, 2015 at 12:53 PM, Paolo Bonzini <bonzini@gnu.org> wrote:
>
> Without $3, git tries to do things that make no sense like "git show-ref
> --heads --tags HEAD"; or that make little sense when requesting a pull,
> like looking for HEAD in the output of "git ls-remote".  But from the
> release notes of 2.0 it looks like it's intended and the script is just
> taking shortcuts.

It is *you* who make no sense.

Looking for HEAD in "git ls-remote"? Perfectly sensible:

    [torvalds@i7 linux]$ git ls-remote origin | grep HEAD
    cc4f9c2a91b7be7b3590bb1cbe8148873556aa3f HEAD

that's the default thing when you don't specify any particular branch or tag.

> Ok, in 1.9.x I used to not say anything; if the new workflow is to
> always specify a tag, that's okay.

Indeed. You have to specify what you want me to pull. Exactly because
in 1.9.x people didn't, and I got *really* tired of getting bogus pull
requests that didn't work, or pointed at the wrong branch when people
had multiple branches with the same contents etc.

> I wanted git to find the matching tag on the remote side when I use "git
> request-pull origin/master URL" with no third parameter, since I never
> request pulls except with a single signed tag.

The thing is, HEAD works. Not for you, because you don't use HEAD. But
because you don't use HEAD, you shouldn't use the default.

I *would* agree to making $3 be mandatory, but there are still people
out there who just use a single branch per repository and no signed
branches. Which is the only reason that "default HEAD' thing exists.

                       :Linus

Re: [PATCH 0/3] request-pull: do something if $3 is passed

[Linus Torvalds]

On Tue, Feb 17, 2015 at 1:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> "HEAD should resolve as a tag" is not sensible, but "HEAD should
> locally DWIM to something sensible" is still possible, no?

I disagree. Why? Because what you have locally is *not* necessarily
the same thing you have remotely.

And that's *exactly* why people used to send me broken pull requests.
"git pull-request" would guess on things, and it would get the guesses
wrong, and write the pull request wrong.

> We could for example make the rule for unset $3 case like this:
> instead of the current "missing $3 is a request to pull HEAD":
>
>     If you have one and only one signed tag that happens to point at
>     the commit sitting at HEAD, behave as if that tag was given as
>     the third argument from the command line.

If you verify that "one and only" to be true both locally and
remotely, then I guess I would be ok with it. But it really would have
to be unique. And truly unique, as in no confusion about branches or
tags, only one or the other. Because the "tag vs branch" was one of
the main sources of confusion that made me repeatedly get bad pull
requests, particularly when there was something locally that wasn't
actually named the same thing remotely.

                         Linus

Re: [PATCH 0/3] request-pull: do something if $3 is passed

[Paolo Bonzini]

> Looking for HEAD in "git ls-remote"? Perfectly sensible:
> 
>     [torvalds@i7 linux]$ git ls-remote origin | grep HEAD
>     cc4f9c2a91b7be7b3590bb1cbe8148873556aa3f HEAD
> 
> that's the default thing when you don't specify any particular branch or tag.

Sure.  But if I got a pull request saying "please pull
git://example.org/foo.git HEAD" I would think that the sender
messed up the pull request.  So *in the context of git-request-pull*
${remote:-HEAD} makes little sense to me.

But hey, you said it's me who makes no sense.  Maybe I really don't.

> The thing is, HEAD works. Not for you, because you don't use HEAD. But
> because you don't use HEAD, you shouldn't use the default.

Oki.  Will adjust my scripts.  Junio, you may still want to apply patch
1 if only for documentation purposes (the "tag foo" functionality is
unused in the rest of the test).

Paolo

Re: [PATCH 0/3] request-pull: do something if $3 is passed

[Linus Torvalds]

On Tue, Feb 17, 2015 at 1:10 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> Sure.  But if I got a pull request saying "please pull
> git://example.org/foo.git HEAD" I would think that the sender
> messed up the pull request.  So *in the context of git-request-pull*
> ${remote:-HEAD} makes little sense to me.

Umm. If somebody actually leaves off the third argument THAT IS NOT AT
ALL what it prints.

It will show

    The following changes since commit <base>...

        .. base commit description ..

   are available in the git repository at:

      git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

    for you to fetch changes up to cc4f9c2a91b7be7b3590bb1cbe8148873556aa3f:
    ...

IOW, it does exactly the right thing. It gives the contents of HEAD,
but it doesn't actually say HEAD anywhere.

And just look at lkml. The above kind of branch-less and tag-less pull
requests are still fairly common. It's the original git model, and it
may be a bit archaic, and I much prefer people to send me signed tags,
but hey, that's what "don't mention a branch or tag" means.

And no, I don't think git request-pull is at all different from other
git commands. "git log" means the same thing as "git log HEAD". Exact
same thing, and nobody would actually write out that HEAD (except
inside scripts, perhaps).

So basically I agree that git request-pull has changed behavior, but
the new behavior is *more* in line with other git commands, and the
old behavior was actually really really odd with that whole extensive
"guess what the user means". No other git command ever did that
guessing thing (ok, famous last words, maybe somebody can come up with
one), and not mentioning a branch/tag/commit explicitly pretty much
always means "HEAD".

                      :Linus

Re: [PATCH 0/3] request-pull: do something if $3 is passed

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> So basically I agree that git request-pull has changed behavior, but
> the new behavior is *more* in line with other git commands, and the
> old behavior was actually really really odd with that whole extensive
> "guess what the user means". No other git command ever did that
> guessing thing (ok, famous last words, maybe somebody can come up with
> one), and not mentioning a branch/tag/commit explicitly pretty much
> always means "HEAD".

OK.

There may be some stuff that DWIMs "HEAD" to something other than
the commit that is at the tip of HEAD, but I agree that the fewer we
have such oddballs, the better.

Thanks.