Re: [RFC PATCH v2 2/4] rebase: prepare cmd before choosing action

[Junio C Hamano <gitster@pobox.com>]

Andrei Rybak <rybak.a.v@gmail.com> writes:

> 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.


> 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 */