git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* git checkout -b origin/mybranch origin/mybranch
@ 2009-03-12 11:36 John Tapsell
  2009-03-12 11:40 ` Sverre Rabbelier
  2009-03-12 11:43 ` Johannes Schindelin
  0 siblings, 2 replies; 20+ messages in thread
From: John Tapsell @ 2009-03-12 11:36 UTC (permalink / raw)
  To: Git List

Hey all,

  One of my collegues did:

git checkout origin/somebranch

  git complained that they need to specify the name with -b.  So they did:

git checkout -b origin/somebranch origin/somebranch

  Git accepts this with no problems, but boy - all hell broke loose.
Doing a push or pull gave errors, because "origin/somebranch" is now
ambigous (since there is two of them).  They can't even:  "git
checkout -b somebranch origin/somebranch"  anymore, since
"origin/somebranch" is ambigous.  It all got into a mess.

  I've sort it out now, but I'd like to request that git doesn't so
easily let the user shoot themselves in the foot.

  I propose that creating a branch called  "origin/*" or "remotes/*"
gives at _least_ a warning, and preferably an error (overrideable with
--force for people who really really want to do it)

John Tapsell

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: git checkout -b origin/mybranch origin/mybranch
  2009-03-12 11:36 git checkout -b origin/mybranch origin/mybranch John Tapsell
@ 2009-03-12 11:40 ` Sverre Rabbelier
  2009-03-12 11:43 ` Johannes Schindelin
  1 sibling, 0 replies; 20+ messages in thread
From: Sverre Rabbelier @ 2009-03-12 11:40 UTC (permalink / raw)
  To: John Tapsell; +Cc: Git List

Heya,

On Thu, Mar 12, 2009 at 12:36, John Tapsell <johnflux@gmail.com> wrote:
>  git complained that they need to specify the name with -b.  So they did:
>
> git checkout -b origin/somebranch origin/somebranch

Slight unrelated, do they know about -t?

$ git checkout -t origin/somebranch

It will automagically create a branch named 'somebranch' tracking
'origin/somebranch'!

-- 
Cheers,

Sverre Rabbelier

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: git checkout -b origin/mybranch origin/mybranch
  2009-03-12 11:36 git checkout -b origin/mybranch origin/mybranch John Tapsell
  2009-03-12 11:40 ` Sverre Rabbelier
@ 2009-03-12 11:43 ` Johannes Schindelin
  2009-03-12 11:48   ` John Tapsell
  1 sibling, 1 reply; 20+ messages in thread
From: Johannes Schindelin @ 2009-03-12 11:43 UTC (permalink / raw)
  To: John Tapsell; +Cc: Git List

Hi,

On Thu, 12 Mar 2009, John Tapsell wrote:

>   One of my collegues did:
> 
> git checkout origin/somebranch
> 
>   git complained that they need to specify the name with -b.  So they did:
> 
> git checkout -b origin/somebranch origin/somebranch

Yeah, a pilot error.  It should have been

	$ git checkout -t origin/somebranch

I have to wonder, though, why "git checkout origin/somebranch" did not 
detach your HEAD.

>   Git accepts this with no problems, but boy - all hell broke loose.
> Doing a push or pull gave errors, because "origin/somebranch" is now
> ambigous (since there is two of them).

I strongly doubt that it gave errors, but rather warnings.  I have a 
repository where I get warnings all the time (it has a cvsimport and an 
origin remote), but it works without problems.

>  They can't even:  "git checkout -b somebranch origin/somebranch"  
> anymore, since "origin/somebranch" is ambigous.  It all got into a mess.

I wonder why you did not just "git branch -m somebranch".

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: git checkout -b origin/mybranch origin/mybranch
  2009-03-12 11:43 ` Johannes Schindelin
@ 2009-03-12 11:48   ` John Tapsell
  2009-03-12 13:02     ` Johannes Schindelin
  0 siblings, 1 reply; 20+ messages in thread
From: John Tapsell @ 2009-03-12 11:48 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Git List

2009/3/12 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> Hi,
>
> On Thu, 12 Mar 2009, John Tapsell wrote:
>
>>   One of my collegues did:
>>
>> git checkout origin/somebranch
>>
>>   git complained that they need to specify the name with -b.  So they did:
>>
>> git checkout -b origin/somebranch origin/somebranch
>
> Yeah, a pilot error.  It should have been
>
>        $ git checkout -t origin/somebranch

