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