[PATCH] push: support remote branches in guess_ref DWIM

[PATCH] push: support remote branches in guess_ref DWIM

[Jeff King]

When we get an unqualified dest refspec like "foo", we try
to guess its full ref path (like "refs/heads/foo") based on
the source ref. Commit f8aae12 mapped local heads to remote
heads, and local tags to remote tags.

This commit maps local tracking branches under
"refs/remotes" to remote branch heads, so

  git push origin origin/branch:renamed-branch

pushes to refs/heads/renamed-branch.

Signed-off-by: Jeff King <peff@peff.net>
---
This came from a discussion on IRC. I don't see any reason not to do
this; would people really expect it to push into refs/remotes/ on the
other side (right now, it complains and dies)?

A related issue (which exists even without this patch) is that doing
this:

  master:remotes/incoming/master

will create "refs/heads/remotes/incoming/master". Perhaps we should DWYM
a little more and recognize "heads", "remotes", and "tags" as special.

 remote.c              |    2 +-
 t/t5516-fetch-push.sh |    9 +++++++++
 2 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/remote.c b/remote.c
index 73d33f2..1a7c81e 100644
--- a/remote.c
+++ b/remote.c
@@ -998,7 +998,7 @@ static char *guess_ref(const char *name, struct ref *peer)
 	if (!r)
 		return NULL;
 
-	if (!prefixcmp(r, "refs/heads/"))
+	if (!prefixcmp(r, "refs/heads/") || !prefixcmp(r, "refs/remotes/"))
 		strbuf_addstr(&buf, "refs/heads/");
 	else if (!prefixcmp(r, "refs/tags/"))
 		strbuf_addstr(&buf, "refs/tags/");
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 6889a53..692c773 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -383,6 +383,15 @@ test_expect_success 'push tag with non-existant, incomplete dest' '
 
 '
 
+test_expect_success 'push remote branch with non-existant, incomplete dest' '
+
+	mk_test &&
+	git update-ref refs/remotes/foo/bar master &&
+	git push testrepo foo/bar:branch &&
+	check_push_result $the_commit heads/branch
+
+'
+
 test_expect_success 'push sha1 with non-existant, incomplete dest' '
 
 	mk_test &&
-- 
1.6.5.1.201.g7feba

Re: [PATCH] push: support remote branches in guess_ref DWIM

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> When we get an unqualified dest refspec like "foo", we try
> to guess its full ref path (like "refs/heads/foo") based on
> the source ref. Commit f8aae12 mapped local heads to remote
> heads, and local tags to remote tags.
>
> This commit maps local tracking branches under
> "refs/remotes" to remote branch heads, so
>
>   git push origin origin/branch:renamed-branch
>
> pushes to refs/heads/renamed-branch.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This came from a discussion on IRC. I don't see any reason not to do
> this; would people really expect it to push into refs/remotes/ on the
> other side (right now, it complains and dies)?

As _our_ origin can never be _their_ origin if we are clone of them, I do
not think anybody sane would expect it to push into refs/remotes/origin/
to begin with.

But "not in refs/remotes/" does not automatically mean "the only sensible
place is refs/heads", does it?  "We do not know what kind of mistake the
user is trying to make" could be more plausible answer, and in that case,
"complain and die" may be a more valid course of action.

For example,

    git push origin origin/master:refs/heads/master

is most likely to be a mistake.  The only situation something similar to
this makes sense is where you pushed out a bogus commit earlier and are
trying to correct it perhaps with

	git push origin +origin/master@{1}:refs/heads/master

but you need to force it and go back in the reflog.

I know you are doing this DWIM only when $dst does not exist over there,
and the "pushing old master back to master" above is a different scenario,
but if "git push origin origin/master:master" shouldn't update their local
master, "git push origin origin/master:naster" shouldn't update their
local naster, in order to avoid confusion arising from inconsistency, no?

Another possibility is to see the type (not refname) of $src and push all
commits to refs/heads/$dst and everything else (most likely tags) to
refs/tags/$dst, to simplify the rule to make it easier to explain and
remember, but there are lightweight tags that make this idea unworkable.

