{"thread":{"id":"52618","subject":"[PATCH v4] branch: let '--edit-description' default to rebased/bisected branch","startedAt":"2020-01-12T06:47:22Z","lastAt":"2020-01-12T10:55:34Z","messageCount":3,"participants":["marcandre.lureau@redhat.com","Eric Sunshine"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"389626","messageId":"20200112064706.2030292-1-marcandre.lureau@redhat.com","threadId":"52618","inReplyTo":null,"subject":"[PATCH v4] branch: let '--edit-description' default to rebased/bisected branch","fromName":"","fromEmail":"marcandre.lureau@redhat.com","sentAt":"2020-01-12T06:47:06Z","receivedAt":"2020-01-12T06:47:22Z","isPatch":true,"sender":{"key":"marcandre.lureau@redhat.com","avatar":null},"body":"From: Marc-André Lureau <marcandre.lureau@redhat.com>\n\nDefaulting to editing the description of the rebased or bisected branch\nwithout an explicit branchname argument would be useful.  Even the git\nbash prompt shows the name of the rebased branch, and then\n\n  ~/src/git (mybranch|REBASE-i 1/2)$ git branch --edit-description\n  fatal: Cannot give description to detached HEAD\n\nlooks quite unhelpful.\n\nSigned-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>\n---\nChanged in v4:\n - use wt_status_get_state() that handles bisect state\n - add a bisecting test\n\nbuiltin/branch.c  | 41 ++++++++++++++++++++++++++++++-----------\n t/t3200-branch.sh | 35 +++++++++++++++++++++++++++++++++++\n 2 files changed, 65 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex d8297f80ff..cda9fd53e6 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -745,33 +745,52 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tstring_list_clear(&output, 0);\n \t\treturn 0;\n \t} else if (edit_description) {\n-\t\tconst char *branch_name;\n+\t\tchar *branch_name = NULL;\n \t\tstruct strbuf branch_ref = 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\tif (!filter.detached)\n+\t\t\t\tbranch_name = xstrdup(head);\n+\t\t\telse {\n+\t\t\t\tstruct wt_status_state state;\n+\n+\t\t\t\tmemset(&state, 0, sizeof(state));\n+\t\t\t\twt_status_get_state(the_repository, &state, 0);\n+\t\t\t\tbranch_name = state.branch;\n+\t\t\t\tif (!branch_name)\n+\t\t\t\t\tdie(_(\"Cannot give description to detached HEAD\"));\n+\t\t\t\tfree(state.onto);\n+\t\t\t\tfree(state.detached_from);\n+\t\t\t}\n \t\t} else if (argc == 1)\n-\t\t\tbranch_name = argv[0];\n+\t\t\tbranch_name = xstrdup(argv[0]);\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\tint ret;\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\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\t\t     branch_name);\n+\t\t\t\tret = error(_(\"No branch named '%s'.\"),\n+\t\t\t\t\t    branch_name);\n+\n+\t\t\tstrbuf_release(&branch_ref);\n+\t\t\tfree(branch_name);\n+\t\t\treturn ret;\n+\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\tfree(branch_name);\n \t\t\treturn 1;\n+\t\t}\n+\n+\t\tfree(branch_name);\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 411a70b0ce..7ea6876fe7 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -1260,6 +1260,41 @@ test_expect_success 'use --edit-description' '\n \ttest_cmp expect EDITOR_OUTPUT\n '\n \n+test_expect_success 'use --edit-description during rebase' '\n+\twrite_script editor <<-\\EOF &&\n+\t\techo \"Rebase contents\" >\"$1\"\n+\tEOF\n+\t(\n+\t\tset_fake_editor &&\n+\t\tFAKE_LINES=\"break 1\" git rebase -i HEAD^ &&\n+\t\tEDITOR=./editor git branch --edit-description &&\n+\t\tgit rebase --continue\n+\t) &&\n+\twrite_script editor <<-\\EOF &&\n+\t\tgit stripspace -s <\"$1\" >\"EDITOR_OUTPUT\"\n+\tEOF\n+\tEDITOR=./editor git branch --edit-description &&\n+\techo \"Rebase contents\" >expect &&\n+\ttest_cmp expect EDITOR_OUTPUT\n+'\n+\n+test_expect_success 'use --edit-description during bisect' '\n+\twrite_script editor <<-\\EOF &&\n+\t\techo \"Bisect contents\" >\"$1\"\n+\tEOF\n+\tgit bisect start &&\n+\tgit bisect bad &&\n+\tgit bisect good HEAD~2 &&\n+\tEDITOR=./editor git branch --edit-description &&\n+\tgit bisect reset &&\n+\twrite_script editor <<-\\EOF &&\n+\t\tgit stripspace -s <\"$1\" >\"EDITOR_OUTPUT\"\n+\tEOF\n+\tEDITOR=./editor git branch --edit-description &&\n+\techo \"Bisect contents\" >expect &&\n+\ttest_cmp expect EDITOR_OUTPUT\n+'\n+\n test_expect_success 'detect typo in branch name when using --edit-description' '\n \twrite_script editor <<-\\EOF &&\n \t\techo \"New contents\" >\"$1\"\n\nbase-commit: 7a6a90c6ec48fc78c83d7090d6c1b95d8f3739c0\n-- \n2.25.0.rc2.1.ga00adf396b.dirty\n\n"},{"id":"389627","messageId":"20200112101735.GA19676@flurp.local","threadId":"52618","inReplyTo":"20200112064706.2030292-1-marcandre.lureau@redhat.com","subject":"Re: [PATCH v4] branch: let '--edit-description' default to rebased/bisected branch","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-01-12T10:17:35Z","receivedAt":"2020-01-12T10:17:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Jan 12, 2020 at 10:47:06AM +0400, marcandre.lureau@redhat.com wrote:\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> @@ -745,33 +745,52 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n>  \t\tstring_list_clear(&output, 0);\n>  \t\treturn 0;\n>  \t} else if (edit_description) {\n> -\t\tconst char *branch_name;\n> +\t\tchar *branch_name = NULL;\n\nDo you need to assign NULL here? Doesn't 'branch_name' get assigned in\nall cases in which the code doesn't otherwise die()?\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\tif (!filter.detached)\n> +\t\t\t\tbranch_name = xstrdup(head);\n> +\t\t\telse {\n> +\t\t\t\tstruct wt_status_state state;\n> +\n> +\t\t\t\tmemset(&state, 0, sizeof(state));\n> +\t\t\t\twt_status_get_state(the_repository, &state, 0);\n> +\t\t\t\tbranch_name = state.branch;\n> +\t\t\t\tif (!branch_name)\n> +\t\t\t\t\tdie(_(\"Cannot give description to detached HEAD\"));\n> +\t\t\t\tfree(state.onto);\n> +\t\t\t\tfree(state.detached_from);\n\nI was wondering if it would make sense to attempt this branch name\nlookup much earlier in the function when it assigns 'head' (if 'head'\nis detached), with the idea that perhaps other git-branch modes might\nbenefit from it rather than doing it only for this one special-case.\nHowever, it looks like other code (such as branch copy and branch\nrename) would actively be hurt by such a change.\n\nAt any rate, it might make the 'edit_description' case easier to read\nif this special-case branch lookup code was factored out into its own\nfunction. Not itself worth a re-roll, but something to consider if you\ndo re-roll.\n\n> +\t\t\t}\n>  \t\t} else if (argc == 1)\n> -\t\t\tbranch_name = argv[0];\n> +\t\t\tbranch_name = xstrdup(argv[0]);\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\tint ret;\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\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\t\t     branch_name);\n> +\t\t\t\tret = error(_(\"No branch named '%s'.\"),\n> +\t\t\t\t\t    branch_name);\n> +\n> +\t\t\tstrbuf_release(&branch_ref);\n> +\t\t\tfree(branch_name);\n> +\t\t\treturn ret;\n> +\n>  \t\t}\n\nUnnecessary blank line after 'return'.\n\nA couple observations...\n\nThe extra cleanup needed to handle 'branch_name' makes this code quite\na bit more verbose. I was wondering if it would be possible to\nconsolidate the cleanup in a \"failure path\" as the target of a 'goto'\n(which is a common way to perform cleanup in the Git code-base).\nHowever, doing it that way doesn't really make the code much nicer,\nwhich leads to the next observation...\n\nThose `return error(...)` invocations are anomalies in this function.\nEvery other case of error in cmd_branch() simply die()s -- without\nbothering to clean up. There is no apparent reason why this code\ninstead uses error(). Changing these two cases to die() would simplify\ncleanup since you wouldn't have to do any, which would make the code\nclearer, shorter, and more consistent with the rest of cmd_branch().\n(Such a change probably ought to be done first in a preparatory patch,\nmaking this a two-patch series.)\n\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\tfree(branch_name);\n>  \t\t\treturn 1;\n> +\t\t}\n> +\n> +\t\tfree(branch_name);\n\nTaking the above comments and observations into account, perhaps\nsomething like this would be cleaner:\n\n--- >8 ---\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex d8297f80ff..0eb519561e 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -601,6 +601,22 @@ static int edit_branch_description(const char *branch_name)\n \treturn 0;\n }\n \n+/*\n+ * Return branch name of current worktree -- even if HEAD is detached -- or\n+ * NULL if no branch is associated with worktree. Caller is responsible for\n+ * freeing result.\n+ */\n+static char *get_worktree_branch()\n+{\n+\tstruct wt_status_state state;\n+\n+\tmemset(&state, 0, sizeof(state));\n+\twt_status_get_state(the_repository, &state, 0);\n+\tfree(state.onto);\n+\tfree(state.detached_from);\n+\treturn state.branch;\n+}\n+\n int cmd_branch(int argc, const char **argv, const char *prefix)\n {\n \tint delete = 0, rename = 0, copy = 0, force = 0, list = 0;\n@@ -745,13 +761,16 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tstring_list_clear(&output, 0);\n \t\treturn 0;\n \t} else if (edit_description) {\n+\t\tint ret;\n \t\tconst char *branch_name;\n+\t\tchar *to_free = NULL;\n \t\tstruct strbuf branch_ref = STRBUF_INIT;\n \n \t\tif (!argc) {\n-\t\t\tif (filter.detached)\n+\t\t\tif (!filter.detached)\n+\t\t\t\tbranch_name = head;\n+\t\t\telse if (!(branch_name = to_free = get_worktree_branch()))\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@@ -759,19 +778,16 @@ 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\t\t     branch_name);\n+\t\t\t\tdie(_(\"No commit on branch '%s' yet.\"), 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\t\tdie(_(\"No branch named '%s'.\"), branch_name);\n \t\t}\n \t\tstrbuf_release(&branch_ref);\n \n-\t\tif (edit_branch_description(branch_name))\n-\t\t\treturn 1;\n+\t\tret = edit_branch_description(branch_name);\n+\t\tfree(to_free);\n+\t\treturn ret;\n \t} else if (copy) {\n \t\tif (!argc)\n \t\t\tdie(_(\"branch name required\"));\n--- >8 ---\n"},{"id":"389629","messageId":"CAPig+cQqKvBc3ugfVtmaSZb+pBkp2wkBG=++GTPODp7oCaGBhQ@mail.gmail.com","threadId":"52618","inReplyTo":"20200112101735.GA19676@flurp.local","subject":"Re: [PATCH v4] branch: let '--edit-description' default to rebased/bisected branch","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-01-12T10:55:19Z","receivedAt":"2020-01-12T10:55:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Jan 12, 2020 at 5:17 AM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> +/*\n> + * Return branch name of current worktree -- even if HEAD is detached -- or\n> + * NULL if no branch is associated with worktree. Caller is responsible for\n> + * freeing result.\n> + */\n> +static char *get_worktree_branch()\n\nThis would of course be:\n\n    static char *get_worktree_branch(void)\n"}]}