Maybe the error message for "git checkout origin/somebranch"  should
suggest:  git checkout -t origin/somebranch?

> I have to wonder, though, why "git checkout origin/somebranch" did not
> detach your HEAD.

It did.  But that doesn't affect doing "git checkout -b
origin/somebranch origin/somebranch"  afterwards.

>>   Git accepts this with no problems, but boy - all hell broke loose.
>> Doing a push or pull gave errors, because "origin/somebranch" is now
>> ambigous (since there is two of them).
>
> I strongly doubt that it gave errors, but rather warnings.  I have a
> repository where I get warnings all the time (it has a cvsimport and an
> origin remote), but it works without problems.
>
>>  They can't even:  "git checkout -b somebranch origin/somebranch"
>> anymore, since "origin/somebranch" is ambigous.  It all got into a mess.
>
> I wonder why you did not just "git branch -m somebranch".

Because they didn't know what on earth was going on, and git was
spitting out errors everywhere, they were afraid git would crash.

John

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: git checkout -b origin/mybranch origin/mybranch
  2009-03-12 11:48   ` John Tapsell
@ 2009-03-12 13:02     ` Johannes Schindelin
  2009-03-12 13:18       ` John Tapsell
  0 siblings, 1 reply; 20+ messages in thread
From: Johannes Schindelin @ 2009-03-12 13:02 UTC (permalink / raw)
  To: John Tapsell; +Cc: Git List

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1899 bytes --]

Hi,

On Thu, 12 Mar 2009, John Tapsell wrote:

> 2009/3/12 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>
> > On Thu, 12 Mar 2009, John Tapsell wrote:
> >
> >>   One of my collegues did:
> >>
> >> git checkout origin/somebranch
> >>
> >>   git complained that they need to specify the name with -b.  So they did:
> >>
> >> git checkout -b origin/somebranch origin/somebranch
> >
> > Yeah, a pilot error.  It should have been
> >
> >        $ git checkout -t origin/somebranch
> 
> Maybe the error message for "git checkout origin/somebranch"  should
> suggest:  git checkout -t origin/somebranch?
> 
> > I have to wonder, though, why "git checkout origin/somebranch" did not
> > detach your HEAD.
> 
> It did.  But that doesn't affect doing "git checkout -b
> origin/somebranch origin/somebranch"  afterwards.

So did the warning read something like this?

-- snip --
moving to "d36a18dc9cdf1dfce8632e42491b826387aa3230" which isn't a local 
branch
If you want to create a new branch from this checkout, you may do so
(now or later) by using -b with the checkout command again. Example:
  git checkout -b <new_branch_name>
-- snap --

?

If so, why did you not follow the recipe

	$ git checkout -b somebranch

but something like

	$ git checkout -b origin/somebranch origin/somebranch

?

I do not think there is a way to tell the user more explicitely what to do 
without insulting the user's intelligence ;-)

> > I wonder why you did not just "git branch -m somebranch".
> 
> Because they didn't know what on earth was going on, and git was 
> spitting out errors everywhere, they were afraid git would crash.

Yeah, I know those reactions.  All of a sudden, people panic, destroying 
everything in the process.  Do tell them to calm down first of all.  
Unless you do something like "rm -rf .git" or "git branch -d", it is 
actually pretty hard to lose data with Git.

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: git checkout -b origin/mybranch origin/mybranch
  2009-03-12 13:02     ` Johannes Schindelin
