patch, 3 partsrequest-pull: do something if $3 is passed

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

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

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(+)

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

Paolo Bonzini <bonzini@gnu.org> writes:

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.

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.

On 16/02/2015 20:47, Junio C Hamano wrote:

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.

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.

I agree with that change.

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.

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

Paolo Bonzini <bonzini@gnu.org> writes:

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

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

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

On 17/02/2015 20:57, Junio C Hamano wrote:

Yes, it is.

FWIW, that would always be wrong for my scenario.

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

Paolo

On Tue, Feb 17, 2015 at 12:34 PM, Paolo Bonzini <bonzini@gnu.org> wrote:

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

On 17/02/2015 21:42, Linus Torvalds wrote:

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.

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

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

On Tue, Feb 17, 2015 at 12:53 PM, Paolo Bonzini <bonzini@gnu.org> wrote:

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.

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.

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

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.

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

On Tue, Feb 17, 2015 at 1:10 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:

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

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

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.

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

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

On Tue, Feb 17, 2015 at 1:03 PM, Junio C Hamano <gitster@pobox.com> wrote:

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.

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