> A related issue (which exists even without this patch) is that doing
> this:
>
>   master:remotes/incoming/master
>
> will create "refs/heads/remotes/incoming/master". Perhaps we should DWYM
> a little more and recognize "heads", "remotes", and "tags" as special.

Yes, it is an independent issue; I think correcting this DWIM (or at least
"warning" if not refusing to create remotes/ under refs/heads/) might be a
good idea.

Re: [PATCH] push: support remote branches in guess_ref DWIM

[Jeff King]

On Mon, Oct 26, 2009 at 04:31:57PM -0700, Junio C Hamano wrote:

> As _our_ origin can never be _their_ origin if we are clone of them, I do
> not think anybody sane would expect it to push into refs/remotes/origin/
> to begin with.

OK, I agree.

> But "not in refs/remotes/" does not automatically mean "the only sensible
> place is refs/heads", does it?  "We do not know what kind of mistake the
> user is trying to make" could be more plausible answer, and in that case,
> "complain and die" may be a more valid course of action.

The thing is that I can't think of another sensible place. And this
sensible place is useful for one particular action: renaming a remote
branch, like this:

  $ git fetch ;# presumably gets origin/branch
  $ git push origin/branch:renamed-branch

which is much nicer than exposing clueless users to
":refs/heads/renamed-branch".

> For example,
> 
>     git push origin origin/master:refs/heads/master
> 
> is most likely to be a mistake.  The only situation something similar to
> this makes sense is where you pushed out a bogus commit earlier and are
> trying to correct it perhaps with

I'm not sure why it's likely to be a mistake. I've given the one use
case I can think of where you _do_ want to do it. But I'm not sure why
you would accidentally provide something in refs/remotes, or not want to
be pushing to refs/heads. Where _else_ do you push, except for tags?

Am I missing some part of your argument?

> > A related issue (which exists even without this patch) is that doing
> > this:
> >
> >   master:remotes/incoming/master
> >
> > will create "refs/heads/remotes/incoming/master". Perhaps we should DWYM
> > a little more and recognize "heads", "remotes", and "tags" as special.
> 
> Yes, it is an independent issue; I think correcting this DWIM (or at least
> "warning" if not refusing to create remotes/ under refs/heads/) might be a
> good idea.

OK, I'll try to work up a patch.

-Peff

Re: [PATCH] push: support remote branches in guess_ref DWIM

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Oct 26, 2009 at 04:31:57PM -0700, Junio C Hamano wrote:
>
>> As _our_ origin can never be _their_ origin if we are clone of them, I do
>> not think anybody sane would expect it to push into refs/remotes/origin/
>> to begin with.
>
> OK, I agree.
>
>> But "not in refs/remotes/" does not automatically mean "the only sensible
>> place is refs/heads", does it?  "We do not know what kind of mistake the
>> user is trying to make" could be more plausible answer, and in that case,
>> "complain and die" may be a more valid course of action.
>
> The thing is that I can't think of another sensible place. And this
> sensible place is useful for one particular action: renaming a remote
> branch, like this:
>
>   $ git fetch ;# presumably gets origin/branch
>   $ git push origin/branch:renamed-branch
>
> which is much nicer than exposing clueless users to
> ":refs/heads/renamed-branch".

You would need to expose ":refs/heads/branch" to make this a rename, not a
copy, wouldn't you?

>> For example,
>> 
>>     git push origin origin/master:refs/heads/master
>> 
>> is most likely to be a mistake.  The only situation something similar to
>> this makes sense is where you pushed out a bogus commit earlier and are
>> trying to correct it perhaps with
>
> I'm not sure why it's likely to be a mistake.
> ...
> Am I missing some part of your argument?

I do not see much point (other than your "rename" example) in pushing what
you got from the other end without changing anything yourself back to the
same remote.

There was a thread in which people argued that the primary thing that is
dangerous in this sequence

    $ git checkout origin/next; work-commit; work-commit; ...