@ 2009-03-12 13:18       ` John Tapsell
  2009-03-12 13:43         ` Sverre Rabbelier
  2009-03-12 18:31         ` Junio C Hamano
  0 siblings, 2 replies; 20+ messages in thread
From: John Tapsell @ 2009-03-12 13:18 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Git List

2009/3/12 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
> Hi,
>
> On Thu, 12 Mar 2009, John Tapsell wrote:
>
>> 2009/3/12 Johannes Schindelin <Johannes.Schindelin@gmx.de>:
>>
>> > On Thu, 12 Mar 2009, John Tapsell wrote:
>> >
>> >>   One of my collegues did:
>> >>
>> >> git checkout origin/somebranch
>> >>
>> >>   git complained that they need to specify the name with -b.  So they did:
>> >>
>> >> git checkout -b origin/somebranch origin/somebranch
>> >
>> > Yeah, a pilot error.  It should have been
>> >
>> >        $ git checkout -t origin/somebranch
>>
>> Maybe the error message for "git checkout origin/somebranch"  should
>> suggest:  git checkout -t origin/somebranch?
>>
>> > I have to wonder, though, why "git checkout origin/somebranch" did not
>> > detach your HEAD.
>>
>> It did.  But that doesn't affect doing "git checkout -b
>> origin/somebranch origin/somebranch"  afterwards.
>
> So did the warning read something like this?
>
> -- snip --
> moving to "d36a18dc9cdf1dfce8632e42491b826387aa3230" which isn't a local
> branch
> If you want to create a new branch from this checkout, you may do so
> (now or later) by using -b with the checkout command again. Example:
>  git checkout -b <new_branch_name>
> -- snap --
>
> ?

I'm really not sure what point you're trying to prove.  Yes, we all
know that the user made a mistake in thinking that the new_branch_name
should be "origin/mybranch".

What I'm suggesting is that git tries to not let the user shoot
himself in the foot so easily.

I'm saying that:

git checkout -b origin/mybranch  origin/mybranch

Is probably a mistake by the user.  We should warn the user and point
them in the right direction.

John

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: git checkout -b origin/mybranch origin/mybranch
  2009-03-12 13:18       ` John Tapsell
@ 2009-03-12 13:43         ` Sverre Rabbelier
  2009-03-12 14:14           ` John Tapsell
  2009-03-12 15:21           ` Pieter de Bie
  2009-03-12 18:31         ` Junio C Hamano
  1 sibling, 2 replies; 20+ messages in thread
From: Sverre Rabbelier @ 2009-03-12 13:43 UTC (permalink / raw)
  To: John Tapsell; +Cc: Johannes Schindelin, Git List

Heya,

On Thu, Mar 12, 2009 at 14:18, John Tapsell <johnflux@gmail.com> wrote:
> Is probably a mistake by the user.  We should warn the user and point
> them in the right direction.

The point is that we _already_ warned the user (like Dscho pointed
out), and that(as you pointed out), it didn't work :P.

-- 
Cheers,

Sverre Rabbelier

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: git checkout -b origin/mybranch origin/mybranch
  2009-03-12 13:43         ` Sverre Rabbelier
@ 2009-03-12 14:14           ` John Tapsell
  2009-03-13 14:08             ` Michael J Gruber
  2009-03-12 15:21           ` Pieter de Bie
  1 sibling, 1 reply; 20+ messages in thread
From: John Tapsell @ 2009-03-12 14:14 UTC (permalink / raw)
  To: Sverre Rabbelier; +Cc: Johannes Schindelin, Git List

2009/3/12 Sverre Rabbelier <srabbelier@gmail.com>:
> Heya,
>
> On Thu, Mar 12, 2009 at 14:18, John Tapsell <johnflux@gmail.com> wrote:
>> Is probably a mistake by the user.  We should warn the user and point
>> them in the right direction.
>
> The point is that we _already_ warned the user (like Dscho pointed
> out), and that(as you pointed out), it didn't work :P.

Just doing:

git branch -b origin/master origin/master

gives no error or warning at all.

John

>
> --
> Cheers,
>
> Sverre Rabbelier
>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: git checkout -b origin/mybranch origin/mybranch
  2009-03-12 13:43         ` Sverre Rabbelier
  2009-03-12 14:14           ` John Tapsell
@ 2009-03-12 15:21           ` Pieter de Bie
  2009-03-12 15:37             ` Jeff King
  1 sibling, 1 reply; 20+ messages in thread
From: Pieter de Bie @ 2009-03-12 15:21 UTC (permalink / raw)
  To: Sverre Rabbelier; +Cc: John Tapsell, Johannes Schindelin, Git List


On Mar 12, 2009, at 1:43 PM, Sverre Rabbelier wrote:

> The point is that we _already_ warned the user (like Dscho pointed
> out), and that(as you pointed out), it didn't work :P.

You can also get this in other interactions, for example:

	$ git checkout -b origin/test HEAD
	$ git checkout -b origin/test master

