* 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).