git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Andrei Rybak <rybak.a.v@gmail.com>
Date
Nov 22, 2019, 19:08 UTC
Message-ID
<b6d8fe84-cba1-543e-b387-4842b61829f6@gmail.com>
In-Reply-To
<xmqqimne9efm.fsf@gitster-ct.c.googlers.com>
On 2019-11-21 03:00, Junio C Hamano wrote:
Show 27 quoted lines
> 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 :-)
Previous: Junio C HamanoNext: Andrei Rybak
Message 10 of 13 in “rebase --edit-todo --exec”
  1. 0/3 rebase --edit-todo --execAndrei Rybak, Nov 14, 2019
  2. 1/3 rebase: prepare cmd before choosing actionAndrei Rybak, Nov 14, 2019
  3. Junio C HamanoNov 15, 2019
  4. 2/3 rebase: extract add_exec()Andrei Rybak, Nov 14, 2019
  5. 3/3 rebase -i: allow --edit-todo with --execAndrei Rybak, Nov 14, 2019
  6. 1/4 builtin/rebase.c: reuse loop variableAndrei Rybak, Nov 20, 2019
  7. Junio C HamanoNov 21, 2019
  8. 2/4 rebase: prepare cmd before choosing actionAndrei Rybak, Nov 20, 2019
  9. Junio C HamanoNov 21, 2019
  10. Andrei RybakNov 22, 2019
  11. 0/4 rebase --edit-todo --execAndrei Rybak, Nov 20, 2019
  12. 3/4 rebase: extract add_exec()Andrei Rybak, Nov 20, 2019
  13. 4/4 rebase -i: allow --edit-todo with --execAndrei Rybak, Nov 20, 2019

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.