Re: [PATCH] RFC: allow branch --edit-description during rebase
- From
SZEDER Gábor <szeder.dev@gmail.com>
- Date
- Jan 10, 2020, 13:18 UTC
- Message-ID
- <20200110131840.GG32750@szeder.dev>
- In-Reply-To
- <20200110071929.119000-1-marcandre.lureau@redhat.com>
On Fri, Jan 10, 2020 at 11:19:29AM +0400, marcandre.lureau@redhat.com wrote:
Show 13 quoted lines
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> This patch aims to allow editing of branch description during a rebase.
>
> A common use case of rebasing is to iterate over a series of patches
> after receiving reviews. During the rebase, various patches will be
> modified, and it is often requested to put a summary of the changes for
> the next version in the cover letter ("v2: - fixed this, - changed
> that.."). This helps the reviewer to focus on the difference with the
> previous version. Unfortunately, git branch --edit-description doesn't
> allow yet to modify the content during a rebase, and forces the author
> to use memory muscles to update the description after finishing the
> rebase.That's not true, 'git branch --edit-description mybranch' already allows you to edit the branch description of the currently rebased branch (well, basically of any branch, really).
So it's not really about allowing '--edit-description' during rebase, but choosing the default branch during rebase sensibly, and the subject line could be something like "branch: let '--edit-description' default to rebased branch during rebase", and the rest of the commit message should be updated accordingly.
Having said that, I agree that defaulting to editing the description of the rebased branch without an explicit branchname argument makes sense. Even the git bash prompt shows the name of the rebased branch, and then
~/src/git (mybranch|REBASE-i 1/2)$ git branch --edit-description fatal: Cannot give description to detached HEAD
looks quite unhelpful.
Show 5 quoted lines
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > builtin/branch.c | 19 ++++++++++++++++--- > worktree.c | 19 +++++++++++++++++++ > worktree.h | 7 +++++++
Tests? :)
I think it's worth checking '--edit-description' while rebasing a branch, while rebasing a detached HEAD, and while rebasing in a different worktree.
Show 11 quoted lines
> 3 files changed, 42 insertions(+), 3 deletions(-) > > diff --git a/builtin/branch.c b/builtin/branch.c > index d8297f80ff..f7122d31d6 100644 > --- a/builtin/branch.c > +++ b/builtin/branch.c > @@ -613,6 +613,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) > int icase = 0; > static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting; > struct ref_format format = REF_FORMAT_INIT; > + struct wt_status_state state;
This variable is only used for '--edit-description', and even then only when on a detached head; please limit its scope.
Show 22 quoted lines
> struct option options[] = {
> OPT_GROUP(N_("Generic options")),
> @@ -664,6 +665,8 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
>
> setup_ref_filter_porcelain_msg();
>
> + memset(&state, 0, sizeof(state));
> +
> memset(&filter, 0, sizeof(filter));
> filter.kind = FILTER_REFS_BRANCHES;
> filter.abbrev = -1;
> @@ -745,13 +748,21 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> string_list_clear(&output, 0);
> return 0;
> } else if (edit_description) {
> - const char *branch_name;
> + const char *branch_name = NULL;
> struct strbuf branch_ref = STRBUF_INIT;
>
> if (!argc) {
> - if (filter.detached)
> + if (filter.detached) {Please use tabs for indentation.
> + const struct worktree *wt = worktree_get_current();
> +
> + if (wt_status_check_rebase(wt, &state)) {I think passing NULL as the 'wt' argument means "check the current worktree". If that's indeed the case then you don't have to add that worktree_get_current() function at all.
Show 35 quoted lines
> + branch_name = state.branch;
> + }
> +
> + if (!branch_name)
> die(_("Cannot give description to detached HEAD"));
> - branch_name = head;
> + } else
> + branch_name = head;
> } else if (argc == 1)
> branch_name = argv[0];
> else
> @@ -851,5 +862,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
> } else
> usage_with_options(builtin_branch_usage, options);
>
> + free(state.branch);
> + free(state.onto);
> return 0;
> }
> diff --git a/worktree.c b/worktree.c
> index 5b4793caa3..0318c6f6a6 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -396,6 +396,25 @@ int is_worktree_being_bisected(const struct worktree *wt,
> return found_rebase;
> }
>
> +const struct worktree *worktree_get_current(void)
> +{
> + static struct worktree **worktrees;
> + int i = 0;
> +
> + if (worktrees)
> + free_worktrees(worktrees);
> + worktrees = get_worktrees(0);I'm not sure about this static worktrees array and how it is handled here. I mean, can the current worktree change mid-process?
(Though this is moot if this function turns out to be unnecessary, as mentioned above.)
Show 37 quoted lines
> + for (i = 0; worktrees[i]; i++) {
> + struct worktree *wt = worktrees[i];
> +
> + if (wt->is_current)
> + return wt;
> + }
> +
> + return NULL;
> +}
> +
> /*
> * note: this function should be able to detect shared symref even if
> * HEAD is temporarily detached (e.g. in the middle of rebase or
> diff --git a/worktree.h b/worktree.h
> index caecc7a281..4fe2b78d24 100644
> --- a/worktree.h
> +++ b/worktree.h
> @@ -91,6 +91,13 @@ void free_worktrees(struct worktree **);
> const struct worktree *find_shared_symref(const char *symref,
> const char *target);
>
> +
> +/*
> + * Return the current worktree. The result may be destroyed by the
> + * next call.
> + */
> +const struct worktree *worktree_get_current(void);
> +
> /*
> * Similar to head_ref() for all HEADs _except_ one from the current
> * worktree, which is covered by head_ref().
>
> base-commit: 042ed3e048af08014487d19196984347e3be7d1c
> prerequisite-patch-id: 9b3cf75545ec4a1e702c8c2b2aae8edf241b87f2
> --
> 2.25.0.rc1.20.g2443f3f80d.dirty
>