yes, these might be user errors, but I still think it's not OK to  
create a new ref 'refs/heads/origin/test' if there's also a 'refs/ 
remotes/origin/test' (as I've said a few months ago).

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: git checkout -b origin/mybranch origin/mybranch
  2009-03-12 15:21           ` Pieter de Bie
@ 2009-03-12 15:37             ` Jeff King
  2009-03-12 16:16               ` John Tapsell
  0 siblings, 1 reply; 20+ messages in thread
From: Jeff King @ 2009-03-12 15:37 UTC (permalink / raw)
  To: Pieter de Bie
  Cc: Sverre Rabbelier, John Tapsell, Johannes Schindelin, Git List

On Thu, Mar 12, 2009 at 03:21:48PM +0000, Pieter de Bie wrote:

> You can also get this in other interactions, for example:
>
> 	$ git checkout -b origin/test HEAD
> 	$ git checkout -b origin/test master
>
> yes, these might be user errors, but I still think it's not OK to create a 
> new ref 'refs/heads/origin/test' if there's also a 'refs/ 
> remotes/origin/test' (as I've said a few months ago).

One thing that has been missing from this discussion (and I think you
are getting to it here) is a concrete rule for "X is harmful, and Y is
not". That is, how do we know when to warn, and then what do we do?

John's original example was "git checkout -b origin/test origin/test".
So it's a problem that they're textually the same, but obviously there
are more problematic cases.

The behavior I think you are implying would be something like:

  When making origin/test, try to resolve_ref("origin/test"); if it
  fails, we are OK. If it succeeds, then we know we will be creating an
  ambiguity. Complain and refuse the creation unless "-f" is given.

This would actually be a superset of the "branch already exists" case,
so it should be pretty simple to code, and it makes for a simple rule:
it is now "ref already exists".

Does that actually cover all of the problematic cases?

-Peff

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: git checkout -b origin/mybranch origin/mybranch
  2009-03-12 15:37             ` Jeff King
@ 2009-03-12 16:16               ` John Tapsell
  2009-03-12 16:35                 ` Jeff King
  0 siblings, 1 reply; 20+ messages in thread
From: John Tapsell @ 2009-03-12 16:16 UTC (permalink / raw)
  To: Jeff King; +Cc: Pieter de Bie, Sverre Rabbelier, Johannes Schindelin, Git List

2009/3/12 Jeff King <peff@peff.net>:
> On Thu, Mar 12, 2009 at 03:21:48PM +0000, Pieter de Bie wrote:
>
>> You can also get this in other interactions, for example:
>>
>>       $ git checkout -b origin/test HEAD
>>       $ git checkout -b origin/test master
>>
>> yes, these might be user errors, but I still think it's not OK to create a
>> new ref 'refs/heads/origin/test' if there's also a 'refs/
>> remotes/origin/test' (as I've said a few months ago).
>
> One thing that has been missing from this discussion (and I think you
> are getting to it here) is a concrete rule for "X is harmful, and Y is
> not". That is, how do we know when to warn, and then what do we do?
>
> John's original example was "git checkout -b origin/test origin/test".
> So it's a problem that they're textually the same, but obviously there
> are more problematic cases.
>
> The behavior I think you are implying would be something like:
>
>  When making origin/test, try to resolve_ref("origin/test"); if it
>  fails, we are OK. If it succeeds, then we know we will be creating an
>  ambiguity. Complain and refuse the creation unless "-f" is given.
>
> This would actually be a superset of the "branch already exists" case,
> so it should be pretty simple to code, and it makes for a simple rule:
> it is now "ref already exists".

+1

I was thinking more along the lines of checking if it begins with
remotes/, origin/, tags/, stash/, bisect/    and blacklisting these.

Can anyone suggest a reason that you really might want to create a
branch called  origin/something ?


> Does that actually cover all of the problematic cases?
>
> -Peff
>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: git checkout -b origin/mybranch origin/mybranch
  2009-03-12 16:16               ` John Tapsell
@ 2009-03-12 16:35                 ` Jeff King
  2009-03-12 16:40                   ` Pieter de Bie
  2009-03-12 16:45                   ` John Tapsell
  0 siblings, 2 replies; 20+ messages in thread
From: Jeff King @ 2009-03-12 16:35 UTC (permalink / raw)
  To: John Tapsell
  Cc: Pieter de Bie, Sverre Rabbelier, Johannes Schindelin, Git List

On Thu, Mar 12, 2009 at 04:16:09PM +0000, John Tapsell wrote:

> I was thinking more along the lines of checking if it begins with
> remotes/, origin/, tags/, stash/, bisect/    and blacklisting these.
> 
> Can anyone suggest a reason that you really might want to create a
> branch called  origin/something ?

