{"thread":{"id":"31294","subject":"[PATCH 0/3] Improve branch UI for setting upstream information","startedAt":"2012-08-20T13:47:37Z","lastAt":"2012-08-27T21:33:46Z","messageCount":13,"participants":["Carlos Martín Nieto","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"197408","messageId":"1345470460-28734-1-git-send-email-cmn@elego.de","threadId":"31294","inReplyTo":null,"subject":"[PATCH 0/3] Improve branch UI for setting upstream information","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2012-08-20T13:47:37Z","receivedAt":"2012-08-20T13:47:37Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"Hi all,\n\nAfter way too long, here's the next iteration of the concept that\nbegan with swapping arguments in --set-upstream like -m does.\n\nAfter the feedback from the list, --set-upstream-to was born and\n--set-upstream is being deprecated in favour of either --track or\n--set-upstream-to depening on which of the behaviours the user wants\nto have.\n\nUsing --set-upsteam with one argument now also leads to a message\nexplaining how to undo it. For that, branch has learnt\n--unset-upstream which will remove the upstream information for the\ngiven branch (or HEAD).\n\nCarlos Martín Nieto (3):\n  branch: introduce --set-upstream-to\n  branch: add --unset-upstream option\n  branch: suggest how to undo a --set-upstream when given one branch\n\n Documentation/git-branch.txt | 14 +++++++-\n builtin/branch.c             | 65 +++++++++++++++++++++++++++++++++--\n t/t3200-branch.sh            | 82 ++++++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 157 insertions(+), 4 deletions(-)\n\n-- \n1.7.11.1.104.ge7b44f1\n"},{"id":"197409","messageId":"1345470460-28734-2-git-send-email-cmn@elego.de","threadId":"31294","inReplyTo":"1345470460-28734-1-git-send-email-cmn@elego.de","subject":"[PATCH 1/3] branch: introduce --set-upstream-to","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2012-08-20T13:47:38Z","receivedAt":"2012-08-20T13:47:38Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"The existing --set-uptream option can cause confusion, as it uses the\nusual branch convention of assuming a starting point of HEAD if none\nis specified, causing\n\n    git branch --set-upstream origin/master\n\nto create a new local branch 'origin/master' that tracks the current\nbranch. As --set-upstream already exists, we can't simply change its\nbehaviour. To work around this, introduce --set-upstream-to which\naccepts a compulsory argument indicating what the new upstream branch\nshould be and one optinal argument indicating which branch to change,\ndefaulting to HEAD.\n\nThe new options allows us to type\n\n    git branch --set-upstream-to origin/master\n\nto set the current branch's upstream to be origin's master.\n\nSigned-off-by: Carlos Martín Nieto <cmn@elego.de>\n---\n Documentation/git-branch.txt |  9 ++++++++-\n builtin/branch.c             | 17 +++++++++++++++--\n t/t3200-branch.sh            | 14 ++++++++++++++\n 3 files changed, 37 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex 47235be..e41c4b5 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -13,6 +13,7 @@ SYNOPSIS\n \t[--column[=<options>] | --no-column]\n \t[(--merged | --no-merged | --contains) [<commit>]] [<pattern>...]\n 'git branch' [--set-upstream | --track | --no-track] [-l] [-f] <branchname> [<start-point>]\n+'git branch' (--set-upstream-to=<upstream> | -u <upstream>) [<branchname>]\n 'git branch' (-m | -M) [<oldbranch>] <newbranch>\n 'git branch' (-d | -D) [-r] <branchname>...\n 'git branch' --edit-description [<branchname>]\n@@ -48,7 +49,7 @@ branch so that 'git pull' will appropriately merge from\n the remote-tracking branch. This behavior may be changed via the global\n `branch.autosetupmerge` configuration flag. That setting can be\n overridden by using the `--track` and `--no-track` options, and\n-changed later using `git branch --set-upstream`.\n+changed later using `git branch --set-upstream-to`.\n \n With a `-m` or `-M` option, <oldbranch> will be renamed to <newbranch>.\n If <oldbranch> had a corresponding reflog, it is renamed to match\n@@ -173,6 +174,12 @@ start-point is either a local or remote-tracking branch.\n \tlike `--track` would when creating the branch, except that where\n \tbranch points to is not changed.\n \n+-u <upstream>::\n+--set-upstream-to=<upstream>::\n+\tSet up <branchname>'s tracking information so <upstream> is\n+\tconsidered <branchname>'s upstream branch. If no <branchname>\n+\tis specified, then it defaults to the current branch.\n+\n --edit-description::\n \tOpen an editor and edit the text to explain what the branch is\n \tfor, to be used by various other commands (e.g. `request-pull`).\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 0e060f2..3c978eb 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -713,6 +713,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \tint verbose = 0, abbrev = -1, detached = 0;\n \tint reflog = 0, edit_description = 0;\n \tint quiet = 0;\n+\tconst char *new_upstream = NULL;\n \tenum branch_track track;\n \tint kinds = REF_LOCAL_BRANCH;\n \tstruct commit_list *with_commit = NULL;\n@@ -726,6 +727,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t\tBRANCH_TRACK_EXPLICIT),\n \t\tOPT_SET_INT( 0, \"set-upstream\",  &track, \"change upstream info\",\n \t\t\tBRANCH_TRACK_OVERRIDE),\n+\t\tOPT_STRING('u', \"set-upstream-to\", &new_upstream, \"upstream\", \"change the upstream info\"),\n \t\tOPT__COLOR(&branch_use_color, \"use colored output\"),\n \t\tOPT_SET_INT('r', \"remotes\",     &kinds, \"act on remote-tracking branches\",\n \t\t\tREF_REMOTE_BRANCH),\n@@ -794,10 +796,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options, builtin_branch_usage,\n \t\t\t     0);\n \n-\tif (!delete && !rename && !edit_description && argc == 0)\n+\tif (!delete && !rename && !edit_description && !new_upstream && argc == 0)\n \t\tlist = 1;\n \n-\tif (!!delete + !!rename + !!force_create + !!list > 1)\n+\tif (!!delete + !!rename + !!force_create + !!list + !!new_upstream > 1)\n \t\tusage_with_options(builtin_branch_usage, options);\n \n \tif (abbrev == -1)\n@@ -852,6 +854,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t\trename_branch(argv[0], argv[1], rename > 1);\n \t\telse\n \t\t\tusage_with_options(builtin_branch_usage, options);\n+\t} else if (new_upstream) {\n+\t\tstruct branch *branch = branch_get(argv[0]);\n+\n+\t\tif (!ref_exists(branch->refname))\n+\t\t\tdie(_(\"branch '%s' does not exist\"), branch->name);\n+\n+\t\t/*\n+\t\t * create_branch takes care of setting up the tracking\n+\t\t * info and making sure new_upstream is correct\n+\t\t */\n+\t\tcreate_branch(head, branch->name, new_upstream, 0, 0, 0, quiet, BRANCH_TRACK_OVERRIDE);\n \t} else if (argc > 0 && argc <= 2) {\n \t\tif (kinds != REF_LOCAL_BRANCH)\n \t\t\tdie(_(\"-a and -r options to 'git branch' do not make sense with a branch name\"));\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex a17f8b2..e9019ac 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -369,6 +369,20 @@ test_expect_success \\\n     'git tag foobar &&\n      test_must_fail git branch --track my11 foobar'\n \n+test_expect_success 'use --set-upstream-to modify HEAD' \\\n+    'test_config branch.master.remote foo &&\n+     test_config branch.master.merge foo &&\n+     git branch my12\n+     git branch --set-upstream-to my12 &&\n+     test \"$(git config branch.master.remote)\" = \".\" &&\n+     test \"$(git config branch.master.merge)\" = \"refs/heads/my12\"'\n+\n+test_expect_success 'use --set-upstream-to modify a particular branch' \\\n+    'git branch my13\n+     git branch --set-upstream-to master my13 &&\n+     test \"$(git config branch.my13.remote)\" = \".\" &&\n+     test \"$(git config branch.my13.merge)\" = \"refs/heads/master\"'\n+\n # Keep this test last, as it changes the current branch\n cat >expect <<EOF\n $_z40 $HEAD $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000\tbranch: Created from master\n-- \n1.7.11.1.104.ge7b44f1\n"},{"id":"197410","messageId":"1345470460-28734-3-git-send-email-cmn@elego.de","threadId":"31294","inReplyTo":"1345470460-28734-1-git-send-email-cmn@elego.de","subject":"[PATCH 2/3] branch: add --unset-upstream option","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2012-08-20T13:47:39Z","receivedAt":"2012-08-20T13:47:39Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"We have ways of setting the upstream information, but if we want to\nunset it, we need to resort to modifying the configuration manually.\n\nTeach branch an --unset-upstream option that unsets this information.\n\nSigned-off-by: Carlos Martín Nieto <cmn@elego.de>\n---\n Documentation/git-branch.txt |  5 +++++\n builtin/branch.c             | 17 ++++++++++++++---\n t/t3200-branch.sh            | 16 ++++++++++++++++\n 3 files changed, 35 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt\nindex e41c4b5..9c1d2f1 100644\n--- a/Documentation/git-branch.txt\n+++ b/Documentation/git-branch.txt\n@@ -14,6 +14,7 @@ SYNOPSIS\n \t[(--merged | --no-merged | --contains) [<commit>]] [<pattern>...]\n 'git branch' [--set-upstream | --track | --no-track] [-l] [-f] <branchname> [<start-point>]\n 'git branch' (--set-upstream-to=<upstream> | -u <upstream>) [<branchname>]\n+'git branch' --unset-upstream [<branchname>]\n 'git branch' (-m | -M) [<oldbranch>] <newbranch>\n 'git branch' (-d | -D) [-r] <branchname>...\n 'git branch' --edit-description [<branchname>]\n@@ -180,6 +181,10 @@ start-point is either a local or remote-tracking branch.\n \tconsidered <branchname>'s upstream branch. If no <branchname>\n \tis specified, then it defaults to the current branch.\n \n+--unset-upstream::\n+\tRemove the upstream information for <branchname>. If no branch\n+\tis specified it defaults to the current branch.\n+\n --edit-description::\n \tOpen an editor and edit the text to explain what the branch is\n \tfor, to be used by various other commands (e.g. `request-pull`).\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 3c978eb..08068f7 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -712,7 +712,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \tint delete = 0, rename = 0, force_create = 0, list = 0;\n \tint verbose = 0, abbrev = -1, detached = 0;\n \tint reflog = 0, edit_description = 0;\n-\tint quiet = 0;\n+\tint quiet = 0, unset_upstream = 0;\n \tconst char *new_upstream = NULL;\n \tenum branch_track track;\n \tint kinds = REF_LOCAL_BRANCH;\n@@ -728,6 +728,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tOPT_SET_INT( 0, \"set-upstream\",  &track, \"change upstream info\",\n \t\t\tBRANCH_TRACK_OVERRIDE),\n \t\tOPT_STRING('u', \"set-upstream-to\", &new_upstream, \"upstream\", \"change the upstream info\"),\n+\t\tOPT_BOOLEAN(0, \"unset-upstream\", &unset_upstream, \"Unset the upstream info\"),\n \t\tOPT__COLOR(&branch_use_color, \"use colored output\"),\n \t\tOPT_SET_INT('r', \"remotes\",     &kinds, \"act on remote-tracking branches\",\n \t\t\tREF_REMOTE_BRANCH),\n@@ -796,10 +797,10 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, options, builtin_branch_usage,\n \t\t\t     0);\n \n-\tif (!delete && !rename && !edit_description && !new_upstream && argc == 0)\n+\tif (!delete && !rename && !edit_description && !new_upstream && !unset_upstream && argc == 0)\n \t\tlist = 1;\n \n-\tif (!!delete + !!rename + !!force_create + !!list + !!new_upstream > 1)\n+\tif (!!delete + !!rename + !!force_create + !!list + !!new_upstream + !!unset_upstream > 1)\n \t\tusage_with_options(builtin_branch_usage, options);\n \n \tif (abbrev == -1)\n@@ -865,6 +866,16 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t * info and making sure new_upstream is correct\n \t\t */\n \t\tcreate_branch(head, branch->name, new_upstream, 0, 0, 0, quiet, BRANCH_TRACK_OVERRIDE);\n+\t} else if (unset_upstream) {\n+\t\tstruct branch *branch = branch_get(argv[0]);\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\n+\t\tstrbuf_addf(&buf, \"branch.%s.remote\", branch->name);\n+\t\tgit_config_set_multivar(buf.buf, NULL, NULL, 1);\n+\t\tstrbuf_reset(&buf);\n+\t\tstrbuf_addf(&buf, \"branch.%s.merge\", branch->name);\n+\t\tgit_config_set_multivar(buf.buf, NULL, NULL, 1);\n+\t\tstrbuf_release(&buf);\n \t} else if (argc > 0 && argc <= 2) {\n \t\tif (kinds != REF_LOCAL_BRANCH)\n \t\t\tdie(_(\"-a and -r options to 'git branch' do not make sense with a branch name\"));\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex e9019ac..93e5d6e 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -383,6 +383,22 @@ test_expect_success 'use --set-upstream-to modify a particular branch' \\\n      test \"$(git config branch.my13.remote)\" = \".\" &&\n      test \"$(git config branch.my13.merge)\" = \"refs/heads/master\"'\n \n+test_expect_success 'test --unset-upstream on HEAD' \\\n+    'git branch my14\n+     test_config branch.master.remote foo &&\n+     test_config branch.master.merge foo &&\n+     git branch --set-upstream-to my14 &&\n+     git branch --unset-upstream &&\n+     test_must_fail git config branch.master.remote &&\n+     test_must_fail git config branch.master.merge'\n+\n+test_expect_success 'test --unset-upstream on a particular branch' \\\n+    'git branch my15\n+     git branch --set-upstream-to master my14 &&\n+     git branch --unset-upstream my14 &&\n+     test_must_fail git config branch.my14.remote &&\n+     test_must_fail git config branch.my14.merge'\n+\n # Keep this test last, as it changes the current branch\n cat >expect <<EOF\n $_z40 $HEAD $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000\tbranch: Created from master\n-- \n1.7.11.1.104.ge7b44f1\n"},{"id":"197411","messageId":"1345470460-28734-4-git-send-email-cmn@elego.de","threadId":"31294","inReplyTo":"1345470460-28734-1-git-send-email-cmn@elego.de","subject":"[PATCH 3/3] branch: suggest how to undo a --set-upstream when given one branch","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2012-08-20T13:47:40Z","receivedAt":"2012-08-20T13:47:40Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"This interface is error prone, and a better one (--set-upstream-to)\nexists. Suggest how to fix a --set-upstream invocation in case the\nuser only gives one argument, which makes it likely that he meant to\ndo the opposite, like with\n\n    git branch --set-upstream origin/master\n\nwhen they meant one of\n\n    git branch --set-upstream origin/master master\n    git branch --set-upstream-to origin/master\n\nWhile we're at it, add a notice that the --set-upstream flag is\ndeprecated and will be removed at some point.\n\nSigned-off-by: Carlos Martín Nieto <cmn@elego.de>\n\n---\n\nThis produces suboptimal output in case that A tracks B and we do\n\n    git checkout B\n    git branch --set-upstream A\n\nas it will suggest\n\n    git branch --set-upstream A B\n\nas a way of undoing what we just did. Avoiding it becomes a bit messy\n(yet another layer of ifs), so I've left it out. Anybody reckon it's\nworth recognising this?\n---\n builtin/branch.c  | 35 +++++++++++++++++++++++++++++++++++\n t/t3200-branch.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 87 insertions(+)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 08068f7..33641d9 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -707,6 +707,21 @@ static int edit_branch_description(const char *branch_name)\n \treturn status;\n }\n \n+static void print_set_upstream_warning(const char *branch, int branch_existed, const char *old_upstream)\n+{\n+\tfprintf(stderr, _(\"If you wanted to make '%s' track '%s', do this:\\n\\n\"), head, branch);\n+\tif (branch_existed) {\n+\t\tif (old_upstream)\n+\t\t\tfprintf(stderr, _(\"    git branch --set-upstream %s %s\\n\"), old_upstream, branch);\n+\t\telse\n+\t\t\tfprintf(stderr, _(\"    git branch --unset-upstream %s\\n\"), branch);\n+\t} else {\n+\t\tfprintf(stderr, _(\"    git branch -d %s\\n\"), branch);\n+\t}\n+\n+\tfprintf(stderr, _(\"    git branch --set-upstream-to %s\\n\"), branch);\n+}\n+\n int cmd_branch(int argc, const char **argv, const char *prefix)\n {\n \tint delete = 0, rename = 0, force_create = 0, list = 0;\n@@ -877,10 +892,30 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tgit_config_set_multivar(buf.buf, NULL, NULL, 1);\n \t\tstrbuf_release(&buf);\n \t} else if (argc > 0 && argc <= 2) {\n+\t\tstruct branch *branch = branch_get(argv[0]);\n+\t\tconst char *old_upstream = NULL;\n+\t\tint branch_existed = 0;\n+\n \t\tif (kinds != REF_LOCAL_BRANCH)\n \t\t\tdie(_(\"-a and -r options to 'git branch' do not make sense with a branch name\"));\n+\n+\t\tif (track == BRANCH_TRACK_OVERRIDE)\n+\t\t\tfprintf(stderr, _(\"The --set-upstream flag is deprecated and will be removed. Consider using --track or --set-upstream-to\\n\"));\n+\n+\t\t/*\n+\t\t * Save what argv[0] was pointing to so we can give\n+\t\t * the --set-upstream-to hint\n+\t\t */\n+\t\tif (branch_has_merge_config(branch))\n+\t\t\told_upstream = shorten_unambiguous_ref(branch->merge[0]->dst, 0);\n+\n+\t\tbranch_existed = ref_exists(branch->refname);\n \t\tcreate_branch(head, argv[0], (argc == 2) ? argv[1] : head,\n \t\t\t      force_create, reflog, 0, quiet, track);\n+\n+\t\tif (argc == 1 && track == BRANCH_TRACK_OVERRIDE)\n+\t\t\tprint_set_upstream_warning(argv[0], branch_existed, old_upstream);\n+\n \t} else\n \t\tusage_with_options(builtin_branch_usage, options);\n \ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 93e5d6e..702bffa 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -399,6 +399,58 @@ test_expect_success 'test --unset-upstream on a particular branch' \\\n      test_must_fail git config branch.my14.remote &&\n      test_must_fail git config branch.my14.merge'\n \n+test_expect_success 'test --set-upstream help message with one arg' \\\n+    'git branch --set-upstream origin/master 2>actual &&\n+     test_when_finished git branch -d origin/master &&\n+     cat >expected <<EOF &&\n+The --set-upstream flag is deprecated and will be removed. Consider using --track or --set-upstream-to\n+If you wanted to make '\"'master'\"' track '\"'origin/master'\"', do this:\n+\n+    git branch -d origin/master\n+    git branch --set-upstream-to origin/master\n+EOF\n+     test_cmp expected actual\n+'\n+\n+test_expect_success '--set-upstream with a branch that already has an upstream' \\\n+    'git branch --set-upstream-to my12 master &&\n+     git branch --set-upstream-to my13 my12 &&\n+     test_when_finished git branch --unset-upstream my12 &&\n+     test_when_finished git branch --unset-upstream my13 &&\n+     git branch --set-upstream my12 2>actual &&\n+     cat actual &&\n+     cat >expected <<EOF &&\n+The --set-upstream flag is deprecated and will be removed. Consider using --track or --set-upstream-to\n+If you wanted to make '\"'master'\"' track '\"'my12'\"', do this:\n+\n+    git branch --set-upstream my13 my12\n+    git branch --set-upstream-to my12\n+EOF\n+     test_cmp expected actual\n+'\n+\n+test_expect_success '--set-upstream with a branch that has no upstream' \\\n+    'git branch --set-upstream my12 2>actual &&\n+     test_when_finished git branch --unset-upstream my12\n+     cat >expected <<EOF &&\n+The --set-upstream flag is deprecated and will be removed. Consider using --track or --set-upstream-to\n+If you wanted to make '\"'master'\"' track '\"'my12'\"', do this:\n+\n+    git branch --unset-upstream my12\n+    git branch --set-upstream-to my12\n+EOF\n+     test_cmp expected actual\n+'\n+\n+test_expect_success '--set-upstream with two args should only show the deprecation message' \\\n+    'git branch --set-upstream master my13 2>actual &&\n+     test_when_finished git branch --unset-upstream master &&\n+     cat >expected <<EOF &&\n+The --set-upstream flag is deprecated and will be removed. Consider using --track or --set-upstream-to\n+EOF\n+     test_cmp expected actual\n+'\n+\n # Keep this test last, as it changes the current branch\n cat >expect <<EOF\n $_z40 $HEAD $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000\tbranch: Created from master\n-- \n1.7.11.1.104.ge7b44f1\n"},{"id":"197432","messageId":"7vk3wtjt3o.fsf@alter.siamese.dyndns.org","threadId":"31294","inReplyTo":"1345470460-28734-4-git-send-email-cmn@elego.de","subject":"Re: [PATCH 3/3] branch: suggest how to undo a --set-upstream when given one branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-20T18:50:19Z","receivedAt":"2012-08-20T18:50:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto <cmn@elego.de> writes:\n\n> This interface is error prone, and a better one (--set-upstream-to)\n> exists. Suggest how to fix a --set-upstream invocation in case the\n> user only gives one argument, which makes it likely that he meant to\n> do the opposite, like with\n>\n>     git branch --set-upstream origin/master\n>\n> when they meant one of\n>\n>     git branch --set-upstream origin/master master\n>     git branch --set-upstream-to origin/master\n\nIn \"git branch --set-upstream $A\", if $A did not exist (i.e. we\nended up creating $A that is forked from the current branch), and if\nrefs/remotes/$A exists, I agree it is very likely that the user\nwanted to set the upstream of the current branch to remotes/$A\ninstead.\n\nBut I am not sure about other cases.  If $A already existed, it is\nequally likely that \"git branch --set-upstream $A\" wanted to make\nthe existing $A track our current branch, or make our branch track\nthat existing $A, isn't it?  We would end up giving unwanted advice\nthat is *wrong* for the user's situation 50% of the time, which does\nnot sound like an acceptable thing to do.\n\nSo I would really prefer to see the condition this advice is offered\nlimited to very specific cases (namely, only one parameter $A was\ngiven to cause us use \"HEAD\" as the other branch, refs/heads/$A did\nnot exist and refs/remotes/$A did; there may be others but I think\nthis is the one we most care about) in which we know the advice is\ncorrect with certainty.\n\n> While we're at it, add a notice that the --set-upstream flag is\n> deprecated and will be removed at some point.\n\nThis part is unquestionably good.\n\n> Signed-off-by: Carlos Martín Nieto <cmn@elego.de>\n>\n> ---\n>\n> This produces suboptimal output in case that A tracks B and we do\n>\n>     git checkout B\n>     git branch --set-upstream A\n>\n> as it will suggest\n>\n>     git branch --set-upstream A B\n>\n> as a way of undoing what we just did.\n\nThe literal meaning of what the user did was to create B and then\nmade A that existed (and used to build upon B) to build upon B,\nwhich is a no-op.  And undoing can be done with the same command.\n\nWhich is funny.\n\nI am sure you will get complaint from the real users.  To avoid it,\nyou would need to know what the old value was (if any), and skip the\n\"undo\" part, but I think it is probably worth it.  You already have\ncode to learn what the old value was anyway, so the ideal is just a\nsingle comparison away, no?\n\nBy the way, are you assuming that what the user wanted to do was to\nmake B build on top of A (i.e. set branch.B.up to A) in this\nexample?  For that setting to make sense, perhaps B should already\nbe a descendant of A, shouldn't it?  If that is the case, that is\nanother clue you can use in your logic to decide if you want to tell\nthe user \"you told me to make $A build on the current branch, but I\nthink you meant the other way around\" with more confidence.\n\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index 08068f7..33641d9 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -707,6 +707,21 @@ static int edit_branch_description(const char *branch_name)\n>  \treturn status;\n>  }\n>  \n> +static void print_set_upstream_warning(const char *branch, int branch_existed, const char *old_upstream)\n> +{\n> +\tfprintf(stderr, _(\"If you wanted to make '%s' track '%s', do this:\\n\\n\"), head, branch);\n\nI would suggest strongly against using the verb \"track\" here, as it\nis overused and has caused confusion \"is it tracking branch, remote\ntracking branch, do I merge in one but fetch to update the other?\".\n\nRegardless of the verb, I think there should be a line _before_ the\nabove to tell the user what the command actually _did_, to make the\ndistinction stand out to help the user decide.\n\nThat is, tell the user \"I set upstream for that other branch to the\ncurrent branch, making that other branch build on top of the current\nbranch.  If that was a mistake, and what you really wanted to do was\nto make the current branch build on top of the other branch, here\nare the things you need to do to recover\".  Without saying anything\nbefore \"If that was a mistake, \" part and giving \"to do X, do Y\"\nwould leave the user confused why he is being given a way to do X in\nthe first place, whether what he really wanted to do was X or\nsomething else.\n\n> +\tif (branch_existed) {\n> +\t\tif (old_upstream)\n> +\t\t\tfprintf(stderr, _(\"    git branch --set-upstream %s %s\\n\"), old_upstream, branch);\n> +\t\telse\n> +\t\t\tfprintf(stderr, _(\"    git branch --unset-upstream %s\\n\"), branch);\n> +\t} else {\n> +\t\tfprintf(stderr, _(\"    git branch -d %s\\n\"), branch);\n> +\t}\n> +\n> +\tfprintf(stderr, _(\"    git branch --set-upstream-to %s\\n\"), branch);\n> +}\n\nAnd I suspect the logic to call into this function needs to be\ntightened a lot.  It may even turn out that we may not need messages\nwhen \"branch_existed\" is true.\n\n>  int cmd_branch(int argc, const char **argv, const char *prefix)\n>  {\n>  \tint delete = 0, rename = 0, force_create = 0, list = 0;\n> @@ -877,10 +892,30 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n>  \t\tgit_config_set_multivar(buf.buf, NULL, NULL, 1);\n>  \t\tstrbuf_release(&buf);\n>  \t} else if (argc > 0 && argc <= 2) {\n> +\t\tstruct branch *branch = branch_get(argv[0]);\n> +\t\tconst char *old_upstream = NULL;\n> +\t\tint branch_existed = 0;\n> +\n>  \t\tif (kinds != REF_LOCAL_BRANCH)\n>  \t\t\tdie(_(\"-a and -r options to 'git branch' do not make sense with a branch name\"));\n> +\n> +\t\tif (track == BRANCH_TRACK_OVERRIDE)\n> +\t\t\tfprintf(stderr, _(\"The --set-upstream flag is deprecated and will be removed. Consider using --track or --set-upstream-to\\n\"));\n> +\n> +\t\t/*\n> +\t\t * Save what argv[0] was pointing to so we can give\n> +\t\t * the --set-upstream-to hint\n> +\t\t */\n> +\t\tif (branch_has_merge_config(branch))\n> +\t\t\told_upstream = shorten_unambiguous_ref(branch->merge[0]->dst, 0);\n> +\n> +\t\tbranch_existed = ref_exists(branch->refname);\n>  \t\tcreate_branch(head, argv[0], (argc == 2) ? argv[1] : head,\n>  \t\t\t      force_create, reflog, 0, quiet, track);\n> +\n> +\t\tif (argc == 1 && track == BRANCH_TRACK_OVERRIDE)\n> +\t\t\tprint_set_upstream_warning(argv[0], branch_existed, old_upstream);\n> +\n"},{"id":"197569","messageId":"1345598806.14938.30.camel@centaur.cmartin.tk","threadId":"31294","inReplyTo":"7vk3wtjt3o.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] branch: suggest how to undo a --set-upstream when given one branch","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2012-08-22T01:26:46Z","receivedAt":"2012-08-22T01:26:46Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"On Mon, 2012-08-20 at 11:50 -0700, Junio C Hamano wrote:\n> Carlos Martín Nieto <cmn@elego.de> writes:\n> \n> > This interface is error prone, and a better one (--set-upstream-to)\n> > exists. Suggest how to fix a --set-upstream invocation in case the\n> > user only gives one argument, which makes it likely that he meant to\n> > do the opposite, like with\n> >\n> >     git branch --set-upstream origin/master\n> >\n> > when they meant one of\n> >\n> >     git branch --set-upstream origin/master master\n> >     git branch --set-upstream-to origin/master\n> \n> In \"git branch --set-upstream $A\", if $A did not exist (i.e. we\n> ended up creating $A that is forked from the current branch), and if\n> refs/remotes/$A exists, I agree it is very likely that the user\n> wanted to set the upstream of the current branch to remotes/$A\n> instead.\n> \n> But I am not sure about other cases.  If $A already existed, it is\n> equally likely that \"git branch --set-upstream $A\" wanted to make\n> the existing $A track our current branch, or make our branch track\n> that existing $A, isn't it?  We would end up giving unwanted advice\n> that is *wrong* for the user's situation 50% of the time, which does\n> not sound like an acceptable thing to do.\n> \n> So I would really prefer to see the condition this advice is offered\n> limited to very specific cases (namely, only one parameter $A was\n> given to cause us use \"HEAD\" as the other branch, refs/heads/$A did\n> not exist and refs/remotes/$A did; there may be others but I think\n> this is the one we most care about) in which we know the advice is\n> correct with certainty.\n\nI'm not sure about the 50%, I've personally never used the one-argument\nversion (correctly), but the restriction does make sense as\n--set-upstream origin/master is the main thing we want to \"protect\"\nagainst.\n\n> \n> > While we're at it, add a notice that the --set-upstream flag is\n> > deprecated and will be removed at some point.\n> \n> This part is unquestionably good.\n> \n> > Signed-off-by: Carlos Martín Nieto <cmn@elego.de>\n> >\n> > ---\n> >\n> > This produces suboptimal output in case that A tracks B and we do\n> >\n> >     git checkout B\n> >     git branch --set-upstream A\n> >\n> > as it will suggest\n> >\n> >     git branch --set-upstream A B\n> >\n> > as a way of undoing what we just did.\n> \n> The literal meaning of what the user did was to create B and then\n> made A that existed (and used to build upon B) to build upon B,\n> which is a no-op.  And undoing can be done with the same command.\n> \n> Which is funny.\n> \n> I am sure you will get complaint from the real users.  To avoid it,\n> you would need to know what the old value was (if any), and skip the\n> \"undo\" part, but I think it is probably worth it.  You already have\n> code to learn what the old value was anyway, so the ideal is just a\n> single comparison away, no?\n\nYes, but this comparison is already two or three levels deep in ifs,\nwhich is why I didn't put it in this version, as it was unlikely to be\ntriggered anyway. As per the next paragraph, this probably won't be an\nissue.\n\n> \n> By the way, are you assuming that what the user wanted to do was to\n> make B build on top of A (i.e. set branch.B.up to A) in this\n> example?  For that setting to make sense, perhaps B should already\n> be a descendant of A, shouldn't it?  If that is the case, that is\n> another clue you can use in your logic to decide if you want to tell\n> the user \"you told me to make $A build on the current branch, but I\n> think you meant the other way around\" with more confidence.\n\nI am pretty much assuming that every use of --set-upstream with a single\nargument was meant to use the --set-upstream-to semantics. This\nparticular examples is an edge-case I found when trying to figure out\nthe possible combinations, not a sane one I expect to see in the wild.\n\n> \n> > diff --git a/builtin/branch.c b/builtin/branch.c\n> > index 08068f7..33641d9 100644\n> > --- a/builtin/branch.c\n> > +++ b/builtin/branch.c\n> > @@ -707,6 +707,21 @@ static int edit_branch_description(const char *branch_name)\n> >  \treturn status;\n> >  }\n> >  \n> > +static void print_set_upstream_warning(const char *branch, int branch_existed, const char *old_upstream)\n> > +{\n> > +\tfprintf(stderr, _(\"If you wanted to make '%s' track '%s', do this:\\n\\n\"), head, branch);\n> \n> I would suggest strongly against using the verb \"track\" here, as it\n> is overused and has caused confusion \"is it tracking branch, remote\n> tracking branch, do I merge in one but fetch to update the other?\".\n> \n> Regardless of the verb, I think there should be a line _before_ the\n> above to tell the user what the command actually _did_, to make the\n> distinction stand out to help the user decide.\n\nI don't like the word either, but that's what 'branch --set-upstream'\nand 'checkout -t' use (or anything that ends up calling\ninstall_branch_config()). There is such a message (see below) and that's\nhow it says it. I worry that changing the wording might be even more\nconfusing, but I'll play with some wording (unless you want to change\nthe message in install_branch_config()).\n\n> \n> That is, tell the user \"I set upstream for that other branch to the\n> current branch, making that other branch build on top of the current\n> branch.  If that was a mistake, and what you really wanted to do was\n> to make the current branch build on top of the other branch, here\n> are the things you need to do to recover\".  Without saying anything\n> before \"If that was a mistake, \" part and giving \"to do X, do Y\"\n> would leave the user confused why he is being given a way to do X in\n> the first place, whether what he really wanted to do was X or\n> something else.\n\nThis message is always printed as a result of create_branch() at the\nbottom of cmd_branch() and it's printed to stdout. The rest is printed\nto stderr and that's what the tests are interested in checking, so you\ncan't really tell from there. The whole output looks like\n        \n    % ./bin-wrappers/git branch --set-upstream master\n    The --set-upstream flag is deprecated and will be removed. Consider using --track or --set-upstream-to\n    Branch master set up to track local branch set-upstream-to.\n    If you wanted to make 'set-upstream-to' track 'master', do this:\n    \n        git branch --set-upstream master set-upstream-to\n        git branch --set-upstream-to master\n\nIt may need an extra LF after the deprecation warning for readability.\n\n> \n> > +\tif (branch_existed) {\n> > +\t\tif (old_upstream)\n> > +\t\t\tfprintf(stderr, _(\"    git branch --set-upstream %s %s\\n\"), old_upstream, branch);\n> > +\t\telse\n> > +\t\t\tfprintf(stderr, _(\"    git branch --unset-upstream %s\\n\"), branch);\n> > +\t} else {\n> > +\t\tfprintf(stderr, _(\"    git branch -d %s\\n\"), branch);\n> > +\t}\n> > +\n> > +\tfprintf(stderr, _(\"    git branch --set-upstream-to %s\\n\"), branch);\n> > +}\n> \n> And I suspect the logic to call into this function needs to be\n> tightened a lot.  It may even turn out that we may not need messages\n> when \"branch_existed\" is true.\n\nWithout that check, this function will be much simpler, yeah. I'll go\nfor showing the help only for a new branch in the next iteration and\nthen we can see if we want to check how existing branches relate.\n\n> \n> >  int cmd_branch(int argc, const char **argv, const char *prefix)\n> >  {\n> >  \tint delete = 0, rename = 0, force_create = 0, list = 0;\n> > @@ -877,10 +892,30 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> >  \t\tgit_config_set_multivar(buf.buf, NULL, NULL, 1);\n> >  \t\tstrbuf_release(&buf);\n> >  \t} else if (argc > 0 && argc <= 2) {\n> > +\t\tstruct branch *branch = branch_get(argv[0]);\n> > +\t\tconst char *old_upstream = NULL;\n> > +\t\tint branch_existed = 0;\n> > +\n> >  \t\tif (kinds != REF_LOCAL_BRANCH)\n> >  \t\t\tdie(_(\"-a and -r options to 'git branch' do not make sense with a branch name\"));\n> > +\n> > +\t\tif (track == BRANCH_TRACK_OVERRIDE)\n> > +\t\t\tfprintf(stderr, _(\"The --set-upstream flag is deprecated and will be removed. Consider using --track or --set-upstream-to\\n\"));\n> > +\n> > +\t\t/*\n> > +\t\t * Save what argv[0] was pointing to so we can give\n> > +\t\t * the --set-upstream-to hint\n> > +\t\t */\n> > +\t\tif (branch_has_merge_config(branch))\n> > +\t\t\told_upstream = shorten_unambiguous_ref(branch->merge[0]->dst, 0);\n> > +\n> > +\t\tbranch_existed = ref_exists(branch->refname);\n> >  \t\tcreate_branch(head, argv[0], (argc == 2) ? argv[1] : head,\n> >  \t\t\t      force_create, reflog, 0, quiet, track);\n> > +\n> > +\t\tif (argc == 1 && track == BRANCH_TRACK_OVERRIDE)\n> > +\t\t\tprint_set_upstream_warning(argv[0], branch_existed, old_upstream);\n> > +\n> \n"},{"id":"197729","messageId":"1345748186-1081-1-git-send-email-cmn@elego.de","threadId":"31294","inReplyTo":"7vk3wtjt3o.fsf@alter.siamese.dyndns.org","subject":"[PATCHv2 3/3] branch: deprecate --set-upstream and show help if we detect possible mistaken use","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2012-08-23T18:56:26Z","receivedAt":"2012-08-23T18:56:26Z","isPatch":false,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"This interface is error prone, and a better one (--set-upstream-to)\nexists. Add a message listing the alternatives and suggest how to fix\na --set-upstream invocation in case the user only gives one argument\nwhich causes a local branch with the same name as a remote-tracking\none to be created. The typical case is\n\n    git branch --set-upstream origin/master\n\nwhen the user meant\n\n    git branch --set-upstream master origin/master\n\nassuming that the current branch is master. Show a message telling the\nuser how to undo their action and get what they wanted. For the\ncommand above, the message would be\n\nThe --set-upstream flag is deprecated and will be removed. Consider using --track or --set-upstream-to\nBranch origin/master set up to track local branch master.\n\nIf you wanted to make 'master' track 'origin/master', do this:\n\n    git branch -d origin/master\n    git branch --set-upstream-to origin/master\n\nSigned-off-by: Carlos Martín Nieto <cmn@elego.de>\n---\n\nThe 'track' in the message is still not great, but it does fit with\nthe one above. Maybe if we make it say \"If youw wanted [...] track the\nremote-tracking branch 'origin/master'\" it would be clearer?\n\nI've simplified and tightened the logic. Now it will only show the\nundo message if the branch didn't exist locally and there is a\nremote-tracking branch of the same name.\n\n builtin/branch.c  | 26 ++++++++++++++++++++++++++\n t/t3200-branch.sh | 34 ++++++++++++++++++++++++++++++++++\n 2 files changed, 60 insertions(+)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 08068f7..a0302a2 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -877,10 +877,36 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tgit_config_set_multivar(buf.buf, NULL, NULL, 1);\n \t\tstrbuf_release(&buf);\n \t} else if (argc > 0 && argc <= 2) {\n+\t\tstruct branch *branch = branch_get(argv[0]);\n+\t\tint branch_existed = 0, remote_tracking = 0;\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\n \t\tif (kinds != REF_LOCAL_BRANCH)\n \t\t\tdie(_(\"-a and -r options to 'git branch' do not make sense with a branch name\"));\n+\n+\t\tif (track == BRANCH_TRACK_OVERRIDE)\n+\t\t\tfprintf(stderr, _(\"The --set-upstream flag is deprecated and will be removed. Consider using --track or --set-upstream-to\\n\"));\n+\n+\t\tstrbuf_addf(&buf, \"refs/remotes/%s\", branch->name);\n+\t\tremote_tracking = ref_exists(buf.buf);\n+\t\tstrbuf_release(&buf);\n+\n+\t\tbranch_existed = ref_exists(branch->refname);\n \t\tcreate_branch(head, argv[0], (argc == 2) ? argv[1] : head,\n \t\t\t      force_create, reflog, 0, quiet, track);\n+\n+\t\t/*\n+\t\t * We only show the instructions if the user gave us\n+\t\t * one branch which doesn't exist locally, but is the\n+\t\t * name of a remote-tracking branch.\n+\t\t */\n+\t\tif (argc == 1 && track == BRANCH_TRACK_OVERRIDE &&\n+\t\t    !branch_existed && remote_tracking) {\n+\t\t\tfprintf(stderr, _(\"\\nIf you wanted to make '%s' track '%s', do this:\\n\\n\"), head, branch->name);\n+\t\t\tfprintf(stderr, _(\"    git branch -d %s\\n\"), branch->name);\n+\t\t\tfprintf(stderr, _(\"    git branch --set-upstream-to %s\\n\"), branch->name);\n+\t\t}\n+\n \t} else\n \t\tusage_with_options(builtin_branch_usage, options);\n \ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 93e5d6e..e9e11cf 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -399,6 +399,40 @@ test_expect_success 'test --unset-upstream on a particular branch' \\\n      test_must_fail git config branch.my14.remote &&\n      test_must_fail git config branch.my14.merge'\n \n+test_expect_success '--set-upstream shows message when creating a new branch that exists as remote-tracking' \\\n+    'git update-ref refs/remotes/origin/master HEAD &&\n+     git branch --set-upstream origin/master 2>actual &&\n+     test_when_finished git update-ref -d refs/remotes/origin/master &&\n+     test_when_finished git branch -d origin/master &&\n+     cat >expected <<EOF &&\n+The --set-upstream flag is deprecated and will be removed. Consider using --track or --set-upstream-to\n+\n+If you wanted to make '\"'master'\"' track '\"'origin/master'\"', do this:\n+\n+    git branch -d origin/master\n+    git branch --set-upstream-to origin/master\n+EOF\n+     test_cmp expected actual\n+'\n+\n+test_expect_success '--set-upstream with two args only shows the deprecation message' \\\n+    'git branch --set-upstream master my13 2>actual &&\n+     test_when_finished git branch --unset-upstream master &&\n+     cat >expected <<EOF &&\n+The --set-upstream flag is deprecated and will be removed. Consider using --track or --set-upstream-to\n+EOF\n+     test_cmp expected actual\n+'\n+\n+test_expect_success '--set-upstream with one arg only shows the deprecation message if the branch existed' \\\n+    'git branch --set-upstream my13 2>actual &&\n+     test_when_finished git branch --unset-upstream master &&\n+     cat >expected <<EOF &&\n+The --set-upstream flag is deprecated and will be removed. Consider using --track or --set-upstream-to\n+EOF\n+     test_cmp expected actual\n+'\n+\n # Keep this test last, as it changes the current branch\n cat >expect <<EOF\n $_z40 $HEAD $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000\tbranch: Created from master\n-- \n1.7.12\n"},{"id":"197736","messageId":"7vtxvt2ts6.fsf@alter.siamese.dyndns.org","threadId":"31294","inReplyTo":"1345748186-1081-1-git-send-email-cmn@elego.de","subject":"Re: [PATCHv2 3/3] branch: deprecate --set-upstream and show help if we detect possible mistaken use","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-23T21:16:41Z","receivedAt":"2012-08-23T21:16:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto <cmn@elego.de> writes:\n\n> The 'track' in the message is still not great, but it does fit with\n> the one above. Maybe if we make it say \"If youw wanted [...] track the\n> remote-tracking branch 'origin/master'\" it would be clearer?\n\nThe verb \"track\" in the phrase \"remote-tracking branch\" means \"keep\ntrack of the branch at the remote, by storing a copy of the last\nobserved state of it\".  In the same sentence, the verb \"track\"\nelsewhere is used to describe what the branch B whose upstream is\nset to B@{upstream} does against B@{upstream}, but that is not\n\"keeping a copy\"---it is doing an entirely different thing.\n\nIf we say that the branch B whose upstream is set to B@{upstream} is\n\"building on top of B@{upstream}\", \"integrating with B@{upstream}\",\n\"forked from B@{upstream}\", etc., without using the verb \"track\"\nthat already means something else (i.e. keeps track of the copy of\nlast observed state), it would reduce the confusion, but I do not\nthink it would clarify anything if the verb \"track\" is used for\nthat.\n\nAs usual, because I am not the best source of phrasing, others may\nfind a better verb than \"builds\", \"forks\", or \"integrates\", though.\n\n> I've simplified and tightened the logic. Now it will only show the\n> undo message if the branch didn't exist locally and there is a\n> remote-tracking branch of the same name.\n\nThe updated and simplified logic reads quite straight-forward, and\nlooks good.  It is likely that the message will be reworded and also\nlocalized in the future, so it would make sense to use test_i18ncmp\nfrom the day one, though.\n\nThanks.  Will queue.\n"},{"id":"197737","messageId":"7vpq6h2tmk.fsf@alter.siamese.dyndns.org","threadId":"31294","inReplyTo":"1345470460-28734-3-git-send-email-cmn@elego.de","subject":"Re: [PATCH 2/3] branch: add --unset-upstream option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-23T21:20:03Z","receivedAt":"2012-08-23T21:20:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlos Martín Nieto <cmn@elego.de> writes:\n\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> index e9019ac..93e5d6e 100755\n> --- a/t/t3200-branch.sh\n> +++ b/t/t3200-branch.sh\n> @@ -383,6 +383,22 @@ test_expect_success 'use --set-upstream-to modify a particular branch' \\\n>       test \"$(git config branch.my13.remote)\" = \".\" &&\n>       test \"$(git config branch.my13.merge)\" = \"refs/heads/master\"'\n>  \n> +test_expect_success 'test --unset-upstream on HEAD' \\\n> +    'git branch my14\n> +     test_config branch.master.remote foo &&\n> +     test_config branch.master.merge foo &&\n> +     git branch --set-upstream-to my14 &&\n> +     git branch --unset-upstream &&\n> +     test_must_fail git config branch.master.remote &&\n> +     test_must_fail git config branch.master.merge'\n> +\n> +test_expect_success 'test --unset-upstream on a particular branch' \\\n> +    'git branch my15\n> +     git branch --set-upstream-to master my14 &&\n> +     git branch --unset-upstream my14 &&\n> +     test_must_fail git config branch.my14.remote &&\n> +     test_must_fail git config branch.my14.merge'\n> +\n\nWhat should happen when you say \"--unset-upstream\" on a branch B\nthat does not have B@{upstream}?  Should it fail?  Should it be\nsilently ignored?  Is it undefined that we do not want to test?\n"},{"id":"197922","messageId":"87a9xgnsxx.fsf@centaur.cmartin.tk","threadId":"31294","inReplyTo":"7vpq6h2tmk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] branch: add --unset-upstream option","fromName":"Carlos Martín Nieto","fromEmail":"cmn@elego.de","sentAt":"2012-08-27T17:30:34Z","receivedAt":"2012-08-27T17:30:34Z","isPatch":true,"sender":{"key":"cmn@elego.de","avatar":"https://avatars.githubusercontent.com/u/335443?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Carlos Martín Nieto <cmn@elego.de> writes:\n>\n>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n>> index e9019ac..93e5d6e 100755\n>> --- a/t/t3200-branch.sh\n>> +++ b/t/t3200-branch.sh\n>> @@ -383,6 +383,22 @@ test_expect_success 'use --set-upstream-to modify a particular branch' \\\n>>       test \"$(git config branch.my13.remote)\" = \".\" &&\n>>       test \"$(git config branch.my13.merge)\" = \"refs/heads/master\"'\n>>  \n>> +test_expect_success 'test --unset-upstream on HEAD' \\\n>> +    'git branch my14\n>> +     test_config branch.master.remote foo &&\n>> +     test_config branch.master.merge foo &&\n>> +     git branch --set-upstream-to my14 &&\n>> +     git branch --unset-upstream &&\n>> +     test_must_fail git config branch.master.remote &&\n>> +     test_must_fail git config branch.master.merge'\n>> +\n>> +test_expect_success 'test --unset-upstream on a particular branch' \\\n>> +    'git branch my15\n>> +     git branch --set-upstream-to master my14 &&\n>> +     git branch --unset-upstream my14 &&\n>> +     test_must_fail git config branch.my14.remote &&\n>> +     test_must_fail git config branch.my14.merge'\n>> +\n>\n> What should happen when you say \"--unset-upstream\" on a branch B\n> that does not have B@{upstream}?  Should it fail?  Should it be\n> silently ignored?  Is it undefined that we do not want to test?\n\nI'd say it should be ignored, as the end result we want is for there to\nbe no upstream information. What we do underneath is remove a couple of\nconfig options, wich doesn't fail if they didn't insist in the first\nplace.\n\nWould you like a test that makes sure two --unset-upstream in a row\ndon't cause any errors?\n\n   cmn\n"},{"id":"197924","messageId":"7vy5l0qknd.fsf@alter.siamese.dyndns.org","threadId":"31294","inReplyTo":"87a9xgnsxx.fsf@centaur.cmartin.tk","subject":"Re: [PATCH 2/3] branch: add --unset-upstream option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-27T18:01:26Z","receivedAt":"2012-08-27T18:01:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"cmn@elego.de (Carlos Martín Nieto) writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Carlos Martín Nieto <cmn@elego.de> writes:\n>>\n>>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n>>> index e9019ac..93e5d6e 100755\n>>> --- a/t/t3200-branch.sh\n>>> +++ b/t/t3200-branch.sh\n>>> @@ -383,6 +383,22 @@ test_expect_success 'use --set-upstream-to modify a particular branch' \\\n>>>       test \"$(git config branch.my13.remote)\" = \".\" &&\n>>>       test \"$(git config branch.my13.merge)\" = \"refs/heads/master\"'\n>>>  \n>>> +test_expect_success 'test --unset-upstream on HEAD' \\\n>>> +    'git branch my14\n>>> +     test_config branch.master.remote foo &&\n>>> +     test_config branch.master.merge foo &&\n>>> +     git branch --set-upstream-to my14 &&\n>>> +     git branch --unset-upstream &&\n>>> +     test_must_fail git config branch.master.remote &&\n>>> +     test_must_fail git config branch.master.merge'\n>>> +\n>>> +test_expect_success 'test --unset-upstream on a particular branch' \\\n>>> +    'git branch my15\n>>> +     git branch --set-upstream-to master my14 &&\n>>> +     git branch --unset-upstream my14 &&\n>>> +     test_must_fail git config branch.my14.remote &&\n>>> +     test_must_fail git config branch.my14.merge'\n>>> +\n>>\n>> What should happen when you say \"--unset-upstream\" on a branch B\n>> that does not have B@{upstream}?  Should it fail?  Should it be\n>> silently ignored?  Is it undefined that we do not want to test?\n>\n> I'd say it should be ignored, as the end result we want is for there to\n> be no upstream information. What we do underneath is remove a couple of\n> config options, wich doesn't fail if they didn't insist in the first\n> place.\n\nI am not sure about that reasoning.\n\n    $ git config --unset core.nosuchvariable ; echo $?\n    5\n\nlooks like a failure to me.\n\nMore importantly, wouldn't we want to catch typo in\n\n\tgit branch --unset-upstream mext\n\nwhich admittedly should come from a different codepath (I do not\nthink your patch checks if \"mext\" branch exists before going ahead\nto the config--unset dance)?\n"},{"id":"197925","messageId":"7vmx1gqk12.fsf@alter.siamese.dyndns.org","threadId":"31294","inReplyTo":"7vy5l0qknd.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] branch: add --unset-upstream option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-27T18:14:49Z","receivedAt":"2012-08-27T18:14:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> cmn@elego.de (Carlos Martín Nieto) writes:\n>\n>> Junio C Hamano <gitster@pobox.com> writes:\n>>\n>>> Carlos Martín Nieto <cmn@elego.de> writes:\n>>>\n>>>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n>>>> index e9019ac..93e5d6e 100755\n>>>> --- a/t/t3200-branch.sh\n>>>> +++ b/t/t3200-branch.sh\n>>>> @@ -383,6 +383,22 @@ test_expect_success 'use --set-upstream-to modify a particular branch' \\\n>>>>       test \"$(git config branch.my13.remote)\" = \".\" &&\n>>>>       test \"$(git config branch.my13.merge)\" = \"refs/heads/master\"'\n>>>>  \n>>>> +test_expect_success 'test --unset-upstream on HEAD' \\\n>>>> +    'git branch my14\n>>>> +     test_config branch.master.remote foo &&\n>>>> +     test_config branch.master.merge foo &&\n>>>> +     git branch --set-upstream-to my14 &&\n>>>> +     git branch --unset-upstream &&\n>>>> +     test_must_fail git config branch.master.remote &&\n>>>> +     test_must_fail git config branch.master.merge'\n>>>> +\n>>>> +test_expect_success 'test --unset-upstream on a particular branch' \\\n>>>> +    'git branch my15\n>>>> +     git branch --set-upstream-to master my14 &&\n>>>> +     git branch --unset-upstream my14 &&\n>>>> +     test_must_fail git config branch.my14.remote &&\n>>>> +     test_must_fail git config branch.my14.merge'\n>>>> +\n>>>\n>>> What should happen when you say \"--unset-upstream\" on a branch B\n>>> that does not have B@{upstream}?  Should it fail?  Should it be\n>>> silently ignored?  Is it undefined that we do not want to test?\n>>\n>> I'd say it should be ignored, as the end result we want is for there to\n>> be no upstream information. What we do underneath is remove a couple of\n>> config options, wich doesn't fail if they didn't insist in the first\n>> place.\n>\n> I am not sure about that reasoning.\n>\n>     $ git config --unset core.nosuchvariable ; echo $?\n>     5\n>\n> looks like a failure to me.\n>\n> More importantly, wouldn't we want to catch typo in\n>\n> \tgit branch --unset-upstream mext\n>\n> which admittedly should come from a different codepath (I do not\n> think your patch checks if \"mext\" branch exists before going ahead\n> to the config--unset dance)?\n\nSorry, the last paragraph was incomplete.\n\nIn general, we should detect errors and allow the user to choose to\nignore.\n\nA script that wants to make sure that B does not have an upstream,\nwithout knowing if it already has one, can say \"--unset-upstream B\"\nand choose to ignore the error if B does not have an upstream.  \n\nIf the script is carefully written, it would try to check if B has\none and call \"--unset-upstream B\" only when it doesn't.  A casually\nwritten loose script would say \"--unset-upstream B 2>/dev/null\"\nand keep going (it would not notice other kinds of errors, but that\nis what makes it \"casual and loose\").\n"},{"id":"197935","messageId":"87vcg4m345.fsf@centaur.cmartin.tk","threadId":"31294","inReplyTo":"7vmx1gqk12.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] branch: add --unset-upstream option","fromName":"Carlos Martín Nieto","fromEmail":"cmn@eleog.de","sentAt":"2012-08-27T21:33:46Z","receivedAt":"2012-08-27T21:33:46Z","isPatch":true,"sender":{"key":"cmn@eleog.de","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> cmn@elego.de (Carlos Martín Nieto) writes:\n>>\n>>> Junio C Hamano <gitster@pobox.com> writes:\n>>>\n>>>> Carlos Martín Nieto <cmn@elego.de> writes:\n>>>>\n>>>>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n>>>>> index e9019ac..93e5d6e 100755\n>>>>> --- a/t/t3200-branch.sh\n>>>>> +++ b/t/t3200-branch.sh\n>>>>> @@ -383,6 +383,22 @@ test_expect_success 'use --set-upstream-to modify a particular branch' \\\n>>>>>       test \"$(git config branch.my13.remote)\" = \".\" &&\n>>>>>       test \"$(git config branch.my13.merge)\" = \"refs/heads/master\"'\n>>>>>  \n>>>>> +test_expect_success 'test --unset-upstream on HEAD' \\\n>>>>> +    'git branch my14\n>>>>> +     test_config branch.master.remote foo &&\n>>>>> +     test_config branch.master.merge foo &&\n>>>>> +     git branch --set-upstream-to my14 &&\n>>>>> +     git branch --unset-upstream &&\n>>>>> +     test_must_fail git config branch.master.remote &&\n>>>>> +     test_must_fail git config branch.master.merge'\n>>>>> +\n>>>>> +test_expect_success 'test --unset-upstream on a particular branch' \\\n>>>>> +    'git branch my15\n>>>>> +     git branch --set-upstream-to master my14 &&\n>>>>> +     git branch --unset-upstream my14 &&\n>>>>> +     test_must_fail git config branch.my14.remote &&\n>>>>> +     test_must_fail git config branch.my14.merge'\n>>>>> +\n>>>>\n>>>> What should happen when you say \"--unset-upstream\" on a branch B\n>>>> that does not have B@{upstream}?  Should it fail?  Should it be\n>>>> silently ignored?  Is it undefined that we do not want to test?\n>>>\n>>> I'd say it should be ignored, as the end result we want is for there to\n>>> be no upstream information. What we do underneath is remove a couple of\n>>> config options, wich doesn't fail if they didn't insist in the first\n>>> place.\n>>\n>> I am not sure about that reasoning.\n>>\n>>     $ git config --unset core.nosuchvariable ; echo $?\n>>     5\n>>\n>> looks like a failure to me.\n>>\n>> More importantly, wouldn't we want to catch typo in\n>>\n>> \tgit branch --unset-upstream mext\n>>\n>> which admittedly should come from a different codepath (I do not\n>> think your patch checks if \"mext\" branch exists before going ahead\n>> to the config--unset dance)?\n>\n> Sorry, the last paragraph was incomplete.\n>\n> In general, we should detect errors and allow the user to choose to\n> ignore.\n>\n> A script that wants to make sure that B does not have an upstream,\n> without knowing if it already has one, can say \"--unset-upstream B\"\n> and choose to ignore the error if B does not have an upstream.  \n>\n> If the script is carefully written, it would try to check if B has\n> one and call \"--unset-upstream B\" only when it doesn't.  A casually\n> written loose script would say \"--unset-upstream B 2>/dev/null\"\n> and keep going (it would not notice other kinds of errors, but that\n> is what makes it \"casual and loose\").\n\nOK, that's a good point, I'll update the patch.\n\n   cmn\n"}]}