{"thread":{"id":"58399","subject":"[PATCH 0/2] branch: support for at-refs like @{-1} in --edit-description, --set-upstream-to and --unset-upstream","startedAt":"2022-09-05T14:34:35Z","lastAt":"2022-10-10T23:25:06Z","messageCount":38,"participants":["Rubén Justo via GitGitGadget","Rubén Justo","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"462634","messageId":"pull.1346.git.1662388460.gitgitgadget@gmail.com","threadId":"58399","inReplyTo":null,"subject":"[PATCH 0/2] branch: support for at-refs like @{-1} in --edit-description, --set-upstream-to and --unset-upstream","fromName":"Rubén Justo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-05T14:34:18Z","receivedAt":"2022-09-05T14:34:35Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"branch command with --edit-description, --set-upstream-to and\n--unset-upstream options expects a branch name. A branch can be specified\nusing at-refs like @{-1}, so those references need to be resolved.\n\nWe can modify the description of the previously checked out branch with:\n\n$ git branch --edit--description @{-1}\n\nWe can modify the upstream of the previously checked out branch with:\n\n$ git branch --set-upstream-to upstream @{-1} $ git branch --unset-upstream\n@{-1}\n\nRubén Justo (2):\n branch: refactor edit_description command switch case\n branch: support for at-refs like @{-1}\n\n builtin/branch.c | 49 +++++++++++++++++++--------\n t/t3204-branch-name-interpretation.sh | 36 ++++++++++++++++++++\n 2 files changed, 70 insertions(+), 15 deletions(-)\n\n\nbase-commit: 795ea8776befc95ea2becd8020c7a284677b4161\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1346%2Frjusto%2Fbranch-support-at-refs-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1346/rjusto/branch-support-at-refs-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1346\n-- \ngitgitgadget\n"},{"id":"462635","messageId":"321854e118c1d0553e3c9efb67851d5763a364fc.1662388460.git.gitgitgadget@gmail.com","threadId":"58399","inReplyTo":"pull.1346.git.1662388460.gitgitgadget@gmail.com","subject":"[PATCH 2/2] branch: support for at-refs like @{-1}","fromName":"Rubén Justo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-05T14:34:20Z","receivedAt":"2022-09-05T14:34:39Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"From: =?UTF-8?q?Rub=C3=A9n=20Justo?= \n\nbranch command with --edit-description, --set-upstream-to and\n--unset-upstream options expects a branch name. A branch can be\nspecified using at-refs like @{-1}, so those references need to\nbe resolved.\n\nWe can modify the description of the previously checked out branch\nwith:\n\n$ git branch --edit--description @{-1}\n\nWe can modify the upstream of the previously checked out branch\nwith:\n\n$ git branch --set-upstream-to upstream @{-1}\n$ git branch --unset-upstream @{-1}\n\nSigned-off-by: Rubén Justo \n---\n builtin/branch.c | 32 +++++++++++++++++++-----\n t/t3204-branch-name-interpretation.sh | 36 +++++++++++++++++++++++++++\n 2 files changed, 62 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 5229cb796f8..df620bee748 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -791,14 +791,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t} else if (edit_description) {\n \t\tconst char *branch_name;\n \t\tstruct strbuf branch_ref = STRBUF_INIT;\n+\t\tstruct strbuf buf = STRBUF_INIT;\n \t\tint ret = 0;\n \n \t\tif (!argc) {\n \t\t\tif (filter.detached)\n \t\t\t\tdie(_(\"Cannot give description to detached HEAD\"));\n \t\t\tbranch_name = head;\n-\t\t} else if (argc == 1)\n-\t\t\tbranch_name = argv[0];\n+\t\t} else if (argc == 1) {\n+\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n+\t\t\tbranch_name = buf.buf;\n+\t\t}\n \t\telse\n \t\t\tdie(_(\"cannot edit description of more than one branch\"));\n \n@@ -814,6 +817,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t\tret = edit_branch_description(branch_name);\n \n \t\tstrbuf_release(&branch_ref);\n+\t\tstrbuf_release(&buf);\n \t\treturn ret;\n \t} else if (copy) {\n \t\tif (!argc)\n@@ -834,11 +838,19 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\telse\n \t\t\tdie(_(\"too many arguments for a rename operation\"));\n \t} else if (new_upstream) {\n-\t\tstruct branch *branch = branch_get(argv[0]);\n+\t\tstruct branch *branch;\n+\t\tstruct strbuf buf = STRBUF_INIT;\n \n-\t\tif (argc > 1)\n+\t\tif (!argc)\n+\t\t\tbranch = branch_get(NULL);\n+\t\telse if (argc == 1) {\n+\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n+\t\t\tbranch = branch_get(buf.buf);\n+\t\t}\n+\t\telse\n \t\t\tdie(_(\"too many arguments to set new upstream\"));\n \n+\n \t\tif (!branch) {\n \t\t\tif (!argc || !strcmp(argv[0], \"HEAD\"))\n \t\t\t\tdie(_(\"could not set upstream of HEAD to %s when \"\n@@ -853,11 +865,18 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tdwim_and_setup_tracking(the_repository, branch->name,\n \t\t\t\t\tnew_upstream, BRANCH_TRACK_OVERRIDE,\n \t\t\t\t\tquiet);\n+\t\tstrbuf_release(&buf);\n \t} else if (unset_upstream) {\n-\t\tstruct branch *branch = branch_get(argv[0]);\n+\t\tstruct branch *branch;\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n-\t\tif (argc > 1)\n+\t\tif (!argc)\n+\t\t\tbranch = branch_get(NULL);\n+\t\telse if (argc == 1) {\n+\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n+\t\t\tbranch = branch_get(buf.buf);\n+\t\t}\n+\t\telse\n \t\t\tdie(_(\"too many arguments to unset upstream\"));\n \n \t\tif (!branch) {\n@@ -870,6 +889,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tif (!branch_has_merge_config(branch))\n \t\t\tdie(_(\"Branch '%s' has no upstream information\"), branch->name);\n \n+\t\tstrbuf_reset(&buf);\n \t\tstrbuf_addf(&buf, \"branch.%s.remote\", branch->name);\n \t\tgit_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);\n \t\tstrbuf_reset(&buf);\ndiff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh\nindex 993a6b5eff7..853315ec20a 100755\n--- a/t/t3204-branch-name-interpretation.sh\n+++ b/t/t3204-branch-name-interpretation.sh\n@@ -133,4 +133,40 @@ test_expect_success 'checkout does not treat remote @{upstream} as a branch' '\n \texpect_branch HEAD one\n '\n \n+test_expect_success 'edit-description via @{-1}' '\n+\tgit checkout main &&\n+\tgit checkout - &&\n+\twrite_script editor <<-\\EOF &&\n+\t\techo \"Branch description\" >\"$1\"\n+\tEOF\n+\tEDITOR=./editor git branch --edit-description @{-1} &&\n+\t\twrite_script editor <<-\\EOF &&\n+\t\tgit stripspace -s <\"$1\" >\"EDITOR_OUTPUT\"\n+\tEOF\n+\tEDITOR=./editor git branch --edit-description @{-1} &&\n+\techo \"Branch description\" >expect &&\n+\ttest_cmp expect EDITOR_OUTPUT\n+'\n+\n+test_expect_success 'modify branch upstream via \"@{-1}@{upstream}\"' \"\n+\tcat >expect <<-\\EOF &&\n+\twarning: not setting branch 'upstream-branch' as its own upstream\n+\tEOF\n+\tgit branch upstream-branch &&\n+\tgit checkout -b upstream-other -t upstream-branch &&\n+\tgit checkout - &&\n+\tgit branch --set-upstream-to upstream-branch @{-1}@{upstream} 2>actual &&\n+\ttest_cmp expect actual\n+\"\n+\n+test_expect_success 'unset branch upstream via \"@{-1}\"' \"\n+\ttest \"$(git config branch.upstream-other.remote)\" = \".\" &&\n+\ttest \"$(git config branch.upstream-other.merge)\" = \"refs/heads/upstream-branch\" &&\n+\tgit checkout upstream-other &&\n+\tgit checkout - &&\n+\tgit branch --unset-upstream @{-1} &&\n+\ttest_must_fail git config branch.upstream-other.remote &&\n+\ttest_must_fail git config branch.upstream-other.merge\n+\"\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"462636","messageId":"aaa2f3ec3139a7a48001d1d0357c302d0dc68780.1662388460.git.gitgitgadget@gmail.com","threadId":"58399","inReplyTo":"pull.1346.git.1662388460.gitgitgadget@gmail.com","subject":"[PATCH 1/2] branch: refactor edit_description command switch case","fromName":"Rubén Justo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-05T14:34:19Z","receivedAt":"2022-09-05T14:34:41Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"From: =?UTF-8?q?Rub=C3=A9n=20Justo?= \n\nMinor refactoring to reduce the return exits in the switch case\nhandling the edit_description command, so the calls to strbuf_release\ncan also be reduced.\n\nSigned-off-by: Rubén Justo \n---\n builtin/branch.c | 17 ++++++++---------\n 1 file changed, 8 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 55cd9a6e998..5229cb796f8 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -614,7 +614,7 @@ static int edit_branch_description(const char *branch_name)\n \tstrbuf_reset(&buf);\n \tif (launch_editor(edit_description(), &buf, NULL)) {\n \t\tstrbuf_release(&buf);\n-\t\treturn -1;\n+\t\treturn 1;\n \t}\n \tstrbuf_stripspace(&buf, 1);\n \n@@ -791,6 +791,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t} else if (edit_description) {\n \t\tconst char *branch_name;\n \t\tstruct strbuf branch_ref = STRBUF_INIT;\n+\t\tint ret = 0;\n \n \t\tif (!argc) {\n \t\t\tif (filter.detached)\n@@ -803,19 +804,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \n \t\tstrbuf_addf(&branch_ref, \"refs/heads/%s\", branch_name);\n \t\tif (!ref_exists(branch_ref.buf)) {\n-\t\t\tstrbuf_release(&branch_ref);\n-\n \t\t\tif (!argc)\n-\t\t\t\treturn error(_(\"No commit on branch '%s' yet.\"),\n+\t\t\t\tret = error(_(\"No commit on branch '%s' yet.\"),\n \t\t\t\t\t branch_name);\n \t\t\telse\n-\t\t\t\treturn error(_(\"No branch named '%s'.\"),\n+\t\t\t\tret = error(_(\"No branch named '%s'.\"),\n \t\t\t\t\t branch_name);\n-\t\t}\n-\t\tstrbuf_release(&branch_ref);\n+\t\t} else\n+\t\t\tret = edit_branch_description(branch_name);\n \n-\t\tif (edit_branch_description(branch_name))\n-\t\t\treturn 1;\n+\t\tstrbuf_release(&branch_ref);\n+\t\treturn ret;\n \t} else if (copy) {\n \t\tif (!argc)\n \t\t\tdie(_(\"branch name required\"));\n-- \ngitgitgadget\n\n"},{"id":"462695","messageId":"93b0b442-b277-66a6-3f5f-5a498593aa07@gmail.com","threadId":"58399","inReplyTo":"pull.1346.git.1662388460.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] branch: support for shortcuts like @{-1}, completed","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-09-07T09:45:36Z","receivedAt":"2022-09-07T09:45:44Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"branch options: \"edit-description\", \"set-upstream-to\" and \"unset-upstream\"\ndon't handle shortcuts like @{-1}.\n\nThis patch address this problem.\n\nIn v2 I've resolved some noise in the code, aggregated two tests in one with\nsome refactoring to do the test in a more simple way while covering the\nfunctionality. Also changed the commit messages to better explain and\ndocument the change.\n\nThanks\n\n\nRubén Justo (2):\n branch: refactor \"edit_description\" code path\n branch: support for shortcuts like @{-1} completed\n\n builtin/branch.c | 46 ++++++++++++++++++---------\n t/t3204-branch-name-interpretation.sh | 25 +++++++++++++++\n 2 files changed, 56 insertions(+), 15 deletions(-)\n\nbase-commit: 795ea8776befc95ea2becd8020c7a284677b4161\n\nRange-diff vs v1:\n\n1: aaa2f3ec31 ! 1: ce14194187 branch: refactor edit_description command switch case\n @@ Metadata\n Author: Rubén Justo \n \n ## Commit message ##\n - branch: refactor edit_description command switch case\n + branch: refactor \"edit_description\" code path\n \n - Minor refactoring to reduce the return exits in the switch case\n - handling the edit_description command, so the calls to strbuf_release\n - can also be reduced.\n + Minor refactoring to reduce the number of returns in the switch case\n + handling the \"edit_description\" option, so the calls to strbuf_release\n + can also be reduced. New resources to be added also do not need to be\n + released in multiple places.\n \n Signed-off-by: Rubén Justo \n \n2: 321854e118 ! 2: 3a591cab8a branch: support for at-refs like @{-1}\n @@ Metadata\n Author: Rubén Justo \n \n ## Commit message ##\n - branch: support for at-refs like @{-1}\n + branch: support for shortcuts like @{-1} completed\n \n - branch command with --edit-description, --set-upstream-to and\n - --unset-upstream options expects a branch name. A branch can be\n - specified using at-refs like @{-1}, so those references need to\n - be resolved.\n + branch command with options \"edit-description\", \"set-upstream-to\"\n + and \"unset-upstream\" expects a branch name. Since ae5a6c3684\n + (checkout: implement \"@{-N}\" shortcut name for N-th last branch,\n + 2009-01-17) a branch can be specified using shortcuts like @{-1}.\n + Those shortcuts need to be resolved when considering the\n + arguments.\n +\n + Usage examples:\n \n We can modify the description of the previously checked out branch\n with:\n @@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix\n +\t\telse if (argc == 1) {\n +\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n +\t\t\tbranch = branch_get(buf.buf);\n -+\t\t}\n -+\t\telse\n ++\t\t} else\n \t\t\tdie(_(\"too many arguments to set new upstream\"));\n \n -+\n \t\tif (!branch) {\n - \t\t\tif (!argc || !strcmp(argv[0], \"HEAD\"))\n - \t\t\t\tdie(_(\"could not set upstream of HEAD to %s when \"\n @@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tdwim_and_setup_tracking(the_repository, branch->name,\n \t\t\t\t\tnew_upstream, BRANCH_TRACK_OVERRIDE,\n @@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix\n +\t\telse if (argc == 1) {\n +\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n +\t\t\tbranch = branch_get(buf.buf);\n -+\t\t}\n -+\t\telse\n ++\t\t} else\n \t\t\tdie(_(\"too many arguments to unset upstream\"));\n \n \t\tif (!branch) {\n @@ t/t3204-branch-name-interpretation.sh: test_expect_success 'checkout does not tr\n +\ttest_cmp expect EDITOR_OUTPUT\n +'\n +\n -+test_expect_success 'modify branch upstream via \"@{-1}@{upstream}\"' \"\n -+\tcat >expect <<-\\EOF &&\n -+\twarning: not setting branch 'upstream-branch' as its own upstream\n -+\tEOF\n ++test_expect_success 'modify branch upstream via \"@{-1}\" and \"@{-1}@{upstream}\"' '\n +\tgit branch upstream-branch &&\n +\tgit checkout -b upstream-other -t upstream-branch &&\n +\tgit checkout - &&\n -+\tgit branch --set-upstream-to upstream-branch @{-1}@{upstream} 2>actual &&\n -+\ttest_cmp expect actual\n -+\"\n -+\n -+test_expect_success 'unset branch upstream via \"@{-1}\"' \"\n -+\ttest \"$(git config branch.upstream-other.remote)\" = \".\" &&\n -+\ttest \"$(git config branch.upstream-other.merge)\" = \"refs/heads/upstream-branch\" &&\n -+\tgit checkout upstream-other &&\n -+\tgit checkout - &&\n ++\tgit branch --set-upstream-to @{-1} @{-1}@{upstream} &&\n ++\ttest \"$(git config branch.upstream-branch.merge)\" = \"refs/heads/upstream-other\" &&\n +\tgit branch --unset-upstream @{-1} &&\n -+\ttest_must_fail git config branch.upstream-other.remote &&\n +\ttest_must_fail git config branch.upstream-other.merge\n -+\"\n ++'\n +\n test_done\n\n-- \n2.36.1\n"},{"id":"462696","messageId":"d2624238-048c-ac5b-1d45-e08051202c79@gmail.com","threadId":"58399","inReplyTo":"93b0b442-b277-66a6-3f5f-5a498593aa07@gmail.com","subject":"[PATCH v2 1/2] branch: refactor \"edit_description\" code path","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-09-07T09:52:26Z","receivedAt":"2022-09-07T09:52:36Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Minor refactoring to reduce the number of returns in the switch case\nhandling the \"edit_description\" option, so the calls to strbuf_release\ncan also be reduced. New resources to be added also do not need to be\nreleased in multiple places.\n\nSigned-off-by: Rubén Justo \n---\n builtin/branch.c | 17 ++++++++---------\n 1 file changed, 8 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 55cd9a6e99..5229cb796f 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -614,7 +614,7 @@ static int edit_branch_description(const char *branch_name)\n \tstrbuf_reset(&buf);\n \tif (launch_editor(edit_description(), &buf, NULL)) {\n \t\tstrbuf_release(&buf);\n-\t\treturn -1;\n+\t\treturn 1;\n \t}\n \tstrbuf_stripspace(&buf, 1);\n \n@@ -791,6 +791,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t} else if (edit_description) {\n \t\tconst char *branch_name;\n \t\tstruct strbuf branch_ref = STRBUF_INIT;\n+\t\tint ret = 0;\n \n \t\tif (!argc) {\n \t\t\tif (filter.detached)\n@@ -803,19 +804,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \n \t\tstrbuf_addf(&branch_ref, \"refs/heads/%s\", branch_name);\n \t\tif (!ref_exists(branch_ref.buf)) {\n-\t\t\tstrbuf_release(&branch_ref);\n-\n \t\t\tif (!argc)\n-\t\t\t\treturn error(_(\"No commit on branch '%s' yet.\"),\n+\t\t\t\tret = error(_(\"No commit on branch '%s' yet.\"),\n \t\t\t\t\t branch_name);\n \t\t\telse\n-\t\t\t\treturn error(_(\"No branch named '%s'.\"),\n+\t\t\t\tret = error(_(\"No branch named '%s'.\"),\n \t\t\t\t\t branch_name);\n-\t\t}\n-\t\tstrbuf_release(&branch_ref);\n+\t\t} else\n+\t\t\tret = edit_branch_description(branch_name);\n \n-\t\tif (edit_branch_description(branch_name))\n-\t\t\treturn 1;\n+\t\tstrbuf_release(&branch_ref);\n+\t\treturn ret;\n \t} else if (copy) {\n \t\tif (!argc)\n \t\t\tdie(_(\"branch name required\"));\n-- \n2.36.1\n"},{"id":"462697","messageId":"4bac4681-d1cf-1469-eaac-a4c2c63c562d@gmail.com","threadId":"58399","inReplyTo":"93b0b442-b277-66a6-3f5f-5a498593aa07@gmail.com","subject":"[PATCH v2 2/2] branch: support for shortcuts like @{-1} completed","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-09-07T09:53:32Z","receivedAt":"2022-09-07T09:53:49Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"branch command with options \"edit-description\", \"set-upstream-to\"\nand \"unset-upstream\" expects a branch name. Since ae5a6c3684\n(checkout: implement \"@{-N}\" shortcut name for N-th last branch,\n2009-01-17) a branch can be specified using shortcuts like @{-1}.\nThose shortcuts need to be resolved when considering the\narguments.\n\nUsage examples:\n\nWe can modify the description of the previously checked out branch\nwith:\n\n$ git branch --edit--description @{-1}\n\nWe can modify the upstream of the previously checked out branch\nwith:\n\n$ git branch --set-upstream-to upstream @{-1}\n$ git branch --unset-upstream @{-1}\n\nSigned-off-by: Rubén Justo \n---\n builtin/branch.c | 29 +++++++++++++++++++++------\n t/t3204-branch-name-interpretation.sh | 25 +++++++++++++++++++++++\n 2 files changed, 48 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 5229cb796f..6ee95b4179 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -791,14 +791,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t} else if (edit_description) {\n \t\tconst char *branch_name;\n \t\tstruct strbuf branch_ref = STRBUF_INIT;\n+\t\tstruct strbuf buf = STRBUF_INIT;\n \t\tint ret = 0;\n \n \t\tif (!argc) {\n \t\t\tif (filter.detached)\n \t\t\t\tdie(_(\"Cannot give description to detached HEAD\"));\n \t\t\tbranch_name = head;\n-\t\t} else if (argc == 1)\n-\t\t\tbranch_name = argv[0];\n+\t\t} else if (argc == 1) {\n+\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n+\t\t\tbranch_name = buf.buf;\n+\t\t}\n \t\telse\n \t\t\tdie(_(\"cannot edit description of more than one branch\"));\n \n@@ -814,6 +817,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t\tret = edit_branch_description(branch_name);\n \n \t\tstrbuf_release(&branch_ref);\n+\t\tstrbuf_release(&buf);\n \t\treturn ret;\n \t} else if (copy) {\n \t\tif (!argc)\n@@ -834,9 +838,15 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\telse\n \t\t\tdie(_(\"too many arguments for a rename operation\"));\n \t} else if (new_upstream) {\n-\t\tstruct branch *branch = branch_get(argv[0]);\n+\t\tstruct branch *branch;\n+\t\tstruct strbuf buf = STRBUF_INIT;\n \n-\t\tif (argc > 1)\n+\t\tif (!argc)\n+\t\t\tbranch = branch_get(NULL);\n+\t\telse if (argc == 1) {\n+\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n+\t\t\tbranch = branch_get(buf.buf);\n+\t\t} else\n \t\t\tdie(_(\"too many arguments to set new upstream\"));\n \n \t\tif (!branch) {\n@@ -853,11 +863,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tdwim_and_setup_tracking(the_repository, branch->name,\n \t\t\t\t\tnew_upstream, BRANCH_TRACK_OVERRIDE,\n \t\t\t\t\tquiet);\n+\t\tstrbuf_release(&buf);\n \t} else if (unset_upstream) {\n-\t\tstruct branch *branch = branch_get(argv[0]);\n+\t\tstruct branch *branch;\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n-\t\tif (argc > 1)\n+\t\tif (!argc)\n+\t\t\tbranch = branch_get(NULL);\n+\t\telse if (argc == 1) {\n+\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n+\t\t\tbranch = branch_get(buf.buf);\n+\t\t} else\n \t\t\tdie(_(\"too many arguments to unset upstream\"));\n \n \t\tif (!branch) {\n@@ -870,6 +886,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tif (!branch_has_merge_config(branch))\n \t\t\tdie(_(\"Branch '%s' has no upstream information\"), branch->name);\n \n+\t\tstrbuf_reset(&buf);\n \t\tstrbuf_addf(&buf, \"branch.%s.remote\", branch->name);\n \t\tgit_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);\n \t\tstrbuf_reset(&buf);\ndiff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh\nindex 993a6b5eff..0a423298d6 100755\n--- a/t/t3204-branch-name-interpretation.sh\n+++ b/t/t3204-branch-name-interpretation.sh\n@@ -133,4 +133,29 @@ test_expect_success 'checkout does not treat remote @{upstream} as a branch' '\n \texpect_branch HEAD one\n '\n \n+test_expect_success 'edit-description via @{-1}' '\n+\tgit checkout main &&\n+\tgit checkout - &&\n+\twrite_script editor <<-\\EOF &&\n+\t\techo \"Branch description\" >\"$1\"\n+\tEOF\n+\tEDITOR=./editor git branch --edit-description @{-1} &&\n+\t\twrite_script editor <<-\\EOF &&\n+\t\tgit stripspace -s <\"$1\" >\"EDITOR_OUTPUT\"\n+\tEOF\n+\tEDITOR=./editor git branch --edit-description @{-1} &&\n+\techo \"Branch description\" >expect &&\n+\ttest_cmp expect EDITOR_OUTPUT\n+'\n+\n+test_expect_success 'modify branch upstream via \"@{-1}\" and \"@{-1}@{upstream}\"' '\n+\tgit branch upstream-branch &&\n+\tgit checkout -b upstream-other -t upstream-branch &&\n+\tgit checkout - &&\n+\tgit branch --set-upstream-to @{-1} @{-1}@{upstream} &&\n+\ttest \"$(git config branch.upstream-branch.merge)\" = \"refs/heads/upstream-other\" &&\n+\tgit branch --unset-upstream @{-1} &&\n+\ttest_must_fail git config branch.upstream-other.merge\n+'\n+\n test_done\n-- \n2.36.1\n"},{"id":"462755","messageId":"xmqq7d2fszhk.fsf@gitster.g","threadId":"58399","inReplyTo":"d2624238-048c-ac5b-1d45-e08051202c79@gmail.com","subject":"Re: [PATCH v2 1/2] branch: refactor \"edit_description\" code path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-07T20:25:59Z","receivedAt":"2022-09-07T20:26:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> Minor refactoring to reduce the number of returns in the switch case\n> handling the \"edit_description\" option, so the calls to strbuf_release\n> can also be reduced. New resources to be added also do not need to be\n> released in multiple places.\n>\n> Signed-off-by: Rubén Justo \n> ---\n> builtin/branch.c | 17 ++++++++---------\n> 1 file changed, 8 insertions(+), 9 deletions(-)\n>\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index 55cd9a6e99..5229cb796f 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -614,7 +614,7 @@ static int edit_branch_description(const char *branch_name)\n> \tstrbuf_reset(&buf);\n> \tif (launch_editor(edit_description(), &buf, NULL)) {\n> \t\tstrbuf_release(&buf);\n> -\t\treturn -1;\n> +\t\treturn 1;\n> \t}\n> \tstrbuf_stripspace(&buf, 1);\n\n\nOur API convention is to signal a failure with negative return\nvalue. Granted that this is not a general API but is merely a\nhelper function in the implementation of a single command, it would\nbe less confusing if you sticked to the convention.\n\nUnless there is a compelling reason not to, that is.\n\n> @@ -791,6 +791,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> \t} else if (edit_description) {\n> \t\tconst char *branch_name;\n> \t\tstruct strbuf branch_ref = STRBUF_INIT;\n> +\t\tint ret = 0;\n> \n> \t\tif (!argc) {\n> \t\t\tif (filter.detached)\n> @@ -803,19 +804,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> \n> \t\tstrbuf_addf(&branch_ref, \"refs/heads/%s\", branch_name);\n> \t\tif (!ref_exists(branch_ref.buf)) {\n> -\t\t\tstrbuf_release(&branch_ref);\n> -\n> \t\t\tif (!argc)\n> -\t\t\t\treturn error(_(\"No commit on branch '%s' yet.\"),\n> +\t\t\t\tret = error(_(\"No commit on branch '%s' yet.\"),\n> \t\t\t\t\t branch_name);\n> \t\t\telse\n> -\t\t\t\treturn error(_(\"No branch named '%s'.\"),\n> +\t\t\t\tret = error(_(\"No branch named '%s'.\"),\n> \t\t\t\t\t branch_name);\n\nOK. These are good uses of a new variable 'ret'. Note that error()\nreturns negative one.\n\n> -\t\t}\n> -\t\tstrbuf_release(&branch_ref);\n> +\t\t} else\n> +\t\t\tret = edit_branch_description(branch_name);\n> \n> -\t\tif (edit_branch_description(branch_name))\n> -\t\t\treturn 1;\n> +\t\tstrbuf_release(&branch_ref);\n> +\t\treturn ret;\n\nWhen editor failed, we leaked branch_ref strbuf, but we no longer\ndo.\n\nWhich is good.\n\nThis makes cmd_branch() return -1 (when we see error() call) or 1\n(when edit_branch_description() fails and returns 1). I would\nsuggest to\n\n * Fix the return value of edit_branch_description() so that it\n signals a failure by returning -1\n\n * cmd_branch() to return (or call exit() with) -ret, as ret has 0\n when everything is peachy, and negative in any error code paths.\n\n"},{"id":"462765","messageId":"a5e5f94d-965b-3766-60c2-27183148e052@gmail.com","threadId":"58399","inReplyTo":"xmqq7d2fszhk.fsf@gitster.g","subject":"Re: [PATCH v2 1/2] branch: refactor \"edit_description\" code path","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-09-07T21:24:10Z","receivedAt":"2022-09-07T21:24:23Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"\nOn 9/7/22 10:25 PM, Junio C Hamano wrote:\n\nMaybe the return 1 when edit_branch_description(), was a typo I was maintaining?\n\n>> -\t\t}\n>> -\t\tstrbuf_release(&branch_ref);\n>> +\t\t} else\n>> +\t\t\tret = edit_branch_description(branch_name);\n>> \n>> -\t\tif (edit_branch_description(branch_name))\n>> -\t\t\treturn 1;\n>> +\t\tstrbuf_release(&branch_ref);\n>> +\t\treturn ret;\n\nThanks for you review.\n"},{"id":"462785","messageId":"cfe2ec91-0d7a-3634-feda-82a48e79638d@gmail.com","threadId":"58399","inReplyTo":"a5e5f94d-965b-3766-60c2-27183148e052@gmail.com","subject":"Re: [PATCH v2 1/2] branch: refactor \"edit_description\" code path","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-09-08T04:32:43Z","receivedAt":"2022-09-08T04:32:49Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 9/7/22 11:24 PM, Rubén Justo wrote:\n> \n> On 9/7/22 10:25 PM, Junio C Hamano wrote:\n> \n> Maybe the return 1 when edit_branch_description(), was a typo I was maintaining?\n> \n>>> -\t\t}\n>>> -\t\tstrbuf_release(&branch_ref);\n>>> +\t\t} else\n>>> +\t\t\tret = edit_branch_description(branch_name);\n>>> \n>>> -\t\tif (edit_branch_description(branch_name))\n>>> -\t\t\treturn 1;\n>>> +\t\tstrbuf_release(&branch_ref);\n>>> +\t\treturn ret;\n> \n> Thanks for you review.\n> \n\nNevermind. 1 is the return for exit. Sorry.\n\nI'm sending a v3 with your suggestions. Thanks again.\n"},{"id":"462786","messageId":"67253834-8bfa-c249-6594-6418d6ae6805@gmail.com","threadId":"58399","inReplyTo":"7abdb5a9-5707-7897-4196-8d2892beeb81@gmail.com","subject":"[PATCH v3 1/2] branch: refactor \"edit_description\" code path","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-09-08T04:51:42Z","receivedAt":"2022-09-08T04:51:49Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Minor refactoring to reduce the number of returns in the switch case\nhandling the \"edit_description\" option, so the calls to strbuf_release\ncan also be reduced. New resources to be added also do not need to be\nreleased in multiple places.\n\nSigned-off-by: Rubén Justo \n---\n builtin/branch.c | 15 +++++++--------\n 1 file changed, 7 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 55cd9a6e99..b1f6519cd9 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -791,6 +791,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t} else if (edit_description) {\n \t\tconst char *branch_name;\n \t\tstruct strbuf branch_ref = STRBUF_INIT;\n+\t\tint ret = 0;\n \n \t\tif (!argc) {\n \t\t\tif (filter.detached)\n@@ -803,19 +804,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \n \t\tstrbuf_addf(&branch_ref, \"refs/heads/%s\", branch_name);\n \t\tif (!ref_exists(branch_ref.buf)) {\n-\t\t\tstrbuf_release(&branch_ref);\n-\n \t\t\tif (!argc)\n-\t\t\t\treturn error(_(\"No commit on branch '%s' yet.\"),\n+\t\t\t\tret = error(_(\"No commit on branch '%s' yet.\"),\n \t\t\t\t\t branch_name);\n \t\t\telse\n-\t\t\t\treturn error(_(\"No branch named '%s'.\"),\n+\t\t\t\tret = error(_(\"No branch named '%s'.\"),\n \t\t\t\t\t branch_name);\n-\t\t}\n-\t\tstrbuf_release(&branch_ref);\n+\t\t} else if (edit_branch_description(branch_name))\n+\t\t\tret = 1;\n \n-\t\tif (edit_branch_description(branch_name))\n-\t\t\treturn 1;\n+\t\tstrbuf_release(&branch_ref);\n+\t\treturn ret;\n \t} else if (copy) {\n \t\tif (!argc)\n \t\t\tdie(_(\"branch name required\"));\n-- \n2.36.1\n"},{"id":"462787","messageId":"7abdb5a9-5707-7897-4196-8d2892beeb81@gmail.com","threadId":"58399","inReplyTo":"93b0b442-b277-66a6-3f5f-5a498593aa07@gmail.com","subject":"[PATCH v3 0/2] branch: support for shortcuts like @{-1}, completed","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-09-08T04:47:00Z","receivedAt":"2022-09-08T04:51:49Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"branch options: \"edit-description\", \"set-upstream-to\" and \"unset-upstream\"\ndon't handle shortcuts like @{-1}.\n \nThis patch address this problem.\n \nChanges in v3 are about following code conventions to avoid unnecessary confusions, pointed out by Junio in .\n \nThanks.\n\n\nRubén Justo (2):\n branch: refactor \"edit_description\" code path\n branch: support for shortcuts like @{-1} completed\n\n builtin/branch.c | 44 ++++++++++++++++++---------\n t/t3204-branch-name-interpretation.sh | 25 +++++++++++++++\n 2 files changed, 55 insertions(+), 14 deletions(-)\n\n\nRange-diff:\n1: ce14194187 ! 1: 0a69b6cf18 branch: refactor \"edit_description\" code path\n @@ Commit message\n Signed-off-by: Rubén Justo \n \n ## builtin/branch.c ##\n -@@ builtin/branch.c: static int edit_branch_description(const char *branch_name)\n - \tstrbuf_reset(&buf);\n - \tif (launch_editor(edit_description(), &buf, NULL)) {\n - \t\tstrbuf_release(&buf);\n --\t\treturn -1;\n -+\t\treturn 1;\n - \t}\n - \tstrbuf_stripspace(&buf, 1);\n - \n @@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix)\n \t} else if (edit_description) {\n \t\tconst char *branch_name;\n @@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix\n \t\t\t\t\t branch_name);\n -\t\t}\n -\t\tstrbuf_release(&branch_ref);\n -+\t\t} else\n -+\t\t\tret = edit_branch_description(branch_name);\n ++\t\t} else if (edit_branch_description(branch_name))\n ++\t\t\tret = 1;\n \n -\t\tif (edit_branch_description(branch_name))\n -\t\t\treturn 1;\n2: 3a591cab8a ! 2: 91b308b52a branch: support for shortcuts like @{-1} completed\n @@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix\n \t\t\tdie(_(\"cannot edit description of more than one branch\"));\n \n @@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix)\n - \t\t\tret = edit_branch_description(branch_name);\n + \t\t\tret = 1;\n \n \t\tstrbuf_release(&branch_ref);\n +\t\tstrbuf_release(&buf);\n-- \n2.36.1\n"},{"id":"462789","messageId":"c38eec87-a88b-761a-c58d-22b0566aa608@gmail.com","threadId":"58399","inReplyTo":"7abdb5a9-5707-7897-4196-8d2892beeb81@gmail.com","subject":"[PATCH v3 2/2] branch: support for shortcuts like @{-1} completed","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-09-08T04:53:54Z","receivedAt":"2022-09-08T04:54:00Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"branch command with options \"edit-description\", \"set-upstream-to\"\nand \"unset-upstream\" expects a branch name. Since ae5a6c3684\n(checkout: implement \"@{-N}\" shortcut name for N-th last branch,\n2009-01-17) a branch can be specified using shortcuts like @{-1}.\nThose shortcuts need to be resolved when considering the\narguments.\n\nUsage examples:\n\nWe can modify the description of the previously checked out branch\nwith:\n\n$ git branch --edit--description @{-1}\n\nWe can modify the upstream of the previously checked out branch\nwith:\n\n$ git branch --set-upstream-to upstream @{-1}\n$ git branch --unset-upstream @{-1}\n\nSigned-off-by: Rubén Justo \n---\n builtin/branch.c | 29 +++++++++++++++++++++------\n t/t3204-branch-name-interpretation.sh | 25 +++++++++++++++++++++++\n 2 files changed, 48 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex b1f6519cd9..a4069d27e0 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -791,14 +791,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t} else if (edit_description) {\n \t\tconst char *branch_name;\n \t\tstruct strbuf branch_ref = STRBUF_INIT;\n+\t\tstruct strbuf buf = STRBUF_INIT;\n \t\tint ret = 0;\n \n \t\tif (!argc) {\n \t\t\tif (filter.detached)\n \t\t\t\tdie(_(\"Cannot give description to detached HEAD\"));\n \t\t\tbranch_name = head;\n-\t\t} else if (argc == 1)\n-\t\t\tbranch_name = argv[0];\n+\t\t} else if (argc == 1) {\n+\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n+\t\t\tbranch_name = buf.buf;\n+\t\t}\n \t\telse\n \t\t\tdie(_(\"cannot edit description of more than one branch\"));\n \n@@ -814,6 +817,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t\tret = 1;\n \n \t\tstrbuf_release(&branch_ref);\n+\t\tstrbuf_release(&buf);\n \t\treturn ret;\n \t} else if (copy) {\n \t\tif (!argc)\n@@ -834,9 +838,15 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\telse\n \t\t\tdie(_(\"too many arguments for a rename operation\"));\n \t} else if (new_upstream) {\n-\t\tstruct branch *branch = branch_get(argv[0]);\n+\t\tstruct branch *branch;\n+\t\tstruct strbuf buf = STRBUF_INIT;\n \n-\t\tif (argc > 1)\n+\t\tif (!argc)\n+\t\t\tbranch = branch_get(NULL);\n+\t\telse if (argc == 1) {\n+\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n+\t\t\tbranch = branch_get(buf.buf);\n+\t\t} else\n \t\t\tdie(_(\"too many arguments to set new upstream\"));\n \n \t\tif (!branch) {\n@@ -853,11 +863,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tdwim_and_setup_tracking(the_repository, branch->name,\n \t\t\t\t\tnew_upstream, BRANCH_TRACK_OVERRIDE,\n \t\t\t\t\tquiet);\n+\t\tstrbuf_release(&buf);\n \t} else if (unset_upstream) {\n-\t\tstruct branch *branch = branch_get(argv[0]);\n+\t\tstruct branch *branch;\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n-\t\tif (argc > 1)\n+\t\tif (!argc)\n+\t\t\tbranch = branch_get(NULL);\n+\t\telse if (argc == 1) {\n+\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n+\t\t\tbranch = branch_get(buf.buf);\n+\t\t} else\n \t\t\tdie(_(\"too many arguments to unset upstream\"));\n \n \t\tif (!branch) {\n@@ -870,6 +886,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tif (!branch_has_merge_config(branch))\n \t\t\tdie(_(\"Branch '%s' has no upstream information\"), branch->name);\n \n+\t\tstrbuf_reset(&buf);\n \t\tstrbuf_addf(&buf, \"branch.%s.remote\", branch->name);\n \t\tgit_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);\n \t\tstrbuf_reset(&buf);\ndiff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh\nindex 993a6b5eff..0a423298d6 100755\n--- a/t/t3204-branch-name-interpretation.sh\n+++ b/t/t3204-branch-name-interpretation.sh\n@@ -133,4 +133,29 @@ test_expect_success 'checkout does not treat remote @{upstream} as a branch' '\n \texpect_branch HEAD one\n '\n \n+test_expect_success 'edit-description via @{-1}' '\n+\tgit checkout main &&\n+\tgit checkout - &&\n+\twrite_script editor <<-\\EOF &&\n+\t\techo \"Branch description\" >\"$1\"\n+\tEOF\n+\tEDITOR=./editor git branch --edit-description @{-1} &&\n+\t\twrite_script editor <<-\\EOF &&\n+\t\tgit stripspace -s <\"$1\" >\"EDITOR_OUTPUT\"\n+\tEOF\n+\tEDITOR=./editor git branch --edit-description @{-1} &&\n+\techo \"Branch description\" >expect &&\n+\ttest_cmp expect EDITOR_OUTPUT\n+'\n+\n+test_expect_success 'modify branch upstream via \"@{-1}\" and \"@{-1}@{upstream}\"' '\n+\tgit branch upstream-branch &&\n+\tgit checkout -b upstream-other -t upstream-branch &&\n+\tgit checkout - &&\n+\tgit branch --set-upstream-to @{-1} @{-1}@{upstream} &&\n+\ttest \"$(git config branch.upstream-branch.merge)\" = \"refs/heads/upstream-other\" &&\n+\tgit branch --unset-upstream @{-1} &&\n+\ttest_must_fail git config branch.upstream-other.merge\n+'\n+\n test_done\n-- \n2.36.1\n"},{"id":"462833","messageId":"de06f80c-4778-7e43-3a88-d7aeae788e0d@gmail.com","threadId":"58399","inReplyTo":"67253834-8bfa-c249-6594-6418d6ae6805@gmail.com","subject":"[PATCH] branch: error codes for \"edit_description\"","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-09-08T20:57:12Z","receivedAt":"2022-09-08T20:57:20Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Stick to the convention of exit with 1 on error. May break some\nthird party tool that expects -1 on \"No branch named 'bla'\" or\n\"No commit on branch 'bla' yet.\" errors.\n\nTest added to avoid regresion on this behaviour.\n---\n\nJunio, here is the patch for the change you suggested [1]. My concern is\nthis might break some third party tool or script. If no one thinks this\nis an issue, I can squash it with the patch I'm responding to.\n\n\n[1] https://lore.kernel.org/git/xmqq7d2fszhk.fsf@gitster.g/\n\n builtin/branch.c | 6 +++---\n t/t3200-branch.sh | 8 ++++++++\n 2 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex b1f6519cd9..01b6364f58 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -810,11 +810,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t\telse\n \t\t\t\tret = error(_(\"No branch named '%s'.\"),\n \t\t\t\t\t branch_name);\n-\t\t} else if (edit_branch_description(branch_name))\n-\t\t\tret = 1;\n+\t\t} else\n+\t\t\tret = edit_branch_description(branch_name);\n \n \t\tstrbuf_release(&branch_ref);\n-\t\treturn ret;\n+\t\treturn -ret;\n \t} else if (copy) {\n \t\tif (!argc)\n \t\t\tdie(_(\"branch name required\"));\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex 9723c2827c..d09db3fe3d 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -1409,6 +1409,14 @@ test_expect_success 'refuse --edit-description on unborn branch for now' '\n \ttest_must_fail env EDITOR=./editor git branch --edit-description\n '\n \n+test_expect_success 'test error codes using --edit-description' '\n+\ttest_expect_code 1 env EDITOR=./invalid-editor git branch --edit-description main &&\n+\twrite_script editor <<-\\EOF &&\n+\t\techo \"New contents\" >\"$1\"\n+\tEOF\n+\ttest_expect_code 1 env EDITOR=./editor git branch --edit-description no-such-branch\n+'\n+\n test_expect_success '--merged catches invalid object names' '\n \ttest_must_fail git branch --merged 0000000000000000000000000000000000000000\n '\n-- \n2.36.1\n"},{"id":"462837","messageId":"xmqq7d2do80r.fsf@gitster.g","threadId":"58399","inReplyTo":"de06f80c-4778-7e43-3a88-d7aeae788e0d@gmail.com","subject":"Re: [PATCH] branch: error codes for \"edit_description\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-08T21:45:08Z","receivedAt":"2022-09-08T21:45:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> Stick to the convention of exit with 1 on error. May break some\n> third party tool that expects -1 on \"No branch named 'bla'\" or\n> \"No commit on branch 'bla' yet.\" errors.\n>\n> Test added to avoid regresion on this behaviour.\n> ---\n>\n> Junio, here is the patch for the change you suggested [1]. My concern is\n> this might break some third party tool or script. If no one thinks this\n> is an issue, I can squash it with the patch I'm responding to.\n>\n>\n> [1] https://lore.kernel.org/git/xmqq7d2fszhk.fsf@gitster.g/\n>\n> builtin/branch.c | 6 +++---\n> t/t3200-branch.sh | 8 ++++++++\n> 2 files changed, 11 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index b1f6519cd9..01b6364f58 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -810,11 +810,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> \t\t\telse\n> \t\t\t\tret = error(_(\"No branch named '%s'.\"),\n> \t\t\t\t\t branch_name);\n> -\t\t} else if (edit_branch_description(branch_name))\n> -\t\t\tret = 1;\n> +\t\t} else\n> +\t\t\tret = edit_branch_description(branch_name);\n> \n> \t\tstrbuf_release(&branch_ref);\n> -\t\treturn ret;\n> +\t\treturn -ret;\n\nIf edit_branch_description() returns -1 on failure, this would\nconsistently exit with 1 on failure, so it does make sense. \n\nIt is bad to exit with a value outside 0..255 for portability\nreasons, but we wrap exit() to chomp off the higher bits, so in\npractice it may be fine to exit(-1) but still the above is a good\nclean-up, I would think.\n\n"},{"id":"464408","messageId":"2e164aea-7dd8-5018-474a-01643553ea49@gmail.com","threadId":"58399","inReplyTo":"7abdb5a9-5707-7897-4196-8d2892beeb81@gmail.com","subject":"[PATCH v4] branch: support for shortcuts like @{-1}, completed","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-10-08T01:00:21Z","receivedAt":"2022-10-08T01:01:02Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"branch command with options \"edit-description\", \"set-upstream-to\" and\n\"unset-upstream\" expects a branch name. Since ae5a6c3684 (checkout:\nimplement \"@{-N}\" shortcut name for N-th last branch, 2009-01-17) a\nbranch can be specified using shortcuts like @{-1}. Those shortcuts\nneed to be resolved when considering the arguments.\n\nWe can modify the description of the previously checked out branch with:\n\n$ git branch --edit--description @{-1}\n\nWe can modify the upstream of the previously checked out branch with:\n\n$ git branch --set-upstream-to upstream @{-1}\n$ git branch --unset-upstream @{-1}\n\nSigned-off-by: Rubén Justo \n\n---\n\nTo simplify the patch and make it easier to review, I removed all\nrefactoring related to return codes and errors. Leaving that and\neverything else discussed in the thread, to next series.\n\nThis patch now only has one commit with the missing @{-N} handling\ncompleted and the tests for it. Some refactoring has been done in\nthe test as well.\n\n\nUn saludo.\n\n\n builtin/branch.c | 34 +++++++++++++++++++++------\n t/t3204-branch-name-interpretation.sh | 24 +++++++++++++++++++\n 2 files changed, 51 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 55cd9a6e99..197603241d 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -791,19 +791,23 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t} else if (edit_description) {\n \t\tconst char *branch_name;\n \t\tstruct strbuf branch_ref = STRBUF_INIT;\n+\t\tstruct strbuf buf = STRBUF_INIT;\n \n \t\tif (!argc) {\n \t\t\tif (filter.detached)\n \t\t\t\tdie(_(\"Cannot give description to detached HEAD\"));\n \t\t\tbranch_name = head;\n-\t\t} else if (argc == 1)\n-\t\t\tbranch_name = argv[0];\n+\t\t} else if (argc == 1) {\n+\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n+\t\t\tbranch_name = buf.buf;\n+\t\t}\n \t\telse\n \t\t\tdie(_(\"cannot edit description of more than one branch\"));\n \n \t\tstrbuf_addf(&branch_ref, \"refs/heads/%s\", branch_name);\n \t\tif (!ref_exists(branch_ref.buf)) {\n \t\t\tstrbuf_release(&branch_ref);\n+\t\t\tstrbuf_release(&buf);\n \n \t\t\tif (!argc)\n \t\t\t\treturn error(_(\"No commit on branch '%s' yet.\"),\n@@ -814,8 +818,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tstrbuf_release(&branch_ref);\n \n-\t\tif (edit_branch_description(branch_name))\n+\t\tif (edit_branch_description(branch_name)) {\n+\t\t\tstrbuf_release(&buf);\n \t\t\treturn 1;\n+\t\t}\n+\t\tstrbuf_release(&buf);\n \t} else if (copy) {\n \t\tif (!argc)\n \t\t\tdie(_(\"branch name required\"));\n@@ -835,9 +842,15 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\telse\n \t\t\tdie(_(\"too many arguments for a rename operation\"));\n \t} else if (new_upstream) {\n-\t\tstruct branch *branch = branch_get(argv[0]);\n+\t\tstruct branch *branch;\n+\t\tstruct strbuf buf = STRBUF_INIT;\n \n-\t\tif (argc > 1)\n+\t\tif (!argc)\n+\t\t\tbranch = branch_get(NULL);\n+\t\telse if (argc == 1) {\n+\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n+\t\t\tbranch = branch_get(buf.buf);\n+\t\t} else\n \t\t\tdie(_(\"too many arguments to set new upstream\"));\n \n \t\tif (!branch) {\n@@ -854,11 +867,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tdwim_and_setup_tracking(the_repository, branch->name,\n \t\t\t\t\tnew_upstream, BRANCH_TRACK_OVERRIDE,\n \t\t\t\t\tquiet);\n+\t\tstrbuf_release(&buf);\n \t} else if (unset_upstream) {\n-\t\tstruct branch *branch = branch_get(argv[0]);\n+\t\tstruct branch *branch;\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n-\t\tif (argc > 1)\n+\t\tif (!argc)\n+\t\t\tbranch = branch_get(NULL);\n+\t\telse if (argc == 1) {\n+\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n+\t\t\tbranch = branch_get(buf.buf);\n+\t\t} else\n \t\t\tdie(_(\"too many arguments to unset upstream\"));\n \n \t\tif (!branch) {\n@@ -871,6 +890,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tif (!branch_has_merge_config(branch))\n \t\t\tdie(_(\"Branch '%s' has no upstream information\"), branch->name);\n \n+\t\tstrbuf_reset(&buf);\n \t\tstrbuf_addf(&buf, \"branch.%s.remote\", branch->name);\n \t\tgit_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);\n \t\tstrbuf_reset(&buf);\ndiff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh\nindex 993a6b5eff..71afceac71 100755\n--- a/t/t3204-branch-name-interpretation.sh\n+++ b/t/t3204-branch-name-interpretation.sh\n@@ -133,4 +133,28 @@ test_expect_success 'checkout does not treat remote @{upstream} as a branch' '\n \texpect_branch HEAD one\n '\n \n+test_expect_success 'edit-description via @{-1}' '\n+\tgit checkout -b desc-branch &&\n+\tgit checkout -b non-desc-branch &&\n+\twrite_script editor <<-\\EOF &&\n+\t\techo \"Branch description\" >\"$1\"\n+\tEOF\n+\tEDITOR=./editor git branch --edit-description @{-1} &&\n+\ttest_must_fail git config branch.non-desc-branch.description &&\n+\tgit config branch.desc-branch.description >actual &&\n+\techo \"Branch description\\n\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'modify branch upstream via \"@{-1}\" and \"@{-1}@{upstream}\"' '\n+\tgit checkout -b upstream-branch &&\n+\tgit checkout -b upstream-other -t upstream-branch &&\n+\tgit branch --set-upstream-to upstream-other @{-1} &&\n+\tgit config branch.upstream-branch.merge >actual &&\n+\techo \"refs/heads/upstream-other\" >expect &&\n+\ttest_cmp expect actual &&\n+\tgit branch --unset-upstream @{-1}@{upstream} &&\n+\ttest_must_fail git config branch.upstream-other.merge\n+'\n+\n test_done\n-- \n2.32.0\n"},{"id":"464409","messageId":"CAPig+cSX9jSPc_fJc0tuiER1-AqnbFGOk0r1tXEkv9gycVH-CA@mail.gmail.com","threadId":"58399","inReplyTo":"2e164aea-7dd8-5018-474a-01643553ea49@gmail.com","subject":"Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-10-08T03:17:34Z","receivedAt":"2022-10-08T03:17:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Oct 7, 2022 at 9:36 PM Rubén Justo wrote:\n> branch command with options \"edit-description\", \"set-upstream-to\" and\n> \"unset-upstream\" expects a branch name. Since ae5a6c3684 (checkout:\n> implement \"@{-N}\" shortcut name for N-th last branch, 2009-01-17) a\n> branch can be specified using shortcuts like @{-1}. Those shortcuts\n> need to be resolved when considering the arguments.\n> [...]\n> Signed-off-by: Rubén Justo \n> ---\n> diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh\n> @@ -133,4 +133,28 @@ test_expect_success 'checkout does not treat remote @{upstream} as a branch' '\n> +test_expect_success 'edit-description via @{-1}' '\n> + git checkout -b desc-branch &&\n> + git checkout -b non-desc-branch &&\n> + write_script editor <<-\\EOF &&\n> + echo \"Branch description\" >\"$1\"\n> + EOF\n> + EDITOR=./editor git branch --edit-description @{-1} &&\n> + test_must_fail git config branch.non-desc-branch.description &&\n> + git config branch.desc-branch.description >actual &&\n> + echo \"Branch description\\n\" >expect &&\n\nIs the intention here with the embedded \"\\n\" that `echo` should emit\ntwo newlines? If so, interpreting \"\\n\" specially is not POSIX behavior\nfor `echo`, thus we probably don't want to rely upon it.\n\n> + test_cmp expect actual\n"},{"id":"464413","messageId":"xmqq8rlrc5jp.fsf@gitster.g","threadId":"58399","inReplyTo":"2e164aea-7dd8-5018-474a-01643553ea49@gmail.com","subject":"Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-08T04:17:46Z","receivedAt":"2022-10-08T04:17:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> \t} else if (new_upstream) {\n> -\t\tstruct branch *branch = branch_get(argv[0]);\n> +\t\tstruct branch *branch;\n> +\t\tstruct strbuf buf = STRBUF_INIT;\n> \n> -\t\tif (argc > 1)\n> +\t\tif (!argc)\n> +\t\t\tbranch = branch_get(NULL);\n> +\t\telse if (argc == 1) {\n> +\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n> +\t\t\tbranch = branch_get(buf.buf);\n> +\t\t} else\n> \t\t\tdie(_(\"too many arguments to set new upstream\"));\n> \n> \t\tif (!branch) {\n> @@ -854,11 +867,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> \t\tdwim_and_setup_tracking(the_repository, branch->name,\n> \t\t\t\t\tnew_upstream, BRANCH_TRACK_OVERRIDE,\n> \t\t\t\t\tquiet);\n> +\t\tstrbuf_release(&buf);\n> \t} else if (unset_upstream) {\n> -\t\tstruct branch *branch = branch_get(argv[0]);\n> +\t\tstruct branch *branch;\n> \t\tstruct strbuf buf = STRBUF_INIT;\n> \n> -\t\tif (argc > 1)\n> +\t\tif (!argc)\n> +\t\t\tbranch = branch_get(NULL);\n> +\t\telse if (argc == 1) {\n> +\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n> +\t\t\tbranch = branch_get(buf.buf);\n> +\t\t} else\n> \t\t\tdie(_(\"too many arguments to unset upstream\"));\n\nThe above two are a bit repetitious. A helper like\n\n\tstatic struct branch *interpret_local(int ac, const char **av)\n\t{\n\t\tstruct branch *branch;\n if (!ac) {\n \tbranch = branch_get(NULL);\n\t\t} else if (ac == 1) {\n\t\t\tstruct strbuf buf = STRBUF_INIT;\n\t\t\tstrbuf_branchname(&buf, av[0], INTERPRET_BRANCH_LOCAL);\n\t\t\tbranch = branch_get(buf.buf);\n\t\t\tstrbuf_release(&buf);\n\t\t} else {\n\t\t\tdie(_(\"too many arguments\"));\n\t\t}\n\t\treturn branch;\n\t}\n\nmight be useful, and it frees the callers from worrying about\ntemporary allocations.\n\nBut the code written is OK as-is.\n"},{"id":"464415","messageId":"xmqqzge7aqq2.fsf@gitster.g","threadId":"58399","inReplyTo":"CAPig+cSX9jSPc_fJc0tuiER1-AqnbFGOk0r1tXEkv9gycVH-CA@mail.gmail.com","subject":"Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-08T04:23:17Z","receivedAt":"2022-10-08T04:23:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Fri, Oct 7, 2022 at 9:36 PM Rubén Justo wrote:\n>> branch command with options \"edit-description\", \"set-upstream-to\" and\n>> \"unset-upstream\" expects a branch name. Since ae5a6c3684 (checkout:\n>> implement \"@{-N}\" shortcut name for N-th last branch, 2009-01-17) a\n>> branch can be specified using shortcuts like @{-1}. Those shortcuts\n>> need to be resolved when considering the arguments.\n>> [...]\n>> Signed-off-by: Rubén Justo \n>> ---\n>> diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh\n>> @@ -133,4 +133,28 @@ test_expect_success 'checkout does not treat remote @{upstream} as a branch' '\n>> +test_expect_success 'edit-description via @{-1}' '\n>> + git checkout -b desc-branch &&\n>> + git checkout -b non-desc-branch &&\n>> + write_script editor <<-\\EOF &&\n>> + echo \"Branch description\" >\"$1\"\n>> + EOF\n>> + EDITOR=./editor git branch --edit-description @{-1} &&\n>> + test_must_fail git config branch.non-desc-branch.description &&\n>> + git config branch.desc-branch.description >actual &&\n>> + echo \"Branch description\\n\" >expect &&\n>\n> Is the intention here with the embedded \"\\n\" that `echo` should emit\n> two newlines? If so, interpreting \"\\n\" specially is not POSIX behavior\n> for `echo`, thus we probably don't want to rely upon it.\n\nGood eyes. You are correct that \"echo\" and \"\\n\" do not play well\ntogether.\n\nThanks.\n"},{"id":"464419","messageId":"f24837e9-7873-c34c-bd78-8ae3be0fc97a@gmail.com","threadId":"58399","inReplyTo":"CAPig+cSX9jSPc_fJc0tuiER1-AqnbFGOk0r1tXEkv9gycVH-CA@mail.gmail.com","subject":"Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-10-08T07:07:33Z","receivedAt":"2022-10-08T07:07:46Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"\nOn 8/10/22 5:17, Eric Sunshine wrote:\n> On Fri, Oct 7, 2022 at 9:36 PM Rubén Justo wrote:\n>> branch command with options \"edit-description\", \"set-upstream-to\" and\n>> \"unset-upstream\" expects a branch name. Since ae5a6c3684 (checkout:\n>> implement \"@{-N}\" shortcut name for N-th last branch, 2009-01-17) a\n>> branch can be specified using shortcuts like @{-1}. Those shortcuts\n>> need to be resolved when considering the arguments.\n>> [...]\n>> Signed-off-by: Rubén Justo \n>> ---\n>> diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh\n>> @@ -133,4 +133,28 @@ test_expect_success 'checkout does not treat remote @{upstream} as a branch' '\n>> +test_expect_success 'edit-description via @{-1}' '\n>> + git checkout -b desc-branch &&\n>> + git checkout -b non-desc-branch &&\n>> + write_script editor <<-\\EOF &&\n>> + echo \"Branch description\" >\"$1\"\n>> + EOF\n>> + EDITOR=./editor git branch --edit-description @{-1} &&\n>> + test_must_fail git config branch.non-desc-branch.description &&\n>> + git config branch.desc-branch.description >actual &&\n>> + echo \"Branch description\\n\" >expect &&\n> \n> Is the intention here with the embedded \"\\n\" that `echo` should emit\n> two newlines? If so, interpreting \"\\n\" specially is not POSIX behavior\n> for `echo`, thus we probably don't want to rely upon it.\n> \n\nOops. Thank you! I'll reroll back to using \"git stripspace\".\n\n>> + test_cmp expect actual\n"},{"id":"464420","messageId":"CAPig+cSn29Fq4ywC9zXoJYRVG8KUEhHuDdwEUSioFMUVs+S-ow@mail.gmail.com","threadId":"58399","inReplyTo":"f24837e9-7873-c34c-bd78-8ae3be0fc97a@gmail.com","subject":"Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-10-08T07:23:39Z","receivedAt":"2022-10-08T07:23:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Oct 8, 2022 at 3:07 AM Rubén Justo wrote:\n> On 8/10/22 5:17, Eric Sunshine wrote:\n> > On Fri, Oct 7, 2022 at 9:36 PM Rubén Justo wrote:\n> >> + echo \"Branch description\\n\" >expect &&\n> >\n> > Is the intention here with the embedded \"\\n\" that `echo` should emit\n> > two newlines? If so, interpreting \"\\n\" specially is not POSIX behavior\n> > for `echo`, thus we probably don't want to rely upon it.\n>\n> Oops. Thank you! I'll reroll back to using \"git stripspace\".\n\n`git stripspace` is perhaps unnecessarily heavyweight. Lightweight\nalternatives would include:\n\n printf \"Branch description\\n\\n\" >expect &&\n\n test_write_lines \"Branch description\" \"\" >expect &&\n\n { echo \"Branch description\" && echo; } >expect &&\n\n cat >expect <<-\\EOF &&\n Branch description\n\n EOF\n"},{"id":"464422","messageId":"32c2fe9a-51c4-493c-5600-ef77f8818b64@gmail.com","threadId":"58399","inReplyTo":"xmqq8rlrc5jp.fsf@gitster.g","subject":"Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-10-08T09:04:38Z","receivedAt":"2022-10-08T09:04:45Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 8/10/22 6:17, Junio C Hamano wrote:\n> Rubén Justo writes:\n> \n>> \t} else if (new_upstream) {\n>> -\t\tstruct branch *branch = branch_get(argv[0]);\n>> +\t\tstruct branch *branch;\n>> +\t\tstruct strbuf buf = STRBUF_INIT;\n>> \n>> -\t\tif (argc > 1)\n>> +\t\tif (!argc)\n>> +\t\t\tbranch = branch_get(NULL);\n>> +\t\telse if (argc == 1) {\n>> +\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n>> +\t\t\tbranch = branch_get(buf.buf);\n>> +\t\t} else\n>> \t\t\tdie(_(\"too many arguments to set new upstream\"));\n>> \n>> \t\tif (!branch) {\n>> @@ -854,11 +867,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n>> \t\tdwim_and_setup_tracking(the_repository, branch->name,\n>> \t\t\t\t\tnew_upstream, BRANCH_TRACK_OVERRIDE,\n>> \t\t\t\t\tquiet);\n>> +\t\tstrbuf_release(&buf);\n>> \t} else if (unset_upstream) {\n>> -\t\tstruct branch *branch = branch_get(argv[0]);\n>> +\t\tstruct branch *branch;\n>> \t\tstruct strbuf buf = STRBUF_INIT;\n>> \n>> -\t\tif (argc > 1)\n>> +\t\tif (!argc)\n>> +\t\t\tbranch = branch_get(NULL);\n>> +\t\telse if (argc == 1) {\n>> +\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n>> +\t\t\tbranch = branch_get(buf.buf);\n>> +\t\t} else\n>> \t\t\tdie(_(\"too many arguments to unset upstream\"));\n> \n> The above two are a bit repetitious. A helper like\n> \n> \tstatic struct branch *interpret_local(int ac, const char **av)\n> \t{\n> \t\tstruct branch *branch;\n> if (!ac) {\n> \tbranch = branch_get(NULL);\n> \t\t} else if (ac == 1) {\n> \t\t\tstruct strbuf buf = STRBUF_INIT;\n> \t\t\tstrbuf_branchname(&buf, av[0], INTERPRET_BRANCH_LOCAL);\n> \t\t\tbranch = branch_get(buf.buf);\n> \t\t\tstrbuf_release(&buf);\n> \t\t} else {\n> \t\t\tdie(_(\"too many arguments\"));\n> \t\t}\n> \t\treturn branch;\n> \t}\n> \n> might be useful, and it frees the callers from worrying about\n> temporary allocations.\n\nYes, it leaves a repetitive taste. I thought about joining the two cases, but\nthe taste with multiple if/else was worse. Following what you suggest with the\n\"too many arguments\" error, I'll try to reduce that repetitive taste.\n\nThanks\n\n> \n> But the code written is OK as-is.\n> \n"},{"id":"464423","messageId":"a7aca891-dd37-7e5e-61fc-8012fec18ae9@gmail.com","threadId":"58399","inReplyTo":"CAPig+cSn29Fq4ywC9zXoJYRVG8KUEhHuDdwEUSioFMUVs+S-ow@mail.gmail.com","subject":"Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-10-08T09:12:23Z","receivedAt":"2022-10-08T09:12:29Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 8/10/22 9:23, Eric Sunshine wrote:\n> On Sat, Oct 8, 2022 at 3:07 AM Rubén Justo wrote:\n>> On 8/10/22 5:17, Eric Sunshine wrote:\n>>> On Fri, Oct 7, 2022 at 9:36 PM Rubén Justo wrote:\n>>>> + echo \"Branch description\\n\" >expect &&\n>>>\n>>> Is the intention here with the embedded \"\\n\" that `echo` should emit\n>>> two newlines? If so, interpreting \"\\n\" specially is not POSIX behavior\n>>> for `echo`, thus we probably don't want to rely upon it.\n>>\n>> Oops. Thank you! I'll reroll back to using \"git stripspace\".\n> \n> `git stripspace` is perhaps unnecessarily heavyweight. Lightweight\n> alternatives would include:\n> \n> printf \"Branch description\\n\\n\" >expect &&\n> \n> test_write_lines \"Branch description\" \"\" >expect &&\n> \n> { echo \"Branch description\" && echo; } >expect &&\n> \n> cat >expect <<-\\EOF &&\n> Branch description\n> \n> EOF\n> \n\nYeah, I thought about that. What convinced me to use \"git stripspace\" was\nthat maybe that '\\n' tail could be removed sometime from the description\nsetting and this will be fine with that. I haven't found any reason for\nthat '\\n' and it bugs me a little seeing it in the config :-)\n\nBut I agree with you about the unnecessarily heavyweight, though all\ninvolves a new process, probably echo, cat or printf are lightweight than\nanother instance of git for that.\n\nAll of this involves two files and that is how it is done almost everywhere\nexcept in some places where it looks like an 'older way' (test_i18ngrep) of\ndoing it. Is there any reason to do it this way and not using variables,\nprocess substitution,..?\n\nAnyway I'll switch to one of your suggestions, as it is definitely easier\nto read, understand and therefore change if needed.\n\nThanks!\n"},{"id":"464430","messageId":"CAPig+cRxy5C+CqUOzmhe16j+hssxsygha3huVga8tLJ+imM4Hw@mail.gmail.com","threadId":"58399","inReplyTo":"a7aca891-dd37-7e5e-61fc-8012fec18ae9@gmail.com","subject":"Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-10-08T17:10:59Z","receivedAt":"2022-10-08T17:11:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Oct 8, 2022 at 5:12 AM Rubén Justo wrote:\n> On 8/10/22 9:23, Eric Sunshine wrote:\n> > On Sat, Oct 8, 2022 at 3:07 AM Rubén Justo wrote:\n> >> Oops. Thank you! I'll reroll back to using \"git stripspace\".\n> >\n> > `git stripspace` is perhaps unnecessarily heavyweight. Lightweight\n> > alternatives would include:\n> >\n> > printf \"Branch description\\n\\n\" >expect &&\n> > [...]\n>\n> Yeah, I thought about that. What convinced me to use \"git stripspace\" was\n> that maybe that '\\n' tail could be removed sometime from the description\n> setting and this will be fine with that. I haven't found any reason for\n> that '\\n' and it bugs me a little seeing it in the config :-)\n\nThat reasoning occurred to me, as well, and I'd have no objection to\ngit-stripspace if that's the motivation for using it. I don't feel\nstrongly one way or the other, and my previous email was intended\nprimarily to point out the lightweight alternatives in case you hadn't\nconsidered them. Feel free to use git-stripspace if you feel it is the\nmore appropriate choice.\n\n> But I agree with you about the unnecessarily heavyweight, though all\n> involves a new process, probably echo, cat or printf are lightweight than\n> another instance of git for that.\n\nIn most shells, `printf`, `echo`, `cat` are builtins, so no extra\nprocesses are involved (and `test_write_lines` is a shell function\nbuilt atop `printf`). As a matter of personal preference, given the\nlightweight options, I find that `printf \"...\\n\\n\"` shows the\nintention of the code most plainly (but if you go with git-stripspace,\nthen `echo` would be an idiomatic way to create the \"expect\" file).\n\n> All of this involves two files and that is how it is done almost everywhere\n> except in some places where it looks like an 'older way' (test_i18ngrep) of\n> doing it. Is there any reason to do it this way and not using variables,\n> process substitution,..?\n\nAn invocation such as:\n\n test $(git foo) = $(git bar) &&\n\nthrows away the exit-code from the two commands, which means we'd miss\nif one or the other (or both) crashed, especially if the crash was\nafter the command produced the correct output. These days we try to\navoid losing the exit command of Git commands. It's possible to avoid\nlosing the exit-code by using variables:\n\n expect=$(git foo) &&\n actual=$(git bar) &&\n test \"$expect\" = \"$actual\" &&\n\nbut, if the expected and actual output don't match, you don't learn\nmuch (other than that they failed). You could address that by showing\na message saying what failed:\n\n expect=... &&\n actual=... &&\n if test \"$expect\" != \"$actual\"\n then\n echo \"expect not match actual\"\n # maybe emit $expect and $actual too\n fi\n\nHowever, `test_cmp` gives you that behavior for free, and it emits a\nhelpful \"diff\" upon failure, so these days we usually go with\n`test_cmp`.\n\n> Anyway I'll switch to one of your suggestions, as it is definitely easier\n> to read, understand and therefore change if needed.\n\nIt's fine to use git-stripspace if you feel it's more appropriate for\nthe situation.\n"},{"id":"464431","messageId":"xmqq1qrib435.fsf@gitster.g","threadId":"58399","inReplyTo":"CAPig+cRxy5C+CqUOzmhe16j+hssxsygha3huVga8tLJ+imM4Hw@mail.gmail.com","subject":"Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-08T17:46:54Z","receivedAt":"2022-10-08T17:47:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> Yeah, I thought about that. What convinced me to use \"git stripspace\" was\n>> that maybe that '\\n' tail could be removed sometime from the description\n>> setting and this will be fine with that. I haven't found any reason for\n>> that '\\n' and it bugs me a little seeing it in the config :-)\n>\n> That reasoning occurred to me, as well, and I'd have no objection to\n> git-stripspace if that's the motivation for using it. I don't feel\n> strongly one way or the other, and my previous email was intended\n> primarily to point out the lightweight alternatives in case you hadn't\n> considered them. Feel free to use git-stripspace if you feel it is the\n> more appropriate choice.\n\nI do not think I would agree with the line of reasoning.\n\nIt all depends on why we anticipate that the terminating LF may go\naway someday, but if it is because we may do so by mistake and\nwithout a good reason when making some unrelated changes to the\nimplementation of \"git branch --edit-desc\", we would want to know\nabout it, and such a loose check that would miss it is definitely\nunwelcome. It is very likely that not just \"git merge\" but people's\nexternal scripts depend on the presence of final LF especially when\nthe description has only one line, so unless we are doing\ndeliberately so, we should prepare to catch such a change.\n\nIf it is because we may gain a consensus that the description string\n(which by the way can well consist of multiple lines) is better\nwithout the LF on its final line, and we are \"fixing\" the code to do\nso very much on purpose, it would be good to have a test to protect\nsuch a change from future unintended breakages. Adding a loose test\nthat won't break across such a change today may be OK, but whoever\nis making such a change in the future has to make sure there is a\ntest that is not loose to protect the change. And it would very\nlikely to be done by adding a new test, instead of noticing that\nthis loosely written test can be tightened to serve the purpose.\n\nSo if we start with a tight test that expects the exact number of\nLFs at the end, we would be better off in that case, too.\n"},{"id":"464432","messageId":"48fa9be6-2e55-e2e1-d1ad-7895811bfca7@gmail.com","threadId":"58399","inReplyTo":"xmqq1qrib435.fsf@gitster.g","subject":"Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-10-08T20:48:43Z","receivedAt":"2022-10-08T20:48:51Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"\nOn 8/10/22 19:46, Junio C Hamano wrote:\n> Eric Sunshine writes:\n> \n>>> Yeah, I thought about that. What convinced me to use \"git stripspace\" was\n>>> that maybe that '\\n' tail could be removed sometime from the description\n>>> setting and this will be fine with that. I haven't found any reason for\n>>> that '\\n' and it bugs me a little seeing it in the config :-)\n>>\n>> That reasoning occurred to me, as well, and I'd have no objection to\n>> git-stripspace if that's the motivation for using it. I don't feel\n>> strongly one way or the other, and my previous email was intended\n>> primarily to point out the lightweight alternatives in case you hadn't\n>> considered them. Feel free to use git-stripspace if you feel it is the\n>> more appropriate choice.\n> \n> I do not think I would agree with the line of reasoning.\n> \n> It all depends on why we anticipate that the terminating LF may go\n> away someday, but if it is because we may do so by mistake and\n> without a good reason when making some unrelated changes to the\n> implementation of \"git branch --edit-desc\", we would want to know\n> about it, and such a loose check that would miss it is definitely\n> unwelcome. It is very likely that not just \"git merge\" but people's\n> external scripts depend on the presence of final LF especially when\n> the description has only one line, so unless we are doing\n> deliberately so, we should prepare to catch such a change.\n> \n> If it is because we may gain a consensus that the description string\n> (which by the way can well consist of multiple lines) is better\n> without the LF on its final line, and we are \"fixing\" the code to do\n> so very much on purpose, it would be good to have a test to protect\n> such a change from future unintended breakages. Adding a loose test\n> that won't break across such a change today may be OK, but whoever\n> is making such a change in the future has to make sure there is a\n> test that is not loose to protect the change. And it would very\n> likely to be done by adding a new test, instead of noticing that\n> this loosely written test can be tightened to serve the purpose.\n> \n> So if we start with a tight test that expects the exact number of\n> LFs at the end, we would be better off in that case, too.\n> \n\nFair point. Thank you for being cautious.\n"},{"id":"464433","messageId":"de200fa0-379d-c1ce-8446-9e4292d0b66a@gmail.com","threadId":"58399","inReplyTo":"2e164aea-7dd8-5018-474a-01643553ea49@gmail.com","subject":"[PATCH v5] branch: support for shortcuts like @{-1}, completed","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-10-08T22:32:16Z","receivedAt":"2022-10-08T22:32:27Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"branch command with options \"edit-description\", \"set-upstream-to\" and\n\"unset-upstream\" expects a branch name. Since ae5a6c3684 (checkout:\nimplement \"@{-N}\" shortcut name for N-th last branch, 2009-01-17) a\nbranch can be specified using shortcuts like @{-1}. Those shortcuts\nneed to be resolved when considering the arguments.\n\nWe can modify the description of the previously checked out branch with:\n\n$ git branch --edit--description @{-1}\n\nWe can modify the upstream of the previously checked out branch with:\n\n$ git branch --set-upstream-to upstream @{-1}\n$ git branch --unset-upstream @{-1}\n\nSigned-off-by: Rubén Justo \n---\n\nChanges from v4:\n\t- Fix non POSIX compatible usage of echo.\n\nJunio, I have taken two approaches to reduce the repetitive code, merging the\ntwo cases \"new_upstream\" and \"unset_upstream\". First keeping the different\nerror messages to each case, which leads to multiple ugly if/elses. Second\napproach, merging the error messages with generics, which resolves the if/elses\nbut leads to changes in t/t3200-branch.sh (at least). I haven't finished\nthe tests yet, but I already have the same feeling as with the other patch for\nbranch. I think this is a good stop point for this change if no new problems\nare detected.\n\nUn saludo.\n\n\nRange-diff:\n1: 72a17039e0 ! 1: 393250bbac branch: support for shortcuts like @{-1}, completed\n @@ t/t3204-branch-name-interpretation.sh: test_expect_success 'checkout does not tr\n +\tEDITOR=./editor git branch --edit-description @{-1} &&\n +\ttest_must_fail git config branch.non-desc-branch.description &&\n +\tgit config branch.desc-branch.description >actual &&\n -+\techo \"Branch description\\n\" >expect &&\n ++\tprintf \"Branch description\\n\\n\" >expect &&\n +\ttest_cmp expect actual\n +'\n +\n\n builtin/branch.c | 34 +++++++++++++++++++++------\n t/t3204-branch-name-interpretation.sh | 24 +++++++++++++++++++\n 2 files changed, 51 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 55cd9a6e99..197603241d 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -791,19 +791,23 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t} else if (edit_description) {\n \t\tconst char *branch_name;\n \t\tstruct strbuf branch_ref = STRBUF_INIT;\n+\t\tstruct strbuf buf = STRBUF_INIT;\n \n \t\tif (!argc) {\n \t\t\tif (filter.detached)\n \t\t\t\tdie(_(\"Cannot give description to detached HEAD\"));\n \t\t\tbranch_name = head;\n-\t\t} else if (argc == 1)\n-\t\t\tbranch_name = argv[0];\n+\t\t} else if (argc == 1) {\n+\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n+\t\t\tbranch_name = buf.buf;\n+\t\t}\n \t\telse\n \t\t\tdie(_(\"cannot edit description of more than one branch\"));\n \n \t\tstrbuf_addf(&branch_ref, \"refs/heads/%s\", branch_name);\n \t\tif (!ref_exists(branch_ref.buf)) {\n \t\t\tstrbuf_release(&branch_ref);\n+\t\t\tstrbuf_release(&buf);\n \n \t\t\tif (!argc)\n \t\t\t\treturn error(_(\"No commit on branch '%s' yet.\"),\n@@ -814,8 +818,11 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tstrbuf_release(&branch_ref);\n \n-\t\tif (edit_branch_description(branch_name))\n+\t\tif (edit_branch_description(branch_name)) {\n+\t\t\tstrbuf_release(&buf);\n \t\t\treturn 1;\n+\t\t}\n+\t\tstrbuf_release(&buf);\n \t} else if (copy) {\n \t\tif (!argc)\n \t\t\tdie(_(\"branch name required\"));\n@@ -835,9 +842,15 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\telse\n \t\t\tdie(_(\"too many arguments for a rename operation\"));\n \t} else if (new_upstream) {\n-\t\tstruct branch *branch = branch_get(argv[0]);\n+\t\tstruct branch *branch;\n+\t\tstruct strbuf buf = STRBUF_INIT;\n \n-\t\tif (argc > 1)\n+\t\tif (!argc)\n+\t\t\tbranch = branch_get(NULL);\n+\t\telse if (argc == 1) {\n+\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n+\t\t\tbranch = branch_get(buf.buf);\n+\t\t} else\n \t\t\tdie(_(\"too many arguments to set new upstream\"));\n \n \t\tif (!branch) {\n@@ -854,11 +867,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tdwim_and_setup_tracking(the_repository, branch->name,\n \t\t\t\t\tnew_upstream, BRANCH_TRACK_OVERRIDE,\n \t\t\t\t\tquiet);\n+\t\tstrbuf_release(&buf);\n \t} else if (unset_upstream) {\n-\t\tstruct branch *branch = branch_get(argv[0]);\n+\t\tstruct branch *branch;\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n-\t\tif (argc > 1)\n+\t\tif (!argc)\n+\t\t\tbranch = branch_get(NULL);\n+\t\telse if (argc == 1) {\n+\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n+\t\t\tbranch = branch_get(buf.buf);\n+\t\t} else\n \t\t\tdie(_(\"too many arguments to unset upstream\"));\n \n \t\tif (!branch) {\n@@ -871,6 +890,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tif (!branch_has_merge_config(branch))\n \t\t\tdie(_(\"Branch '%s' has no upstream information\"), branch->name);\n \n+\t\tstrbuf_reset(&buf);\n \t\tstrbuf_addf(&buf, \"branch.%s.remote\", branch->name);\n \t\tgit_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);\n \t\tstrbuf_reset(&buf);\ndiff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh\nindex 993a6b5eff..793bf4d269 100755\n--- a/t/t3204-branch-name-interpretation.sh\n+++ b/t/t3204-branch-name-interpretation.sh\n@@ -133,4 +133,28 @@ test_expect_success 'checkout does not treat remote @{upstream} as a branch' '\n \texpect_branch HEAD one\n '\n \n+test_expect_success 'edit-description via @{-1}' '\n+\tgit checkout -b desc-branch &&\n+\tgit checkout -b non-desc-branch &&\n+\twrite_script editor <<-\\EOF &&\n+\t\techo \"Branch description\" >\"$1\"\n+\tEOF\n+\tEDITOR=./editor git branch --edit-description @{-1} &&\n+\ttest_must_fail git config branch.non-desc-branch.description &&\n+\tgit config branch.desc-branch.description >actual &&\n+\tprintf \"Branch description\\n\\n\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'modify branch upstream via \"@{-1}\" and \"@{-1}@{upstream}\"' '\n+\tgit checkout -b upstream-branch &&\n+\tgit checkout -b upstream-other -t upstream-branch &&\n+\tgit branch --set-upstream-to upstream-other @{-1} &&\n+\tgit config branch.upstream-branch.merge >actual &&\n+\techo \"refs/heads/upstream-other\" >expect &&\n+\ttest_cmp expect actual &&\n+\tgit branch --unset-upstream @{-1}@{upstream} &&\n+\ttest_must_fail git config branch.upstream-other.merge\n+'\n+\n test_done\n-- \n2.36.1\n"},{"id":"464435","messageId":"32e4f8cf-696c-317d-236b-0f64c1398a01@gmail.com","threadId":"58399","inReplyTo":"CAPig+cRxy5C+CqUOzmhe16j+hssxsygha3huVga8tLJ+imM4Hw@mail.gmail.com","subject":"Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-10-08T23:28:51Z","receivedAt":"2022-10-08T23:28:58Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"\nOn 8/10/22 19:10, Eric Sunshine wrote:\n\n>> All of this involves two files and that is how it is done almost everywhere\n>> except in some places where it looks like an 'older way' (test_i18ngrep) of\n>> doing it. Is there any reason to do it this way and not using variables,\n>> process substitution,..?\n> \n> An invocation such as:\n> \n> test $(git foo) = $(git bar) &&\n> \n> throws away the exit-code from the two commands, which means we'd miss\n> if one or the other (or both) crashed, especially if the crash was\n> after the command produced the correct output. These days we try to\n> avoid losing the exit command of Git commands. It's possible to avoid\n> losing the exit-code by using variables:\n> \n> expect=$(git foo) &&\n> actual=$(git bar) &&\n> test \"$expect\" = \"$actual\" &&\n> \n> but, if the expected and actual output don't match, you don't learn\n> much (other than that they failed). You could address that by showing\n> a message saying what failed:\n> \n> expect=... &&\n> actual=... &&\n> if test \"$expect\" != \"$actual\"\n> then\n> echo \"expect not match actual\"\n> # maybe emit $expect and $actual too\n> fi\n> \n> However, `test_cmp` gives you that behavior for free, and it emits a\n> helpful \"diff\" upon failure, so these days we usually go with\n> `test_cmp`.\n>\n\nThis is already out of the subject, the patch, of this thread, so sorry,\nbut the context is worth of it.\n\nI understand the possibility of losing exit codes or segfaults in\nsubcommands or pipes, but I'm more thinking in the element to compare to.\nHaving something like:\n \n\ttest_cmp_str () {\n\t\ttest -f \"$1\" || BUG \"first argument must be a file\"\n\t\tif test \"$#\" -gt 1\n\t\tthen\n\t\t\tlocal F=$1\n\t\t\tshift\n\t\t\tprintf \"$@\" | eval \"$GIT_TEST_CMP\" '\"$F\"' -\n\t\telse\n\t\t\teval \"$GIT_TEST_CMP\" '\"$1\"' -\n\t\tfi\n\t}\n\nto allow writing simpler tests like:\n\n--- a/t/t3204-branch-name-interpretation.sh\n+++ b/t/t3204-branch-name-interpretation.sh\n@@ -142,8 +142,7 @@ test_expect_success 'edit-description via @{-1}' '\n \tEDITOR=./editor git branch --edit-description @{-1} &&\n \ttest_must_fail git config branch.non-desc-branch.description &&\n \tgit config branch.desc-branch.description >actual &&\n-\tprintf \"Branch description\\n\\n\" >expect &&\n-\ttest_cmp expect actual\n+\ttest_cmp_str actual \"Branch description\\n\\n\"\n '\n\n--- a/t/t3204-branch-name-interpretation.sh\n+++ b/t/t3204-branch-name-interpretation.sh\n@@ -142,8 +142,10 @@ test_expect_success 'edit-description via @{-1}' '\n EDITOR=./editor git branch --edit-description @{-1} &&\n test_must_fail git config branch.non-desc-branch.description &&\n git config branch.desc-branch.description >actual &&\n- printf \"Branch description\\n\\n\" >expect &&\n- test_cmp expect actual\n+ test_cmp_str actual <<-\\EOF\n+ Branch description\n+\n+ EOF\n '\n\nMy doubts are that maybe this can induce to some bad usages, is\nunusable in some systems, it has already been explored and discarded,\nusing files gives the diff with nice names not \"-\",...\n\nMaybe this needs a new RFC thread. I dunno.\n\nThanks for your comments and explanations.\n"},{"id":"464440","messageId":"xmqqlepp8smc.fsf@gitster.g","threadId":"58399","inReplyTo":"de200fa0-379d-c1ce-8446-9e4292d0b66a@gmail.com","subject":"Re: [PATCH v5] branch: support for shortcuts like @{-1}, completed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-09T05:37:31Z","receivedAt":"2022-10-09T05:37:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> ... I think this is a good stop point for this change if no new problems\n> are detected.\n>\n> Un saludo.\n\nLooks good.\n\nThanks.\n"},{"id":"464441","messageId":"CAPig+cSdQsK7xVBb=ooOfP-hq1zavSV++iorLoni6o6etoLjzQ@mail.gmail.com","threadId":"58399","inReplyTo":"32e4f8cf-696c-317d-236b-0f64c1398a01@gmail.com","subject":"Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-10-09T06:46:30Z","receivedAt":"2022-10-09T06:46:57Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Oct 8, 2022 at 7:28 PM Rubén Justo wrote:\n> On 8/10/22 19:10, Eric Sunshine wrote:\n> > However, `test_cmp` gives you that behavior for free, and it emits a\n> > helpful \"diff\" upon failure, so these days we usually go with\n> > `test_cmp`.\n>\n> I understand the possibility of losing exit codes or segfaults in\n> subcommands or pipes, but I'm more thinking in the element to compare to.\n> Having something like:\n>\n> test_cmp_str () {\n> test -f \"$1\" || BUG \"first argument must be a file\"\n> if test \"$#\" -gt 1\n> then\n> local F=$1\n> shift\n> printf \"$@\" | eval \"$GIT_TEST_CMP\" '\"$F\"' -\n> else\n> eval \"$GIT_TEST_CMP\" '\"$1\"' -\n> fi\n> }\n>\n> to allow writing simpler tests like:\n>\n> + test_cmp_str actual \"Branch description\\n\\n\"\n>\n> + test_cmp_str actual <<-\\EOF\n> + Branch description\n> +\n> + EOF\n>\n> My doubts are that maybe this can induce to some bad usages, is\n> unusable in some systems, it has already been explored and discarded,\n> using files gives the diff with nice names not \"-\",...\n\nI wouldn't be at all surprised if this sort of thing has been\ndiscussed already (it sounds familiar enough), or that such an\nimprovement has already been submitted in the form of a patch\n(possibly by Ævar, Cc:'d).\n\nMy knee-jerk reaction is that the form which takes a string as\nargument would hardly be used, thus an unnecessary complication. The\nform which accepts stdin could even be retrofitted onto the existing\ntest_cmp, thus you don't even need to invent a new function name. A\ndifferent approach would be to introduce a function `test_stdout`\nwhich both accepts a command to run, as well as the \"expected\" text on\nstdin with which to compare the output of the command; i.e. a\ncombination of the existing `test_stdout_line_count` in\nt/test-lib-functions.sh and your proposed function above.\n\nThat said, I'm not yet seeing all that much added value in such a\nfunction; at most, it seems to save only a single line of code, and\nit's not as if the code it's replacing was doing anything complicated\nor hard to grok. So, I'm pretty ambivalent about it. But, of course,\nI'm only one person expressing a knee-jerk reaction. Submitting the\nidea as an RFC patch might generate more feedback.\n"},{"id":"464458","messageId":"xmqqa6647qx2.fsf@gitster.g","threadId":"58399","inReplyTo":"de200fa0-379d-c1ce-8446-9e4292d0b66a@gmail.com","subject":"Re: [PATCH v5] branch: support for shortcuts like @{-1}, completed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-09T19:11:53Z","receivedAt":"2022-10-09T19:11:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> @@ -791,19 +791,23 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> \t} else if (edit_description) {\n> \t\tconst char *branch_name;\n> \t\tstruct strbuf branch_ref = STRBUF_INIT;\n> +\t\tstruct strbuf buf = STRBUF_INIT;\n> \n> \t\tif (!argc) {\n> \t\t\tif (filter.detached)\n> \t\t\t\tdie(_(\"Cannot give description to detached HEAD\"));\n> \t\t\tbranch_name = head;\n> -\t\t} else if (argc == 1)\n> -\t\t\tbranch_name = argv[0];\n> +\t\t} else if (argc == 1) {\n> +\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n> +\t\t\tbranch_name = buf.buf;\n> +\t\t}\n> \t\telse\n> \t\t\tdie(_(\"cannot edit description of more than one branch\"));\n\nHere branch_name could be pointing at buf.buf (or head).\n\n> \t\tstrbuf_addf(&branch_ref, \"refs/heads/%s\", branch_name);\n> \t\tif (!ref_exists(branch_ref.buf)) {\n> \t\t\tstrbuf_release(&branch_ref);\n> +\t\t\tstrbuf_release(&buf);\n\nBut the contents of the buf becomes invalid at this point.\n\n> \t\t\tif (!argc)\n> \t\t\t\treturn error(_(\"No commit on branch '%s' yet.\"),\n\nIn the post context of this hunk, we see that the '%s' above is\nfilled with branch_name, accessing the potentially invalid contents.\n\nI'll put a squashable band-aid on top of the topic for now.\n\n--- >8 ---\n\n * cmd_foo() should not return an negative value.\n\n * branch_name used in the calls to error() could point at buf.buf\n that holds the expansion of @{-1}, but buf was released way too\n early, leading to a use-after-free.\n\n * Style: if/else if/else cascade whose one arm has multiple\n statements and requires {braces} around it should have {braces}\n around all of its arms.\n\n * each arm in the top-level if/else if/else cascade for \"git\n branch\" subcommands were more or less independent, and there\n wasn't anything common that they need to execute after exiting\n the cascade. Unconditionally returning from the arm for the\n edit-description subcommand seems to make the logic flow easier\n to read.\n\n builtin/branch.c | 31 +++++++++++++------------------\n 1 file changed, 13 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 197603241d..17853225fa 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -792,6 +792,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tconst char *branch_name;\n \t\tstruct strbuf branch_ref = STRBUF_INIT;\n \t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tint ret = 1; /* assume failure */\n \n \t\tif (!argc) {\n \t\t\tif (filter.detached)\n@@ -800,29 +801,23 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\t} else if (argc == 1) {\n \t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n \t\t\tbranch_name = buf.buf;\n-\t\t}\n-\t\telse\n+\t\t} else {\n \t\t\tdie(_(\"cannot edit description of more than one branch\"));\n+\t\t}\n \n \t\tstrbuf_addf(&branch_ref, \"refs/heads/%s\", branch_name);\n-\t\tif (!ref_exists(branch_ref.buf)) {\n-\t\t\tstrbuf_release(&branch_ref);\n-\t\t\tstrbuf_release(&buf);\n-\n-\t\t\tif (!argc)\n-\t\t\t\treturn error(_(\"No commit on branch '%s' yet.\"),\n-\t\t\t\t\t branch_name);\n-\t\t\telse\n-\t\t\t\treturn error(_(\"No branch named '%s'.\"),\n-\t\t\t\t\t branch_name);\n-\t\t}\n-\t\tstrbuf_release(&branch_ref);\n+\t\tif (!ref_exists(branch_ref.buf))\n+\t\t\terror(!argc\n+\t\t\t ? _(\"No commit on branch '%s' yet.\")\n+\t\t\t : _(\"No branch named '%s'.\"),\n+\t\t\t branch_name);\n+\t\telse if (!edit_branch_description(branch_name))\n+\t\t\tret = 0; /* happy */\n \n-\t\tif (edit_branch_description(branch_name)) {\n-\t\t\tstrbuf_release(&buf);\n-\t\t\treturn 1;\n-\t\t}\n+\t\tstrbuf_release(&branch_ref);\n \t\tstrbuf_release(&buf);\n+\n+\t\treturn ret;\n \t} else if (copy) {\n \t\tif (!argc)\n \t\t\tdie(_(\"branch name required\"));\n-- \n2.38.0-167-g3030fd6006\n\n"},{"id":"464459","messageId":"xmqq5ygs7pxg.fsf@gitster.g","threadId":"58399","inReplyTo":"CAPig+cSdQsK7xVBb=ooOfP-hq1zavSV++iorLoni6o6etoLjzQ@mail.gmail.com","subject":"Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-09T19:33:15Z","receivedAt":"2022-10-09T19:33:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> That said, I'm not yet seeing all that much added value in such a\n> function; at most, it seems to save only a single line of code, and\n> it's not as if the code it's replacing was doing anything complicated\n> or hard to grok.\n\nI share the sentiment. Leaving the result that was used in comparison\nin file(s) also helps debugging.\n"},{"id":"464460","messageId":"dacb1f40-2765-5978-1131-f78ba919e6e6@gmail.com","threadId":"58399","inReplyTo":"xmqqa6647qx2.fsf@gitster.g","subject":"Re: [PATCH v5] branch: support for shortcuts like @{-1}, completed","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-10-09T21:26:56Z","receivedAt":"2022-10-09T21:27:03Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 9/10/22 21:11, Junio C Hamano wrote:\n> Rubén Justo writes:\n> \n> * cmd_foo() should not return an negative value.\n\nYes, the refactoring we already discussed early in this thread.\n\n> \n> * branch_name used in the calls to error() could point at buf.buf\n> that holds the expansion of @{-1}, but buf was released way too\n> early, leading to a use-after-free.\n\n:-( good catch, thanks. Removing the refactoring commit was not\ncarefully done.\n\n> \n> * Style: if/else if/else cascade whose one arm has multiple\n> statements and requires {braces} around it should have {braces}\n> around all of its arms.\n\nOk.\n\n> \n> * each arm in the top-level if/else if/else cascade for \"git\n> branch\" subcommands were more or less independent, and there\n> wasn't anything common that they need to execute after exiting\n> the cascade. Unconditionally returning from the arm for the\n> edit-description subcommand seems to make the logic flow easier\n> to read.\n\nMmm, I don't feel the same here, we already discussed about this. Maybe?:\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 17853225fa..307073cc47 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -817,7 +817,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n strbuf_release(&branch_ref);\n strbuf_release(&buf);\n \n- return ret;\n+ if (ret)\n+ return ret; /* some failure happened */\n } else if (copy) {\n if (!argc)\n die(_(\"branch name required\"));\n\n\nnot much important, though.\n\nYou can squash the changes in the commit or if you need me to send a v6,\nplease let me know. Thank you for your careful review.\n"},{"id":"464464","messageId":"afec5288-7551-ff42-6dd8-899b5116394f@gmail.com","threadId":"58399","inReplyTo":"xmqq5ygs7pxg.fsf@gitster.g","subject":"Re: [PATCH v4] branch: support for shortcuts like @{-1}, completed","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-10-09T22:27:57Z","receivedAt":"2022-10-09T23:40:07Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"For me, the most value here is in the expressiveness, writing and reading\na simple test.\n\n git config branch.desc-branch.description >actual &&\n test_cmp actual \"Branch description\\n\\n\"\n\nvs\n\n git config branch.desc-branch.description >actual &&\n printf \"Branch description\\n\\n\" >expect &&\n test_cmp expect actual\n\nvs\n\n printf \"Branch description\\n\\n\" >expect &&\n...\n git config branch.desc-branch.description >actual &&\n test_cmp expect actual\n\nvs (oops)\n\n printf \"Branch description\\n\\n\" >expect &&\n...\n git config branch.desc-branch.description >actual &&\n printf \"New branch description\\n\\n\" >expect_ &&\n test_cmp expect actual\n\n\nLess lines, less files and less error prone are a plus.\n\n\nOn 9/10/22 8:46, Eric Sunshine wrote:\n> My knee-jerk reaction is that the form which takes a string as\n> argument would hardly be used, thus an unnecessary complication. The\n\nA quick overview over a not much elaborated search:\n\ngit grep -B 3 'test_cmp expect' | grep '\\(echo\\|cat\\).*expect'\n\ngives many results that imo should be beneficial of this expressiveness.\n\n\n> form which accepts stdin could even be retrofitted onto the existing\n> test_cmp, thus you don't even need to invent a new function name. A\n\nNice. Maybe even the printf form with a \"test ! -f\", some risky though.\n\n\n> different approach would be to introduce a function `test_stdout`\n> which both accepts a command to run, as well as the \"expected\" text on\n> stdin with which to compare the output of the command; i.e. a\n> combination of the existing `test_stdout_line_count` in\n> t/test-lib-functions.sh and your proposed function above.\n\nSounds good to me but maybe this goes in the direction of \"inducing\nbad usages\", and the tests to cover starts to be not so simple...\n\n\nOn 9/10/22 21:33, Junio C Hamano wrote:\n> Eric Sunshine writes:\n> \n>> That said, I'm not yet seeing all that much added value in such a\n>> function; at most, it seems to save only a single line of code, and\n>> it's not as if the code it's replacing was doing anything complicated\n>> or hard to grok.\n> \n> I share the sentiment. Leaving the result that was used in comparison\n> in file(s) also helps debugging.\n> \n\nAnd if this is fulfilled with a drop a file on failure? That resolves the\n'-' in the diff output result too.\n\n\nThank you.\n"},{"id":"464467","messageId":"xmqq4jwc5x7y.fsf@gitster.g","threadId":"58399","inReplyTo":"dacb1f40-2765-5978-1131-f78ba919e6e6@gmail.com","subject":"Re: [PATCH v5] branch: support for shortcuts like @{-1}, completed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-10T00:38:41Z","receivedAt":"2022-10-10T00:56:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> Mmm, I don't feel the same here, we already discussed about this. Maybe?:\n>\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index 17853225fa..307073cc47 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -817,7 +817,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n> strbuf_release(&branch_ref);\n> strbuf_release(&buf);\n> \n> - return ret;\n> + if (ret)\n> + return ret; /* some failure happened */\n> } else if (copy) {\n> if (!argc)\n> die(_(\"branch name required\"));\n\nBefore the above change, the body of the \"else if\" clause for the\noption was self contained. With the above change, the reader has to\nfollow to the end of the long top-level cascade to see the rest of\nthe function does not do anything funny.\n\nIf we have a big common clean-up after each operation, then, falling\nthrough in the success case might be good, but that is not what I am\nseeing here. So...\n\n"},{"id":"464469","messageId":"bf4a45cb-2f44-88bd-e12e-98ca31d5bbd4@gmail.com","threadId":"58399","inReplyTo":"xmqq4jwc5x7y.fsf@gitster.g","subject":"Re: [PATCH v5] branch: support for shortcuts like @{-1}, completed","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-10-10T06:05:26Z","receivedAt":"2022-10-10T06:05:39Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"\nOn 10/10/22 2:38, Junio C Hamano wrote:\n> Rubén Justo writes:\n> \n>> Mmm, I don't feel the same here, we already discussed about this. Maybe?:\n>>\n>> diff --git a/builtin/branch.c b/builtin/branch.c\n>> index 17853225fa..307073cc47 100644\n>> --- a/builtin/branch.c\n>> +++ b/builtin/branch.c\n>> @@ -817,7 +817,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n>> strbuf_release(&branch_ref);\n>> strbuf_release(&buf);\n>> \n>> - return ret;\n>> + if (ret)\n>> + return ret; /* some failure happened */\n>> } else if (copy) {\n>> if (!argc)\n>> die(_(\"branch name required\"));\n> \n> Before the above change, the body of the \"else if\" clause for the\n> option was self contained. With the above change, the reader has to\n> follow to the end of the long top-level cascade to see the rest of\n> the function does not do anything funny.\n> \n> If we have a big common clean-up after each operation, then, falling\n> through in the success case might be good, but that is not what I am\n> seeing here. So...\n> \n\nI would like to see some kind of free(head) in a clean-up to not get\ndistracted with that. Not a proper leak though and the leak checkers\ndoes not refer to that as leak. So not important. We can go with the\nunconditional return and let the dust settle.\n"},{"id":"464498","messageId":"xmqqzge34nzp.fsf@gitster.g","threadId":"58399","inReplyTo":"bf4a45cb-2f44-88bd-e12e-98ca31d5bbd4@gmail.com","subject":"Re: [PATCH v5] branch: support for shortcuts like @{-1}, completed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-10T16:55:38Z","receivedAt":"2022-10-10T16:55:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n>> If we have a big common clean-up after each operation, then, falling\n>> through in the success case might be good, but that is not what I am\n>> seeing here. So...\n>> \n>\n> I would like to see some kind of free(head) in a clean-up to not get\n> distracted with that. Not a proper leak though and the leak checkers\n> does not refer to that as leak. So not important. We can go with the\n> unconditional return and let the dust settle.\n\n\"head\" is not leaking, as a pointer to it is head in a location that\nis still in scope (namely, a file-scope global variable) when the\nprogram exits.\n\nIn fact, the only thing the code before or after this patch does\nafter leaving this top-level if/elseif/else cascade is to return 0\nand doing nothing else. Inserting free(head) would be an unneeded\ndistraction for human developers (doing such a patch, reviewing, and\neven worse, having to read the resulting code in the coming years)\nand waste of computer resources (exiting the process will reclaim\nsuch a piece of memory just fine).\n\n"},{"id":"464511","messageId":"7d51d297-7a18-de05-0da8-a93276455471@gmail.com","threadId":"58399","inReplyTo":"xmqqzge34nzp.fsf@gitster.g","subject":"Re: [PATCH v5] branch: support for shortcuts like @{-1}, completed","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-10-10T18:08:36Z","receivedAt":"2022-10-10T18:08:46Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On 10/10/22 18:55, Junio C Hamano wrote:\n> Rubén Justo writes:\n> \n>>> If we have a big common clean-up after each operation, then, falling\n>>> through in the success case might be good, but that is not what I am\n>>> seeing here. So...\n>>>\n>>\n>> I would like to see some kind of free(head) in a clean-up to not get\n>> distracted with that. Not a proper leak though and the leak checkers\n>> does not refer to that as leak. So not important. We can go with the\n>> unconditional return and let the dust settle.\n> \n> \"head\" is not leaking, as a pointer to it is head in a location that\n> is still in scope (namely, a file-scope global variable) when the\n> program exits.\n> \n> In fact, the only thing the code before or after this patch does\n> after leaving this top-level if/elseif/else cascade is to return 0\n> and doing nothing else. Inserting free(head) would be an unneeded\n> distraction for human developers (doing such a patch, reviewing, and\n> even worse, having to read the resulting code in the coming years)\n> and waste of computer resources (exiting the process will reclaim\n> such a piece of memory just fine).\n> \n\nJust to say that I truly agree.\n\nUn saludo.\n"},{"id":"464542","messageId":"fbf84e26-4306-c8df-0e2c-45dc94129e3a@gmail.com","threadId":"58399","inReplyTo":"de200fa0-379d-c1ce-8446-9e4292d0b66a@gmail.com","subject":"[PATCH v6] branch: support for shortcuts like @{-1}, completed","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2022-10-10T23:24:58Z","receivedAt":"2022-10-10T23:25:06Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"branch command with options \"edit-description\", \"set-upstream-to\" and\n\"unset-upstream\" expects a branch name. Since ae5a6c3684 (checkout:\nimplement \"@{-N}\" shortcut name for N-th last branch, 2009-01-17) a\nbranch can be specified using shortcuts like @{-1}. Those shortcuts\nneed to be resolved when considering the arguments.\n\nWe can modify the description of the previously checked out branch with:\n\n$ git branch --edit--description @{-1}\n\nWe can modify the upstream of the previously checked out branch with:\n\n$ git branch --set-upstream-to upstream @{-1}\n$ git branch --unset-upstream @{-1}\n\nSigned-off-by: Rubén Justo \nSigned-off-by: Junio C Hamano \n---\n\nThis is the v5 with your fix squashed, but with one change to maintain\nthe -1 on error.\n\n -\t\t\terror(!argc\n +\t\t\tret = error(!argc\n \t\t\t ? _(\"No commit on branch '%s' yet.\")\n \t\t\t : _(\"No branch named '%s'.\"),\n \t\t\t branch_name);\n\n\nWe discussed to keep this and other things like the capitalized error\nmessages, to be sorted out in next series, when the dust settles after\nthis and the other patch about errors on unborn branches.\n\nref:\n\n\n\n\nRange-diff:\n1: 00ca255874 ! 1: 11650c6340 branch: support for shortcuts like @{-1}, completed\n @@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix\n \t\tconst char *branch_name;\n \t\tstruct strbuf branch_ref = STRBUF_INIT;\n +\t\tstruct strbuf buf = STRBUF_INIT;\n ++\t\tint ret = 1; /* assume failure */\n \n \t\tif (!argc) {\n \t\t\tif (filter.detached)\n @@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix\n \t\t\tbranch_name = head;\n -\t\t} else if (argc == 1)\n -\t\t\tbranch_name = argv[0];\n +-\t\telse\n +\t\t} else if (argc == 1) {\n +\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n +\t\t\tbranch_name = buf.buf;\n -+\t\t}\n - \t\telse\n ++\t\t} else {\n \t\t\tdie(_(\"cannot edit description of more than one branch\"));\n ++\t\t}\n \n \t\tstrbuf_addf(&branch_ref, \"refs/heads/%s\", branch_name);\n - \t\tif (!ref_exists(branch_ref.buf)) {\n - \t\t\tstrbuf_release(&branch_ref);\n -+\t\t\tstrbuf_release(&buf);\n - \n - \t\t\tif (!argc)\n - \t\t\t\treturn error(_(\"No commit on branch '%s' yet.\"),\n -@@ builtin/branch.c: int cmd_branch(int argc, const char **argv, const char *prefix)\n - \t\t}\n +-\t\tif (!ref_exists(branch_ref.buf)) {\n +-\t\t\tstrbuf_release(&branch_ref);\n +-\n +-\t\t\tif (!argc)\n +-\t\t\t\treturn error(_(\"No commit on branch '%s' yet.\"),\n +-\t\t\t\t\t branch_name);\n +-\t\t\telse\n +-\t\t\t\treturn error(_(\"No branch named '%s'.\"),\n +-\t\t\t\t\t branch_name);\n +-\t\t}\n ++\t\tif (!ref_exists(branch_ref.buf))\n ++\t\t\tret = error(!argc\n ++\t\t\t ? _(\"No commit on branch '%s' yet.\")\n ++\t\t\t : _(\"No branch named '%s'.\"),\n ++\t\t\t branch_name);\n ++\t\telse if (!edit_branch_description(branch_name))\n ++\t\t\tret = 0; /* happy */\n ++\n \t\tstrbuf_release(&branch_ref);\n ++\t\tstrbuf_release(&buf);\n \n -\t\tif (edit_branch_description(branch_name))\n -+\t\tif (edit_branch_description(branch_name)) {\n -+\t\t\tstrbuf_release(&buf);\n - \t\t\treturn 1;\n -+\t\t}\n -+\t\tstrbuf_release(&buf);\n +-\t\t\treturn 1;\n ++\t\treturn ret;\n \t} else if (copy) {\n \t\tif (!argc)\n \t\t\tdie(_(\"branch name required\"));\n\n builtin/branch.c | 53 +++++++++++++++++----------\n t/t3204-branch-name-interpretation.sh | 24 ++++++++++++\n 2 files changed, 58 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 55cd9a6e99..658c9f8741 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -791,31 +791,33 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t} else if (edit_description) {\n \t\tconst char *branch_name;\n \t\tstruct strbuf branch_ref = STRBUF_INIT;\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tint ret = 1; /* assume failure */\n \n \t\tif (!argc) {\n \t\t\tif (filter.detached)\n \t\t\t\tdie(_(\"Cannot give description to detached HEAD\"));\n \t\t\tbranch_name = head;\n-\t\t} else if (argc == 1)\n-\t\t\tbranch_name = argv[0];\n-\t\telse\n+\t\t} else if (argc == 1) {\n+\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n+\t\t\tbranch_name = buf.buf;\n+\t\t} else {\n \t\t\tdie(_(\"cannot edit description of more than one branch\"));\n+\t\t}\n \n \t\tstrbuf_addf(&branch_ref, \"refs/heads/%s\", branch_name);\n-\t\tif (!ref_exists(branch_ref.buf)) {\n-\t\t\tstrbuf_release(&branch_ref);\n-\n-\t\t\tif (!argc)\n-\t\t\t\treturn error(_(\"No commit on branch '%s' yet.\"),\n-\t\t\t\t\t branch_name);\n-\t\t\telse\n-\t\t\t\treturn error(_(\"No branch named '%s'.\"),\n-\t\t\t\t\t branch_name);\n-\t\t}\n+\t\tif (!ref_exists(branch_ref.buf))\n+\t\t\tret = error(!argc\n+\t\t\t ? _(\"No commit on branch '%s' yet.\")\n+\t\t\t : _(\"No branch named '%s'.\"),\n+\t\t\t branch_name);\n+\t\telse if (!edit_branch_description(branch_name))\n+\t\t\tret = 0; /* happy */\n+\n \t\tstrbuf_release(&branch_ref);\n+\t\tstrbuf_release(&buf);\n \n-\t\tif (edit_branch_description(branch_name))\n-\t\t\treturn 1;\n+\t\treturn ret;\n \t} else if (copy) {\n \t\tif (!argc)\n \t\t\tdie(_(\"branch name required\"));\n@@ -835,9 +837,15 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\telse\n \t\t\tdie(_(\"too many arguments for a rename operation\"));\n \t} else if (new_upstream) {\n-\t\tstruct branch *branch = branch_get(argv[0]);\n+\t\tstruct branch *branch;\n+\t\tstruct strbuf buf = STRBUF_INIT;\n \n-\t\tif (argc > 1)\n+\t\tif (!argc)\n+\t\t\tbranch = branch_get(NULL);\n+\t\telse if (argc == 1) {\n+\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n+\t\t\tbranch = branch_get(buf.buf);\n+\t\t} else\n \t\t\tdie(_(\"too many arguments to set new upstream\"));\n \n \t\tif (!branch) {\n@@ -854,11 +862,17 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tdwim_and_setup_tracking(the_repository, branch->name,\n \t\t\t\t\tnew_upstream, BRANCH_TRACK_OVERRIDE,\n \t\t\t\t\tquiet);\n+\t\tstrbuf_release(&buf);\n \t} else if (unset_upstream) {\n-\t\tstruct branch *branch = branch_get(argv[0]);\n+\t\tstruct branch *branch;\n \t\tstruct strbuf buf = STRBUF_INIT;\n \n-\t\tif (argc > 1)\n+\t\tif (!argc)\n+\t\t\tbranch = branch_get(NULL);\n+\t\telse if (argc == 1) {\n+\t\t\tstrbuf_branchname(&buf, argv[0], INTERPRET_BRANCH_LOCAL);\n+\t\t\tbranch = branch_get(buf.buf);\n+\t\t} else\n \t\t\tdie(_(\"too many arguments to unset upstream\"));\n \n \t\tif (!branch) {\n@@ -871,6 +885,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tif (!branch_has_merge_config(branch))\n \t\t\tdie(_(\"Branch '%s' has no upstream information\"), branch->name);\n \n+\t\tstrbuf_reset(&buf);\n \t\tstrbuf_addf(&buf, \"branch.%s.remote\", branch->name);\n \t\tgit_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);\n \t\tstrbuf_reset(&buf);\ndiff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh\nindex 993a6b5eff..793bf4d269 100755\n--- a/t/t3204-branch-name-interpretation.sh\n+++ b/t/t3204-branch-name-interpretation.sh\n@@ -133,4 +133,28 @@ test_expect_success 'checkout does not treat remote @{upstream} as a branch' '\n \texpect_branch HEAD one\n '\n \n+test_expect_success 'edit-description via @{-1}' '\n+\tgit checkout -b desc-branch &&\n+\tgit checkout -b non-desc-branch &&\n+\twrite_script editor <<-\\EOF &&\n+\t\techo \"Branch description\" >\"$1\"\n+\tEOF\n+\tEDITOR=./editor git branch --edit-description @{-1} &&\n+\ttest_must_fail git config branch.non-desc-branch.description &&\n+\tgit config branch.desc-branch.description >actual &&\n+\tprintf \"Branch description\\n\\n\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'modify branch upstream via \"@{-1}\" and \"@{-1}@{upstream}\"' '\n+\tgit checkout -b upstream-branch &&\n+\tgit checkout -b upstream-other -t upstream-branch &&\n+\tgit branch --set-upstream-to upstream-other @{-1} &&\n+\tgit config branch.upstream-branch.merge >actual &&\n+\techo \"refs/heads/upstream-other\" >expect &&\n+\ttest_cmp expect actual &&\n+\tgit branch --unset-upstream @{-1}@{upstream} &&\n+\ttest_must_fail git config branch.upstream-other.merge\n+'\n+\n test_done\n-- \n2.36.1\n"}]}