The name "origin" is simply convention. So if you are thinking about
blacklisting "origin/*", then it is certainly possible to have a false
positive (although as you note, it is unlikely). But what is worse is
that it is very likely for you to have a false negative if you use a
different remote name (and people often do if they have multiple
remotes).

For example, in one of my projects where I do integration, "origin" is
my own public repo, and I have a remote pointing to the public repo of a
number of other developers from whom I pull. So I would encounter the
same error by doing:

  git checkout -b mike/master mike/master

but it would not be caught by your rule.

One area where your rule _is_ nicer than mine is that mine only looks at
what exists _now_ and doesn't future-proof you at all. So I could say

  git checkout -b origin/newtopic

which might not be ambiguous. But if the remote adds a "newtopic"
branch, then the next time I fetch it will _become_ ambiguous.

Potentially you could blacklist "X/*" for every remote.X.* that
exists in your config. Even that, of course, isn't future-proof against
you creating a new remote. :)

I think the future-proofing is probably not worth the effort. Catching
things that are ambiguous _now_ will cover the "oops, I typed the wrong
thing" case, which I think is really the issue.

-Peff

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: git checkout -b origin/mybranch origin/mybranch
  2009-03-12 16:35                 ` Jeff King
@ 2009-03-12 16:40                   ` Pieter de Bie
  2009-03-12 16:51                     ` Jeff King
  2009-03-12 16:45                   ` John Tapsell
  1 sibling, 1 reply; 20+ messages in thread
From: Pieter de Bie @ 2009-03-12 16:40 UTC (permalink / raw)
  To: Jeff King; +Cc: John Tapsell, Sverre Rabbelier, Johannes Schindelin, Git List


On Mar 12, 2009, at 4:35 PM, Jeff King wrote:

> I think the future-proofing is probably not worth the effort. Catching
> things that are ambiguous _now_ will cover the "oops, I typed the  
> wrong
> thing" case, which I think is really the issue.

Exactly, that's the common case where things go wrong. I guess using  
dwim_ref
should be DWIM enough? :)

- Pieter

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: git checkout -b origin/mybranch origin/mybranch
  2009-03-12 16:35                 ` Jeff King
  2009-03-12 16:40                   ` Pieter de Bie
@ 2009-03-12 16:45                   ` John Tapsell
  1 sibling, 0 replies; 20+ messages in thread
From: John Tapsell @ 2009-03-12 16:45 UTC (permalink / raw)
  To: Jeff King; +Cc: Pieter de Bie, Sverre Rabbelier, Johannes Schindelin, Git List

2009/3/12 Jeff King <peff@peff.net>:
> On Thu, Mar 12, 2009 at 04:16:09PM +0000, John Tapsell wrote:
>
>> I was thinking more along the lines of checking if it begins with
>> remotes/, origin/, tags/, stash/, bisect/    and blacklisting these.
>>
>> Can anyone suggest a reason that you really might want to create a
>> branch called  origin/something ?
>
> The name "origin" is simply convention. So if you are thinking about
> blacklisting "origin/*", then it is certainly possible to have a false
> positive (although as you note, it is unlikely). But what is worse is
> that it is very likely for you to have a false negative if you use a
> different remote name (and people often do if they have multiple
> remotes).
>
> For example, in one of my projects where I do integration, "origin" is
> my own public repo, and I have a remote pointing to the public repo of a
> number of other developers from whom I pull. So I would encounter the
> same error by doing:
>
>  git checkout -b mike/master mike/master
>
> but it would not be caught by your rule.
>
> One area where your rule _is_ nicer than mine is that mine only looks at
> what exists _now_ and doesn't future-proof you at all. So I could say
>
>  git checkout -b origin/newtopic
>
> which might not be ambiguous. But if the remote adds a "newtopic"
> branch, then the next time I fetch it will _become_ ambiguous.
>
> Potentially you could blacklist "X/*" for every remote.X.* that
> exists in your config. Even that, of course, isn't future-proof against
> you creating a new remote. :)
>
> I think the future-proofing is probably not worth the effort. Catching
> things that are ambiguous _now_ will cover the "oops, I typed the wrong
> thing" case, which I think is really the issue.

Yep, makes sense.

I suppose you could do both.  Blacklist and check if it already
exists, but I think just checking if it exists is "good enough".

John

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: git checkout -b origin/mybranch origin/mybranch
  2009-03-12 16:40                   ` Pieter de Bie
