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

[Andrei Rybak <rybak.a.v@gmail.com>]

On 2019-11-21 03:00, Junio C Hamano wrote:
> 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.

I would guess that it had something to do with passing this value to the helper
binary rebase--interactive before libification.  I couldn't figure out this
mechanism before commit 460bc3ce73 ("rebase -i: run without forking
rebase--interactive", 2019-04-17), so that's just a guess.

I will look into possible simplification of this to avoid the chain of
conversions string_list -> char * -> string_list.

> Thanks for working on this.

Thank you for review :-)