{"thread":{"id":"52610","subject":"[PATCH] RFC: allow branch --edit-description during rebase","startedAt":"2020-01-10T07:19:43Z","lastAt":"2020-01-10T13:18:47Z","messageCount":2,"participants":["marcandre.lureau@redhat.com","SZEDER Gábor"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"389567","messageId":"20200110071929.119000-1-marcandre.lureau@redhat.com","threadId":"52610","inReplyTo":null,"subject":"[PATCH] RFC: allow branch --edit-description during rebase","fromName":"","fromEmail":"marcandre.lureau@redhat.com","sentAt":"2020-01-10T07:19:29Z","receivedAt":"2020-01-10T07:19:43Z","isPatch":true,"sender":{"key":"marcandre.lureau@redhat.com","avatar":null},"body":"From: Marc-André Lureau <marcandre.lureau@redhat.com>\n\nThis patch aims to allow editing of branch description during a rebase.\n\nA common use case of rebasing is to iterate over a series of patches\nafter receiving reviews. During the rebase, various patches will be\nmodified, and it is often requested to put a summary of the changes for\nthe next version in the cover letter (\"v2: - fixed this, - changed\nthat..\"). This helps the reviewer to focus on the difference with the\nprevious version.  Unfortunately, git branch --edit-description doesn't\nallow yet to modify the content during a rebase, and forces the author\nto use memory muscles to update the description after finishing the\nrebase.\n\nSigned-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>\n---\n builtin/branch.c | 19 ++++++++++++++++---\n worktree.c       | 19 +++++++++++++++++++\n worktree.h       |  7 +++++++\n 3 files changed, 42 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex d8297f80ff..f7122d31d6 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -613,6 +613,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \tint icase = 0;\n \tstatic struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n \tstruct ref_format format = REF_FORMAT_INIT;\n+\tstruct wt_status_state state;\n \n \tstruct option options[] = {\n \t\tOPT_GROUP(N_(\"Generic options\")),\n@@ -664,6 +665,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \n \tsetup_ref_filter_porcelain_msg();\n \n+\tmemset(&state, 0, sizeof(state));\n+\n \tmemset(&filter, 0, sizeof(filter));\n \tfilter.kind = FILTER_REFS_BRANCHES;\n \tfilter.abbrev = -1;\n@@ -745,13 +748,21 @@ 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\tconst char *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    if (filter.detached) {\n+\t\t\tconst struct worktree *wt = worktree_get_current();\n+\n+\t\t\tif (wt_status_check_rebase(wt, &state)) {\n+\t\t\t\tbranch_name = state.branch;\n+\t\t\t}\n+\n+\t\t\tif (!branch_name)\n \t\t\t\tdie(_(\"Cannot give description to detached HEAD\"));\n-\t\t\tbranch_name = head;\n+\t\t    } else\n+\t\t\t    branch_name = head;\n \t\t} else if (argc == 1)\n \t\t\tbranch_name = argv[0];\n \t\telse\n@@ -851,5 +862,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t} else\n \t\tusage_with_options(builtin_branch_usage, options);\n \n+\tfree(state.branch);\n+\tfree(state.onto);\n \treturn 0;\n }\ndiff --git a/worktree.c b/worktree.c\nindex 5b4793caa3..0318c6f6a6 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -396,6 +396,25 @@ int is_worktree_being_bisected(const struct worktree *wt,\n \treturn found_rebase;\n }\n \n+const struct worktree *worktree_get_current(void)\n+{\n+\tstatic struct worktree **worktrees;\n+\tint i = 0;\n+\n+\tif (worktrees)\n+\t\tfree_worktrees(worktrees);\n+\tworktrees = get_worktrees(0);\n+\n+\tfor (i = 0; worktrees[i]; i++) {\n+\t\tstruct worktree *wt = worktrees[i];\n+\n+\t\tif (wt->is_current)\n+\t\t\treturn wt;\n+\t}\n+\n+\treturn NULL;\n+}\n+\n /*\n  * note: this function should be able to detect shared symref even if\n  * HEAD is temporarily detached (e.g. in the middle of rebase or\ndiff --git a/worktree.h b/worktree.h\nindex caecc7a281..4fe2b78d24 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -91,6 +91,13 @@ void free_worktrees(struct worktree **);\n const struct worktree *find_shared_symref(const char *symref,\n \t\t\t\t\t  const char *target);\n \n+\n+/*\n+ * Return the current worktree. The result may be destroyed by the\n+ * next call.\n+ */\n+const struct worktree *worktree_get_current(void);\n+\n /*\n  * Similar to head_ref() for all HEADs _except_ one from the current\n  * worktree, which is covered by head_ref().\n\nbase-commit: 042ed3e048af08014487d19196984347e3be7d1c\nprerequisite-patch-id: 9b3cf75545ec4a1e702c8c2b2aae8edf241b87f2\n-- \n2.25.0.rc1.20.g2443f3f80d.dirty\n\n"},{"id":"389579","messageId":"20200110131840.GG32750@szeder.dev","threadId":"52610","inReplyTo":"20200110071929.119000-1-marcandre.lureau@redhat.com","subject":"Re: [PATCH] RFC: allow branch --edit-description during rebase","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2020-01-10T13:18:40Z","receivedAt":"2020-01-10T13:18:47Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jan 10, 2020 at 11:19:29AM +0400, marcandre.lureau@redhat.com wrote:\n> From: Marc-André Lureau <marcandre.lureau@redhat.com>\n> \n> This patch aims to allow editing of branch description during a rebase.\n> \n> A common use case of rebasing is to iterate over a series of patches\n> after receiving reviews. During the rebase, various patches will be\n> modified, and it is often requested to put a summary of the changes for\n> the next version in the cover letter (\"v2: - fixed this, - changed\n> that..\"). This helps the reviewer to focus on the difference with the\n> previous version.  Unfortunately, git branch --edit-description doesn't\n> allow yet to modify the content during a rebase, and forces the author\n> to use memory muscles to update the description after finishing the\n> rebase.\n\nThat's not true, 'git branch --edit-description mybranch' already\nallows you to edit the branch description of the currently rebased\nbranch (well, basically of any branch, really).\n\nSo it's not really about allowing '--edit-description' during rebase,\nbut choosing the default branch during rebase sensibly, and the\nsubject line could be something like \"branch: let '--edit-description'\ndefault to rebased branch during rebase\", and the rest of the commit\nmessage should be updated accordingly.\n\nHaving said that, I agree that defaulting to editing the description\nof the rebased branch without an explicit branchname argument makes\nsense.  Even the git bash prompt shows the name of the rebased branch,\nand 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\n> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>\n> ---\n>  builtin/branch.c | 19 ++++++++++++++++---\n>  worktree.c       | 19 +++++++++++++++++++\n>  worktree.h       |  7 +++++++\n\nTests? :)\n\nI think it's worth checking '--edit-description' while rebasing a\nbranch, while rebasing a detached HEAD, and while rebasing in a\ndifferent worktree.\n\n>  3 files changed, 42 insertions(+), 3 deletions(-)\n> \n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index d8297f80ff..f7122d31d6 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -613,6 +613,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n>  \tint icase = 0;\n>  \tstatic struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;\n>  \tstruct ref_format format = REF_FORMAT_INIT;\n> +\tstruct wt_status_state state;\n\nThis variable is only used for '--edit-description', and even then only\nwhen on a detached head; please limit its scope.\n\n>  \tstruct option options[] = {\n>  \t\tOPT_GROUP(N_(\"Generic options\")),\n> @@ -664,6 +665,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n>  \n>  \tsetup_ref_filter_porcelain_msg();\n>  \n> +\tmemset(&state, 0, sizeof(state));\n> +\n>  \tmemset(&filter, 0, sizeof(filter));\n>  \tfilter.kind = FILTER_REFS_BRANCHES;\n>  \tfilter.abbrev = -1;\n> @@ -745,13 +748,21 @@ 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\tconst char *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    if (filter.detached) {\n\nPlease use tabs for indentation.\n\n> +\t\t\tconst struct worktree *wt = worktree_get_current();\n> +\n> +\t\t\tif (wt_status_check_rebase(wt, &state)) {\n\nI think passing NULL as the 'wt' argument means \"check the current\nworktree\".  If that's indeed the case then you don't have to add that\nworktree_get_current() function at all.\n\n> +\t\t\t\tbranch_name = state.branch;\n> +\t\t\t}\n> +\n> +\t\t\tif (!branch_name)\n>  \t\t\t\tdie(_(\"Cannot give description to detached HEAD\"));\n> -\t\t\tbranch_name = head;\n> +\t\t    } else\n> +\t\t\t    branch_name = head;\n>  \t\t} else if (argc == 1)\n>  \t\t\tbranch_name = argv[0];\n>  \t\telse\n> @@ -851,5 +862,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n>  \t} else\n>  \t\tusage_with_options(builtin_branch_usage, options);\n>  \n> +\tfree(state.branch);\n> +\tfree(state.onto);\n>  \treturn 0;\n>  }\n> diff --git a/worktree.c b/worktree.c\n> index 5b4793caa3..0318c6f6a6 100644\n> --- a/worktree.c\n> +++ b/worktree.c\n> @@ -396,6 +396,25 @@ int is_worktree_being_bisected(const struct worktree *wt,\n>  \treturn found_rebase;\n>  }\n>  \n> +const struct worktree *worktree_get_current(void)\n> +{\n> +\tstatic struct worktree **worktrees;\n> +\tint i = 0;\n> +\n> +\tif (worktrees)\n> +\t\tfree_worktrees(worktrees);\n> +\tworktrees = get_worktrees(0);\n\nI'm not sure about this static worktrees array and how it is handled\nhere.  I mean, can the current worktree change mid-process?\n\n(Though this is moot if this function turns out to be unnecessary, as\nmentioned above.)\n\n> +\tfor (i = 0; worktrees[i]; i++) {\n> +\t\tstruct worktree *wt = worktrees[i];\n> +\n> +\t\tif (wt->is_current)\n> +\t\t\treturn wt;\n> +\t}\n> +\n> +\treturn NULL;\n> +}\n> +\n>  /*\n>   * note: this function should be able to detect shared symref even if\n>   * HEAD is temporarily detached (e.g. in the middle of rebase or\n> diff --git a/worktree.h b/worktree.h\n> index caecc7a281..4fe2b78d24 100644\n> --- a/worktree.h\n> +++ b/worktree.h\n> @@ -91,6 +91,13 @@ void free_worktrees(struct worktree **);\n>  const struct worktree *find_shared_symref(const char *symref,\n>  \t\t\t\t\t  const char *target);\n>  \n> +\n> +/*\n> + * Return the current worktree. The result may be destroyed by the\n> + * next call.\n> + */\n> +const struct worktree *worktree_get_current(void);\n> +\n>  /*\n>   * Similar to head_ref() for all HEADs _except_ one from the current\n>   * worktree, which is covered by head_ref().\n> \n> base-commit: 042ed3e048af08014487d19196984347e3be7d1c\n> prerequisite-patch-id: 9b3cf75545ec4a1e702c8c2b2aae8edf241b87f2\n> -- \n> 2.25.0.rc1.20.g2443f3f80d.dirty\n> \n"}]}