| Message ID | 20191120095238.4349-3-rybak.a.v@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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 */
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 :-)
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 */
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. Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com> --- builtin/rebase.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)