{"thread":{"id":"28409","subject":"git branch --set-upstream regression in master","startedAt":"2011-09-16T21:43:41Z","lastAt":"2011-09-17T00:19:08Z","messageCount":5,"participants":["Jay Soffian","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"175672","messageId":"CAG+J_DyxNpPevwfrJVkv3GBmv0tEXgW2LZtdHgarFoXb9Qqghw@mail.gmail.com","threadId":"28409","inReplyTo":null,"subject":"git branch --set-upstream regression in master","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-09-16T21:43:41Z","receivedAt":"2011-09-16T21:43:41Z","isPatch":false,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"This used to be possible on the checked out branch:\n\n  $ git branch master --set-upstream origin/master\n\nNow it gives \"fatal: Cannot force update the current branch.\" which is\nbroken. You should be able to setup/change the tracking information on\nthe checked out branch.\n\nIt's apparently due to ci/forbid-unwanted-current-branch-update.\n\nSorry I don't have time to contribute a patch at the moment.\n\n(BTW, --set-upstream still needs to be fixed so that these mean the same thing:\n\n  $ git branch master --set-upstream origin/master\n  $ git branch --set-upstream origin/master master\n\nand to just allow:\n\n  $ git branch --set-upstream origin/master\n\nw/o having to specify the checked-out branch.)\n\nj.\n"},{"id":"175676","messageId":"7v7h58dri4.fsf@alter.siamese.dyndns.org","threadId":"28409","inReplyTo":"CAG+J_DyxNpPevwfrJVkv3GBmv0tEXgW2LZtdHgarFoXb9Qqghw@mail.gmail.com","subject":"Re: git branch --set-upstream regression in master","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-16T22:58:11Z","receivedAt":"2011-09-16T22:58:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jay Soffian <jaysoffian@gmail.com> writes:\n\n> This used to be possible on the checked out branch:\n>\n>   $ git branch master --set-upstream origin/master\n>\n> Now it gives \"fatal: Cannot force update the current branch.\" which is\n> broken. You should be able to setup/change the tracking information on\n> the checked out branch.\n>\n> It's apparently due to ci/forbid-unwanted-current-branch-update.\n\nDoes\n\n\tgit branch --set-upstream master origin/master\n\nwork? If so then I wouldn't worry too much about it (your \"arg then\noption\" should be forbidden in the longer term anyway). If not, we would\nneed to patch it.\n\n> (BTW, --set-upstream still needs to be fixed so that these mean the same\n> thing:\n>\n>   $ git branch master --set-upstream origin/master\n>   $ git branch --set-upstream origin/master master\n\nIf we are doing anythning, I think it needs to be fixed not to allow the\nformer, period.\n\n> .. to just allow:\n>\n>   $ git branch --set-upstream origin/master\n>\n> w/o having to specify the checked-out branch.\n\nThat may be a nice feature enhancement, post 1.7.7 release.\n\nI took a brief look at --set-upstream codepath, and I have to say that the\nimplementation is totally broken with respect to an existing branch.\n\nGiven\n\n\t$ git branch master --set-upstream origin/master\n\nit passes the exact same codepath as\n\n\t$ git branch master origin/master\n\nuses, only with a different \"track\" flag, no?  That is, it calls a\nfunction that is meant to _create_ branch \"master\" from given branch point\n\"origin/master\", namely create_branch().  And then create_branch(),\ncontrary to its name, is littered with \"dont_change_ref\" special case to\nwork it around, depending on the value of \"track\".\n"},{"id":"175677","messageId":"7v39fwdqqc.fsf@alter.siamese.dyndns.org","threadId":"28409","inReplyTo":"7v7h58dri4.fsf@alter.siamese.dyndns.org","subject":"Re: git branch --set-upstream regression in master","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-16T23:14:51Z","receivedAt":"2011-09-16T23:14:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I took a brief look at --set-upstream codepath, and I have to say that the\n> implementation is totally broken with respect to an existing branch.\n>\n> Given\n>\n> \t$ git branch master --set-upstream origin/master\n>\n> it passes the exact same codepath as\n>\n> \t$ git branch master origin/master\n>\n> uses, only with a different \"track\" flag, no?  That is, it calls a\n> function that is meant to _create_ branch \"master\" from given branch point\n> \"origin/master\", namely create_branch().  And then create_branch(),\n> contrary to its name, is littered with \"dont_change_ref\" special case to\n> work it around, depending on the value of \"track\".\n\nSo here is a quick-and-dirty patch, which may or may not compile or pass\ntests.\n\n branch.c           |   21 ++++++++++++---------\n branch.h           |   12 +++++++++++-\n builtin/branch.c   |    2 +-\n builtin/checkout.c |    3 ++-\n 4 files changed, 26 insertions(+), 12 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 478d825..fecedd3 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -135,23 +135,25 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,\n \treturn 0;\n }\n \n-int validate_new_branchname(const char *name, struct strbuf *ref, int force)\n+int validate_new_branchname(const char *name, struct strbuf *ref,\n+\t\t\t    int force, int attr_only)\n {\n-\tconst char *head;\n-\tunsigned char sha1[20];\n-\n \tif (strbuf_check_branch_ref(ref, name))\n \t\tdie(\"'%s' is not a valid branch name.\", name);\n \n \tif (!ref_exists(ref->buf))\n \t\treturn 0;\n-\telse if (!force)\n+\telse if (!force && !attr_only)\n \t\tdie(\"A branch named '%s' already exists.\", ref->buf + strlen(\"refs/heads/\"));\n \n-\thead = resolve_ref(\"HEAD\", sha1, 0, NULL);\n-\tif (!is_bare_repository() && head && !strcmp(head, ref->buf))\n-\t\tdie(\"Cannot force update the current branch.\");\n+\tif (!attr_only) {\n+\t\tconst char *head;\n+\t\tunsigned char sha1[20];\n \n+\t\thead = resolve_ref(\"HEAD\", sha1, 0, NULL);\n+\t\tif (!is_bare_repository() && head && !strcmp(head, ref->buf))\n+\t\t\tdie(\"Cannot force update the current branch.\");\n+\t}\n \treturn 1;\n }\n \n@@ -171,7 +173,8 @@ void create_branch(const char *head,\n \tif (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)\n \t\texplicit_tracking = 1;\n \n-\tif (validate_new_branchname(name, &ref, force || track == BRANCH_TRACK_OVERRIDE)) {\n+\tif (validate_new_branchname(name, &ref, force,\n+\t\t\t\t    track == BRANCH_TRACK_OVERRIDE)) {\n \t\tif (!force)\n \t\t\tdont_change_ref = 1;\n \t\telse\ndiff --git a/branch.h b/branch.h\nindex 01544e2..1285158 100644\n--- a/branch.h\n+++ b/branch.h\n@@ -20,8 +20,18 @@ void create_branch(const char *head, const char *name, const char *start_name,\n  * interpreted ref in ref, force indicates whether (non-head) branches\n  * may be overwritten. A non-zero return value indicates that the force\n  * parameter was non-zero and the branch already exists.\n+ *\n+ * Contrary to all of the above, when attr_only is 1, the caller is\n+ * not interested in verifying if it is Ok to update the named\n+ * branch to point at a potentially different commit. It is merely\n+ * asking if it is OK to change some attribute for the named branch\n+ * (e.g. tracking upstream).\n+ *\n+ * NEEDSWORK: This needs to be split into two separate functions in the\n+ * longer run for sanity.\n+ *\n  */\n-int validate_new_branchname(const char *name, struct strbuf *ref, int force);\n+int validate_new_branchname(const char *name, struct strbuf *ref, int force, int attr_only);\n \n /*\n  * Remove information about the state of working on the current\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex aa705a0..f49596f 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -566,7 +566,7 @@ static void rename_branch(const char *oldname, const char *newname, int force)\n \t\t\tdie(_(\"Invalid branch name: '%s'\"), oldname);\n \t}\n \n-\tvalidate_new_branchname(newname, &newref, force);\n+\tvalidate_new_branchname(newname, &newref, force, 0);\n \n \tstrbuf_addf(&logmsg, \"Branch: renamed %s to %s\",\n \t\t oldref.buf, newref.buf);\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 3bb6525..5e356a6 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1073,7 +1073,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \tif (opts.new_branch) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n-\t\topts.branch_exists = validate_new_branchname(opts.new_branch, &buf, !!opts.new_branch_force);\n+\t\topts.branch_exists = validate_new_branchname(opts.new_branch, &buf,\n+\t\t\t\t\t\t\t     !!opts.new_branch_force, 0);\n \n \t\tstrbuf_release(&buf);\n \t}\n"},{"id":"175678","messageId":"7vy5xocafg.fsf@alter.siamese.dyndns.org","threadId":"28409","inReplyTo":"CAG+J_DyxNpPevwfrJVkv3GBmv0tEXgW2LZtdHgarFoXb9Qqghw@mail.gmail.com","subject":"Re: git branch --set-upstream regression in master","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-16T23:52:19Z","receivedAt":"2011-09-16T23:52:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jay Soffian <jaysoffian@gmail.com> writes:\n\n> This used to be possible on the checked out branch:\n>\n>   $ git branch --set-upstream origin/master master\n>\n> Now it gives \"fatal: Cannot force update the current branch.\"\n> broken.\n\nLet's do this. I would be happy if I can include it in -rc2 with Tested-by:\nor something ;-)\n\n-- >8 --\nSubject: [PATCH] branch --set-upstream: regression fix\n\nThe \"git branch\" command, while not in listing mode, calls create_branch()\neven when the target branch already exists, and it does so even when it is\nnot interested in updating the value of the branch (i.e. the name of the\ncommit object that sits at the tip of the existing branch). This happens\nwhen the command is run with \"--set-upstream\" option.\n\nThe earlier safety-measure to prevent \"git branch -f $branch $commit\" from\nupdating the currently checked out branch did not take it into account,\nand we no longer can update the tracking information of the current branch.\n\nMinimally fix this regression by telling the validation code if it is\ncalled to really update the value of a potentially existing branch, or if\nthe caller merely is interested in updating auxiliary aspects of a branch.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n branch.c           |   21 ++++++++++++---------\n branch.h           |   12 +++++++++++-\n builtin/branch.c   |    2 +-\n builtin/checkout.c |    3 ++-\n t/t3200-branch.sh  |   13 +++++++++++++\n 5 files changed, 39 insertions(+), 12 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex 1fe3078..4338a90 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -135,23 +135,25 @@ static int setup_tracking(const char *new_ref, const char *orig_ref,\n \treturn 0;\n }\n \n-int validate_new_branchname(const char *name, struct strbuf *ref, int force)\n+int validate_new_branchname(const char *name, struct strbuf *ref,\n+\t\t\t    int force, int attr_only)\n {\n-\tconst char *head;\n-\tunsigned char sha1[20];\n-\n \tif (strbuf_check_branch_ref(ref, name))\n \t\tdie(\"'%s' is not a valid branch name.\", name);\n \n \tif (!ref_exists(ref->buf))\n \t\treturn 0;\n-\telse if (!force)\n+\telse if (!force && !attr_only)\n \t\tdie(\"A branch named '%s' already exists.\", ref->buf + strlen(\"refs/heads/\"));\n \n-\thead = resolve_ref(\"HEAD\", sha1, 0, NULL);\n-\tif (!is_bare_repository() && head && !strcmp(head, ref->buf))\n-\t\tdie(\"Cannot force update the current branch.\");\n+\tif (!attr_only) {\n+\t\tconst char *head;\n+\t\tunsigned char sha1[20];\n \n+\t\thead = resolve_ref(\"HEAD\", sha1, 0, NULL);\n+\t\tif (!is_bare_repository() && head && !strcmp(head, ref->buf))\n+\t\t\tdie(\"Cannot force update the current branch.\");\n+\t}\n \treturn 1;\n }\n \n@@ -171,7 +173,8 @@ void create_branch(const char *head,\n \tif (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)\n \t\texplicit_tracking = 1;\n \n-\tif (validate_new_branchname(name, &ref, force || track == BRANCH_TRACK_OVERRIDE)) {\n+\tif (validate_new_branchname(name, &ref, force,\n+\t\t\t\t    track == BRANCH_TRACK_OVERRIDE)) {\n \t\tif (!force)\n \t\t\tdont_change_ref = 1;\n \t\telse\ndiff --git a/branch.h b/branch.h\nindex 01544e2..1285158 100644\n--- a/branch.h\n+++ b/branch.h\n@@ -20,8 +20,18 @@ void create_branch(const char *head, const char *name, const char *start_name,\n  * interpreted ref in ref, force indicates whether (non-head) branches\n  * may be overwritten. A non-zero return value indicates that the force\n  * parameter was non-zero and the branch already exists.\n+ *\n+ * Contrary to all of the above, when attr_only is 1, the caller is\n+ * not interested in verifying if it is Ok to update the named\n+ * branch to point at a potentially different commit. It is merely\n+ * asking if it is OK to change some attribute for the named branch\n+ * (e.g. tracking upstream).\n+ *\n+ * NEEDSWORK: This needs to be split into two separate functions in the\n+ * longer run for sanity.\n+ *\n  */\n-int validate_new_branchname(const char *name, struct strbuf *ref, int force);\n+int validate_new_branchname(const char *name, struct strbuf *ref, int force, int attr_only);\n \n /*\n  * Remove information about the state of working on the current\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 40f885c..5fb3d85 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -566,7 +566,7 @@ static void rename_branch(const char *oldname, const char *newname, int force)\n \t\t\tdie(_(\"Invalid branch name: '%s'\"), oldname);\n \t}\n \n-\tvalidate_new_branchname(newname, &newref, force);\n+\tvalidate_new_branchname(newname, &newref, force, 0);\n \n \tstrbuf_addf(&logmsg, \"Branch: renamed %s to %s\",\n \t\t oldref.buf, newref.buf);\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex ddefec0..909a334 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1072,7 +1072,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \tif (opts.new_branch) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n-\t\topts.branch_exists = validate_new_branchname(opts.new_branch, &buf, !!opts.new_branch_force);\n+\t\topts.branch_exists = validate_new_branchname(opts.new_branch, &buf,\n+\t\t\t\t\t\t\t     !!opts.new_branch_force, 0);\n \n \t\tstrbuf_release(&buf);\n \t}\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex cb6458d..7633930 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -554,4 +554,17 @@ test_expect_success 'attempt to delete a branch merged to its base' '\n \ttest_must_fail git branch -d my10\n '\n \n+test_expect_success 'use set-upstream on the current branch' '\n+\tgit checkout master &&\n+\tgit --bare init myupstream.git &&\n+\tgit push myupstream.git master:refs/heads/frotz &&\n+\tgit remote add origin myupstream.git &&\n+\tgit fetch &&\n+\tgit branch --set-upstream master origin/frotz &&\n+\n+\ttest \"z$(git config branch.master.remote)\" = \"zorigin\" &&\n+\ttest \"z$(git config branch.master.merge)\" = \"zrefs/heads/frotz\"\n+\n+'\n+\n test_done\n-- \n1.7.7.rc1.4.g26e426\n"},{"id":"175679","messageId":"CAG+J_DwtkVws_kDbpb2HDW56BCdixa=DtnZ-uXei=URCkFq1=w@mail.gmail.com","threadId":"28409","inReplyTo":"7vy5xocafg.fsf@alter.siamese.dyndns.org","subject":"Re: git branch --set-upstream regression in master","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2011-09-17T00:19:08Z","receivedAt":"2011-09-17T00:19:08Z","isPatch":false,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Fri, Sep 16, 2011 at 7:52 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jay Soffian <jaysoffian@gmail.com> writes:\n>\n>> This used to be possible on the checked out branch:\n>>\n>>   $ git branch --set-upstream origin/master master\n>>\n>> Now it gives \"fatal: Cannot force update the current branch.\"\n>> broken.\n>\n> Let's do this. I would be happy if I can include it in -rc2 with Tested-by:\n> or something ;-)\n\nTested-by: Jay Soffian\n\nThanks for the quick turnaround. I'll try to find some time to make\n--set-upstream a proper option for post 1.7.7.\n\nj.\n"}]}