Re: [RFC PATCH v2 2/4] rebase: prepare cmd before choosing action
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 21, 2019, 02:00 UTC
- Message-ID
- <xmqqimne9efm.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20191120095238.4349-3-rybak.a.v@gmail.com>
Andrei Rybak <rybak.a.v@gmail.com> writes:
Show 6 quoted lines
> When git rebase is started with option --exec, its arguments are parsed > into string_list exec and then converted into options.cmd. > > In following commits, action --edit-todo will be taught to use arguments > passed with --exec option. Prepare options.cmd before switch (action) > to make it available for the ACTION_EDIT_TODO branch of the switch.
Hmph. With or without this change, when we hit the run_rebase label in this function and call into run_rebase_interactive(), opts->cmd does contain what came from the --exec option. In that function, I see ACTION_EDIT_TODO calls edit_todo_file() that edits the on-disk file without paying attention to opts->cmd (the only thing in the function that pays attention to this field is ACTION_ADD_EXEC).
So I am not sure what makes this step necessary. I guess it is not wrong per-se, but if the objetive of this series is to add what came from the --exec option when the user interacts with the editor in rebase-interactive.c::edit_todo_list(), wouldn't it be sufficient to skip this step, pass opts to edit_todo_file() and let the helper use opts->cmd while preparing the todo_list it passes to underlying edit_todo_list() function?
I am not claiming that it would be a better way---I wouldn't be surprised if it is an incorrect approach---but it is unclear why this step is needed and why the tweak of the todo list must be done in the "switch (action)" we see in the post context of the first hunk in this patch.
Thanks for working on this.
Show 56 quoted lines
> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
> ---
> builtin/rebase.c | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 793cac1386..fa27f7b439 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1595,6 +1595,19 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> trace2_cmd_mode(action_names[action]);
> }
>
> + for (i = 0; i < exec.nr; i++)
> + if (check_exec_cmd(exec.items[i].string))
> + exit(1);
> +
> + if (exec.nr) {
> + imply_interactive(&options, "--exec");
> +
> + strbuf_reset(&buf);
> + for (i = 0; i < exec.nr; i++)
> + strbuf_addf(&buf, "exec %s\n", exec.items[i].string);
> + options.cmd = xstrdup(buf.buf);
> + }
> +
> switch (action) {
> case ACTION_CONTINUE: {
> struct object_id head;
> @@ -1731,10 +1744,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> }
> }
>
> - for (i = 0; i < exec.nr; i++)
> - if (check_exec_cmd(exec.items[i].string))
> - exit(1);
> -
> if (!(options.flags & REBASE_NO_QUIET))
> argv_array_push(&options.git_am_opts, "-q");
>
> @@ -1746,15 +1755,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign);
> }
>
> - if (exec.nr) {
> - imply_interactive(&options, "--exec");
> -
> - strbuf_reset(&buf);
> - for (i = 0; i < exec.nr; i++)
> - strbuf_addf(&buf, "exec %s\n", exec.items[i].string);
> - options.cmd = xstrdup(buf.buf);
> - }
> -
> if (rebase_merges) {
> if (!*rebase_merges)
> ; /* default mode; do nothing */