@ 2009-03-12 16:51                     ` Jeff King
  2009-03-12 16:58                       ` John Tapsell
  0 siblings, 1 reply; 20+ messages in thread
From: Jeff King @ 2009-03-12 16:51 UTC (permalink / raw)
  To: Pieter de Bie
  Cc: John Tapsell, Sverre Rabbelier, Johannes Schindelin, Git List

On Thu, Mar 12, 2009 at 04:40:10PM +0000, Pieter de Bie wrote:

>> I think the future-proofing is probably not worth the effort.
>> Catching things that are ambiguous _now_ will cover the "oops, I
>> typed the wrong thing" case, which I think is really the issue.
>
> Exactly, that's the common case where things go wrong. I guess using
> dwim_ref should be DWIM enough? :)

Hmm. Yeah, I mispoke before: I should have said dwim_ref instead of
resolve_ref (which doesn't dwym :) ).

Here's a sloppy patch that I think does what you want; but it might make
more sense to just iterate over ref_rev_parse_rules ourselves, as
dwim_ref does more than we care about (and we should probably
differentiate between "a branch already exists" and "this would make an
ambiguous ref").

---
diff --git a/branch.c b/branch.c
index d20fb04..409f445 100644
--- a/branch.c
+++ b/branch.c
@@ -122,6 +122,7 @@ void create_branch(const char *head,
 	unsigned char sha1[20];
 	char *real_ref, msg[PATH_MAX + 20];
 	struct strbuf ref = STRBUF_INIT;
+	char *junk;
 	int forcing = 0;
 	int len;
 
@@ -135,7 +136,8 @@ void create_branch(const char *head,
 	if (check_ref_format(ref.buf))
 		die("'%s' is not a valid branch name.", name);
 
-	if (resolve_ref(ref.buf, sha1, 1, NULL)) {
+	if (dwim_ref(name, strlen(name), sha1, &junk)) {
+		free(junk);
 		if (!force)
 			die("A branch named '%s' already exists.", name);
 		else if (!is_bare_repository() && !strcmp(head, name))

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: git checkout -b origin/mybranch origin/mybranch
  2009-03-12 16:51                     ` Jeff King
@ 2009-03-12 16:58                       ` John Tapsell
  2009-03-12 17:14                         ` Jeff King
  0 siblings, 1 reply; 20+ messages in thread
From: John Tapsell @ 2009-03-12 16:58 UTC (permalink / raw)
  To: Jeff King; +Cc: Pieter de Bie, Sverre Rabbelier, Johannes Schindelin, Git List

2009/3/12 Jeff King <peff@peff.net>:
> On Thu, Mar 12, 2009 at 04:40:10PM +0000, Pieter de Bie wrote:
>
>>> I think the future-proofing is probably not worth the effort.
>>> Catching things that are ambiguous _now_ will cover the "oops, I
>>> typed the wrong thing" case, which I think is really the issue.
>>
>> Exactly, that's the common case where things go wrong. I guess using
>> dwim_ref should be DWIM enough? :)
>
> Hmm. Yeah, I mispoke before: I should have said dwim_ref instead of
> resolve_ref (which doesn't dwym :) ).
>
> Here's a sloppy patch that I think does what you want; but it might make
> more sense to just iterate over ref_rev_parse_rules ourselves, as
> dwim_ref does more than we care about (and we should probably
> differentiate between "a branch already exists" and "this would make an
> ambiguous ref").
>
> ---
> diff --git a/branch.c b/branch.c
> index d20fb04..409f445 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -122,6 +122,7 @@ void create_branch(const char *head,
>        unsigned char sha1[20];
>        char *real_ref, msg[PATH_MAX + 20];
>        struct strbuf ref = STRBUF_INIT;
> +       char *junk;
>        int forcing = 0;
>        int len;
>
> @@ -135,7 +136,8 @@ void create_branch(const char *head,
>        if (check_ref_format(ref.buf))
>                die("'%s' is not a valid branch name.", name);
>
> -       if (resolve_ref(ref.buf, sha1, 1, NULL)) {
> +       if (dwim_ref(name, strlen(name), sha1, &junk)) {
> +               free(junk);

Presumably 'junk' is the resolved name?  I wonder if it's worth
putting this info in the error message?

>                if (!force)
>                        die("A branch named '%s' already exists.", name);

>                        die("A branch named '%s' already exists (%s).", name, junk);

That would give  "A branched named 'origin/master' already exists
(refs/remotes/origin/master)"  right?

Dunno if it's worth it, just wondering.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: git checkout -b origin/mybranch origin/mybranch
  2009-03-12 16:58                       ` John Tapsell
@ 2009-03-12 17:14                         ` Jeff King
  2009-03-12 17:45                           ` John Tapsell
  0 siblings, 1 reply; 20+ messages in thread
From: Jeff King @ 2009-03-12 17:14 UTC (permalink / raw)
  To: John Tapsell
  Cc: Pieter de Bie, Sverre Rabbelier, Johannes Schindelin, Git List

On Thu, Mar 12, 2009 at 04:58:22PM +0000, John Tapsell wrote:

> > -       if (resolve_ref(ref.buf, sha1, 1, NULL)) {
> > +       if (dwim_ref(name, strlen(name), sha1, &junk)) {
> > +               free(junk);
> 
> Presumably 'junk' is the resolved name?  I wonder if it's worth
> putting this info in the error message?

Hey, I said it was sloppy, right? ;)

Here's your suggestion, plus specifying which situation (existing branch
or ambiguous ref) would occur. It would still need tests. But I'm
curious to hear more opinions on this direction before cleaning it up
much more (at the very least, it needs some tests).

--- a/branch.c
+++ b/branch.c
@@ -133,6 +133,7 @@ void create_branch(const char *head,
 	unsigned char sha1[20];
 	char *real_ref, msg[PATH_MAX + 20];
 	struct strbuf ref = STRBUF_INIT;
+	char *existing;
 	int forcing = 0;
 	int len;
 
@@ -146,12 +147,18 @@ void create_branch(const char *head,
 	if (check_ref_format(ref.buf))
 		die("'%s' is not a valid branch name.", name);
 
-	if (resolve_ref(ref.buf, sha1, 1, NULL)) {
-		if (!force)
-			die("A branch named '%s' already exists.", name);
+	if (dwim_ref(name, strlen(name), sha1, &existing)) {
+		if (!force) {
+			if (!prefixcmp(existing, "refs/heads/"))
+				die("A branch named '%s' already exists.",
+				    name);
+			die("Creating '%s' would be ambiguous with"
+			    " the existing %s", name, existing);
+		}
 		else if (!is_bare_repository() && !strcmp(head, name))
 			die("Cannot force update the current branch.");
 		forcing = 1;
+		free(existing);
 	}
 
 	real_ref = NULL;

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: git checkout -b origin/mybranch origin/mybranch
  2009-03-12 17:14                         ` Jeff King
@ 2009-03-12 17:45                           ` John Tapsell
  0 siblings, 0 replies; 20+ messages in thread
From: John Tapsell @ 2009-03-12 17:45 UTC (permalink / raw)
  To: Jeff King; +Cc: Pieter de Bie, Sverre Rabbelier, Johannes Schindelin, Git List

2009/3/12 Jeff King <peff@peff.net>:
> On Thu, Mar 12, 2009 at 04:58:22PM +0000, John Tapsell wrote:
>
>> > -       if (resolve_ref(ref.buf, sha1, 1, NULL)) {
>> > +       if (dwim_ref(name, strlen(name), sha1, &junk)) {
>> > +               free(junk);
>>
>> Presumably 'junk' is the resolved name?  I wonder if it's worth
>> putting this info in the error message?
>
> Hey, I said it was sloppy, right? ;)
>
> Here's your suggestion, plus specifying which situation (existing branch
> or ambiguous ref) would occur. It would still need tests. But I'm
> curious to hear more opinions on this direction before cleaning it up
> much more (at the very least, it needs some tests).

I like :-)  Just minor comments:

> --- a/branch.c
> +++ b/branch.c
> @@ -133,6 +133,7 @@ void create_branch(const char *head,
>        unsigned char sha1[20];
>        char *real_ref, msg[PATH_MAX + 20];
>        struct strbuf ref = STRBUF_INIT;
> +       char *existing;

Don't suppose you could set this NULL.  Just in case dwim_ref doesn't
set &existing for whatever reason.

>        int forcing = 0;
>        int len;
>
> @@ -146,12 +147,18 @@ void create_branch(const char *head,
>        if (check_ref_format(ref.buf))
>                die("'%s' is not a valid branch name.", name);
>
> -       if (resolve_ref(ref.buf, sha1, 1, NULL)) {
> -               if (!force)
> -                       die("A branch named '%s' already exists.", name);
> +       if (dwim_ref(name, strlen(name), sha1, &existing)) {
> +               if (!force) {
> +                       if (!prefixcmp(existing, "refs/heads/"))
> +                               die("A branch named '%s' already exists.",
> +                                   name);
> +                       die("Creating '%s' would be ambiguous with"
> +                           " the existing %s", name, existing);

Maybe put single quotes around the second %s, for consistency with the first?

> +               }
>                else if (!is_bare_repository() && !strcmp(head, name))
>                        die("Cannot force update the current branch.");
>                forcing = 1;
> +               free(existing);
>        }
>
>        real_ref = NULL;
>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: git checkout -b origin/mybranch origin/mybranch
  2009-03-12 13:18       ` John Tapsell
  2009-03-12 13:43         ` Sverre Rabbelier
@ 2009-03-12 18:31         ` Junio C Hamano
  1 sibling, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2009-03-12 18:31 UTC (permalink / raw)
  To: John Tapsell; +Cc: Johannes Schindelin, Git List

John Tapsell <johnflux@gmail.com> writes:

> I'm saying that:
>
> git checkout -b origin/mybranch  origin/mybranch
>
> Is probably a mistake by the user.  We should warn the user and point
> them in the right direction.

Given Finn Arne Gangstad's use case in a nearby thread, I do not think you
can so easily declare that it is a mistake by the user.

We have traditionally encouraged people to track remote origin/<name> with
matching local <name>, but in a workflow that interact with multiple
remotes, one obvious way to map would be to track remote origin/<name>
with local origin/<name> branch.  You can say remotes/origin/<name> when
you mean the remote tracking one, and say heads/origin/<name> when you
mean the local one, in order to disambiguate in such a setting.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: git checkout -b origin/mybranch origin/mybranch
  2009-03-12 14:14           ` John Tapsell
@ 2009-03-13 14:08             ` Michael J Gruber
  0 siblings, 0 replies; 20+ messages in thread
From: Michael J Gruber @ 2009-03-13 14:08 UTC (permalink / raw)
  To: John Tapsell; +Cc: Sverre Rabbelier, Johannes Schindelin, Git List

John Tapsell venit, vidit, dixit 12.03.2009 15:14:
> 2009/3/12 Sverre Rabbelier <srabbelier@gmail.com>:
>> Heya,
>>
>> On Thu, Mar 12, 2009 at 14:18, John Tapsell <johnflux@gmail.com> wrote:
>>> Is probably a mistake by the user.  We should warn the user and point
>>> them in the right direction.
>>
>> The point is that we _already_ warned the user (like Dscho pointed
>> out), and that(as you pointed out), it didn't work :P.
> 
> Just doing:
> 
> git branch -b origin/master origin/master
> 
> gives no error or warning at all.

...and it really should not. If you have a repo with lots of remotes and
tracking branches, it makes a lot of sense to have a local branch
reponame/branchname which tracks the remote branch reponame/branchname.
Note that the first is refs/heads/reponame/branchname whereas the latter
is refs/remotes/reponame/branchname. It gives warnings when it's
ambiguous, yay.

Michael J Gruber

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2009-03-13 14:10 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-03-12 11:36 git checkout -b origin/mybranch origin/mybranch John Tapsell
2009-03-12 11:40 ` Sverre Rabbelier
2009-03-12 11:43 ` Johannes Schindelin
2009-03-12 11:48   ` John Tapsell
2009-03-12 13:02     ` Johannes Schindelin
2009-03-12 13:18       ` John Tapsell
2009-03-12 13:43         ` Sverre Rabbelier
2009-03-12 14:14           ` John Tapsell
2009-03-13 14:08             ` Michael J Gruber
2009-03-12 15:21           ` Pieter de Bie
2009-03-12 15:37             ` Jeff King
2009-03-12 16:16               ` John Tapsell
2009-03-12 16:35                 ` Jeff King
2009-03-12 16:40                   ` Pieter de Bie
2009-03-12 16:51                     ` Jeff King
2009-03-12 16:58                       ` John Tapsell
2009-03-12 17:14                         ` Jeff King
2009-03-12 17:45                           ` John Tapsell
2009-03-12 16:45                   ` John Tapsell
2009-03-12 18:31         ` Junio C Hamano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).