Hi Alban I think it is a good idea to move these functions out of sequencer.c, previously I suggested moving them to rebase-interactive.c rather than builtin/rebase--interactive.c but I don't think it matters that much. If these can be moved when they're refactored (or in a separate commit immediately after they're refactored if that makes the refactoring patch clearer) rather than at the end it might make the history cleaner.
Best Wishes Phillip On 10/02/2019 13:26, Alban Gruin wrote: > As sequencer_add_exec_commands(), rearrange_squash_in_todo_file(), and > transform_todo_file() are only needed inside of rebase--interactive.c > for rebase -p, they are moved there from sequencer.c. > > The parameter r (repository) is dropped from them, and the error > handling of rearrange_squash_in_todo_file() is slightly improved. > > Signed-off-by: Alban Gruin <[email protected]> > --- > builtin/rebase--interactive.c | 81 +++++++++++++++++++++++++++++++++-- > sequencer.c | 79 ++-------------------------------- > sequencer.h | 7 ++- > 3 files changed, 84 insertions(+), 83 deletions(-) > > diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c > index 370d584683..fb11f23692 100644 > --- a/builtin/rebase--interactive.c > +++ b/builtin/rebase--interactive.c > @@ -13,6 +13,81 @@ static GIT_PATH_FUNC(path_state_dir, "rebase-merge/") > static GIT_PATH_FUNC(path_squash_onto, "rebase-merge/squash-onto") > static GIT_PATH_FUNC(path_interactive, "rebase-merge/interactive") > > +static int add_exec_commands(struct string_list *commands) > +{ > + const char *todo_file = rebase_path_todo(); > + struct todo_list todo_list = TODO_LIST_INIT; > + int res; > + > + if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) > + return error_errno(_("could not read '%s'."), todo_file); > + > + if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf, > + &todo_list)) { > + todo_list_release(&todo_list); > + return error(_("unusable todo list: '%s'"), todo_file); > + } > + > + todo_list_add_exec_commands(&todo_list, commands); > + res = todo_list_write_to_file(the_repository, &todo_list, > + todo_file, NULL, NULL, -1, 0); > + todo_list_release(&todo_list); > + > + if (res) > + return error_errno(_("could not write '%s'."), todo_file); > + return 0; > +} > + > +static int rearrange_squash_in_todo_file(void) > +{ > + const char *todo_file = rebase_path_todo(); > + struct todo_list todo_list = TODO_LIST_INIT; > + int res = 0; > + > + if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) > + return error_errno(_("could not read '%s'."), todo_file); > + if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf, > + &todo_list)) { > + todo_list_release(&todo_list); > + return error(_("unusable todo list: '%s'"), todo_file); > + } > + > + res = todo_list_rearrange_squash(&todo_list); > + if (!res) > + res = todo_list_write_to_file(the_repository, &todo_list, > + todo_file, NULL, NULL, -1, 0); > + > + todo_list_release(&todo_list); > + > + if (res) > + return error_errno(_("could not write '%s'."), todo_file); > + return 0; > +} > + > +static int transform_todo_file(unsigned flags) > +{ > + const char *todo_file = rebase_path_todo(); > + struct todo_list todo_list = TODO_LIST_INIT; > + int res; > + > + if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) > + return error_errno(_("could not read '%s'."), todo_file); > + > + if (todo_list_parse_insn_buffer(the_repository, todo_list.buf.buf, > + &todo_list)) { > + todo_list_release(&todo_list); > + return error(_("unusable todo list: '%s'"), todo_file); > + } > + > + res = todo_list_write_to_file(the_repository, &todo_list, todo_file, > + NULL, NULL, -1, flags); > + todo_list_release(&todo_list); > + > + if (res) > + return error_errno(_("could not write '%s'."), todo_file); > + return 0; > +} > + > static int edit_todo_file(unsigned flags) > { > const char *todo_file = rebase_path_todo(); > @@ -276,16 +351,16 @@ int cmd_rebase__interactive(int argc, const char > **argv, const char *prefix) > } > case SHORTEN_OIDS: > case EXPAND_OIDS: > - ret = transform_todo_file(the_repository, flags); > + ret = transform_todo_file(flags); > break; > case CHECK_TODO_LIST: > ret = check_todo_list_from_file(the_repository); > break; > case REARRANGE_SQUASH: > - ret = rearrange_squash_in_todo_file(the_repository); > + ret = rearrange_squash_in_todo_file(); > break; > case ADD_EXEC: > - ret = sequencer_add_exec_commands(the_repository, &commands); > + ret = add_exec_commands(&commands); > break; > default: > BUG("invalid command '%d'", command); > diff --git a/sequencer.c b/sequencer.c > index df8b239fdc..7d35f83f4c 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -4499,8 +4499,8 @@ int sequencer_make_script(struct repository *r, struct > strbuf *out, int argc, > * Add commands after pick and (series of) squash/fixup commands > * in the todo list. > */ > -static void todo_list_add_exec_commands(struct todo_list *todo_list, > - struct string_list *commands) > +void todo_list_add_exec_commands(struct todo_list *todo_list, > + struct string_list *commands) > { > struct strbuf *buf = &todo_list->buf; > size_t base_offset = buf->len; > @@ -4559,30 +4559,6 @@ static void todo_list_add_exec_commands(struct > todo_list *todo_list, > todo_list->alloc = alloc; > } > > -int sequencer_add_exec_commands(struct repository *r, > - struct string_list *commands) > -{ > - const char *todo_file = rebase_path_todo(); > - struct todo_list todo_list = TODO_LIST_INIT; > - int res; > - > - if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) > - return error_errno(_("could not read '%s'."), todo_file); > - > - if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { > - todo_list_release(&todo_list); > - return error(_("unusable todo list: '%s'"), todo_file); > - } > - > - todo_list_add_exec_commands(&todo_list, commands); > - res = todo_list_write_to_file(r, &todo_list, todo_file, NULL, NULL, -1, > 0); > - todo_list_release(&todo_list); > - > - if (res) > - return error_errno(_("could not write '%s'."), todo_file); > - return 0; > -} > - > static void todo_list_to_strbuf(struct repository *r, struct todo_list > *todo_list, > struct strbuf *buf, int num, unsigned flags) > { > @@ -4649,29 +4625,6 @@ int todo_list_write_to_file(struct repository *r, > struct todo_list *todo_list, > return res; > } > > -int transform_todo_file(struct repository *r, unsigned flags) > -{ > - const char *todo_file = rebase_path_todo(); > - struct todo_list todo_list = TODO_LIST_INIT; > - int res; > - > - if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) > - return error_errno(_("could not read '%s'."), todo_file); > - > - if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) { > - todo_list_release(&todo_list); > - return error(_("unusable todo list: '%s'"), todo_file); > - } > - > - res = todo_list_write_to_file(r, &todo_list, todo_file, > - NULL, NULL, -1, flags); > - todo_list_release(&todo_list); > - > - if (res) > - return error_errno(_("could not write '%s'."), todo_file); > - return 0; > -} > - > static const char edit_todo_list_advice[] = > N_("You can fix this with 'git rebase --edit-todo' " > "and then run 'git rebase --continue'.\n" > @@ -4754,8 +4707,6 @@ static int skip_unnecessary_picks(struct repository *r, > return 0; > } > > -static int todo_list_rearrange_squash(struct todo_list *todo_list); > - > int complete_action(struct repository *r, struct replay_opts *opts, unsigned > flags, > const char *shortrevisions, const char *onto_name, > const char *onto, const char *orig_head, struct string_list > *commands, > @@ -4862,7 +4813,7 @@ define_commit_slab(commit_todo_item, struct todo_item > *); > * message will have to be retrieved from the commit (as the oneline in the > * script cannot be trusted) in order to normalize the autosquash > arrangement. > */ > -static int todo_list_rearrange_squash(struct todo_list *todo_list) > +int todo_list_rearrange_squash(struct todo_list *todo_list) > { > struct hashmap subject2item; > int rearranged = 0, *next, *tail, i, nr = 0, alloc = 0; > @@ -5000,27 +4951,3 @@ static int todo_list_rearrange_squash(struct todo_list > *todo_list) > > return 0; > } > - > -int rearrange_squash_in_todo_file(struct repository *r) > -{ > - const char *todo_file = rebase_path_todo(); > - struct todo_list todo_list = TODO_LIST_INIT; > - int res = 0; > - > - if (strbuf_read_file_or_whine(&todo_list.buf, todo_file) < 0) > - return -1; > - if (todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list) < 0) { > - todo_list_release(&todo_list); > - return -1; > - } > - > - res = todo_list_rearrange_squash(&todo_list); > - if (!res) > - res = todo_list_write_to_file(r, &todo_list, todo_file, NULL, > NULL, -1, 0); > - > - todo_list_release(&todo_list); > - > - if (res) > - return error_errno(_("could not write '%s'."), todo_file); > - return 0; > -} > diff --git a/sequencer.h b/sequencer.h > index b0688ba2a1..7cca49eff2 100644 > --- a/sequencer.h > +++ b/sequencer.h > @@ -143,15 +143,14 @@ int sequencer_remove_state(struct replay_opts *opts); > int sequencer_make_script(struct repository *r, struct strbuf *out, int argc, > const char **argv, unsigned flags); > > -int sequencer_add_exec_commands(struct repository *r, > - struct string_list *commands); > -int transform_todo_file(struct repository *r, unsigned flags); > +void todo_list_add_exec_commands(struct todo_list *todo_list, > + struct string_list *commands); > int check_todo_list_from_file(struct repository *r); > int complete_action(struct repository *r, struct replay_opts *opts, unsigned > flags, > const char *shortrevisions, const char *onto_name, > const char *onto, const char *orig_head, struct string_list > *commands, > unsigned autosquash, struct todo_list *todo_list); > -int rearrange_squash_in_todo_file(struct repository *r); > +int todo_list_rearrange_squash(struct todo_list *todo_list); > > extern const char sign_off_header[]; > >