is when you leave the detached HEAD state without saving it to a local
branch.  I think what is more dangerous is that it will not give the user
a solid understanding that these commits do _not_ change origin/next in
any way.  After doing the above, it is understandable that a novice would
mistakenly think: "I started from origin/next and built some, let's push
the result".

	Side note: This is the reason why I think Dscho's "git checkout
        next" that dwims to "-t -b next origin/next" is OK (perhaps on the
        right side of the borderline), because it is more clear that this
        is about creating and using your own "next", compared to the
        existing "-t origin/next" shorthand.  The latter risks imprinting
        a misconception on an uninitiated that we are somehow working on
        origin/next.

With such a misconception, you will see the likely mistake here, too:

    $ git push origin origin/next:refs/heads/next

If "rename" is the _only_ valid reason you might want to push what you got
from there back to the same remote, _and_ if "rename" is something very
often needed, I think we would prefer to have a way to support that
operation directly, instead of more general DWIM that would risk passing
mistakes like the above unwarned.

> ...  But I'm not sure why
> you would accidentally provide something in refs/remotes, or not want to
> be pushing to refs/heads. Where _else_ do you push, except for tags?

The above "checking out origin/next" illustrates the mistake on the $src
side, not on the $dst side.  That's why my alternative solution in the
previous message was to see the type of $src and push commits to local
branches, exactly because "where else do you push".  

IOW, it's between "prevent push with dubious $src from happening in the
first place" vs "give them rope".  Historically I always sided with the
latter camp, but I am trying to play a devil's advocate for a change ;-).

Re: [PATCH] push: support remote branches in guess_ref DWIM

[Jeff King]

On Tue, Oct 27, 2009 at 03:33:16PM -0700, Junio C Hamano wrote:

> >   $ git fetch ;# presumably gets origin/branch
> >   $ git push origin/branch:renamed-branch
> >
> > which is much nicer than exposing clueless users to
> > ":refs/heads/renamed-branch".
> 
> You would need to expose ":refs/heads/branch" to make this a rename, not a
> copy, wouldn't you?

Yeah, you're right. This was based on an actual user request, and I
didn't think too closely about the other steps. But since the deletion
is of an existing branch, you should be able to do that without
refs/heads. So:

  $ git push origin origin/branch:renamed-branch
  $ git push origin :branch

Which of course you could do in one command if you wanted to live
(more) dangerously.

> > Am I missing some part of your argument?
> 
> I do not see much point (other than your "rename" example) in pushing what
> you got from the other end without changing anything yourself back to the
> same remote.

I don't either; my hope was that we can make that case a little bit
easier without creating undue hardship for anybody else.

> There was a thread in which people argued that the primary thing that is
> dangerous in this sequence
> 
>     $ git checkout origin/next; work-commit; work-commit; ...
> 
> is when you leave the detached HEAD state without saving it to a local
> branch.  I think what is more dangerous is that it will not give the user
> a solid understanding that these commits do _not_ change origin/next in
> any way.  After doing the above, it is understandable that a novice would
> mistakenly think: "I started from origin/next and built some, let's push
> the result".

I suppose it's possible. I don't have any evidence that users actually
think that way.

> With such a misconception, you will see the likely mistake here, too:
> 
>     $ git push origin origin/next:refs/heads/next
> 
> If "rename" is the _only_ valid reason you might want to push what you got
> from there back to the same remote, _and_ if "rename" is something very
> often needed, I think we would prefer to have a way to support that
> operation directly, instead of more general DWIM that would risk passing
> mistakes like the above unwarned.

OK, I can buy that. It would be much nicer even to support explicit
renaming (in fact, the user request started with that, and I just didn't
want to give them an answer that involved refs/heads/, which I think is
unnecessarily scary to users).

> IOW, it's between "prevent push with dubious $src from happening in the
> first place" vs "give them rope".  Historically I always sided with the
> latter camp, but I am trying to play a devil's advocate for a change ;-).

Heh. Thanks for explaining your thinking.

Let's scrap this for now, then. Remote rename doesn't actually come up
very often, and I agree that it would be nice to have an actual atomic
movement, which is what people really want.

-Peff