| Message ID | 20190129150159.10588-15-alban.gruin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sequencer: refactor functions working on a todo_list | expand |
Hi Alban This looks good apart from some missing error handling. On 29/01/2019 15:01, Alban Gruin wrote: > edit_todo_list() is changed to work on a todo_list, and to handle the > initial edition of the todo list (ie. making a backup of the todo > list). > > It does not check for dropped commits yet, as todo_list_check() does not > take the commits that have already been processed by the rebase (ie. the > todo list is edited in the middle of a rebase session). > > Signed-off-by: Alban Gruin <alban.gruin@gmail.com> > --- > builtin/rebase--interactive.c | 24 +++++++++++++++++- > rebase-interactive.c | 48 ++++++++++++++++++----------------- > rebase-interactive.h | 4 ++- > sequencer.c | 3 +-- > sequencer.h | 1 + > 5 files changed, 53 insertions(+), 27 deletions(-) > > diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c > index 2dbf8fc08b..645ac587f7 100644 > --- a/builtin/rebase--interactive.c > +++ b/builtin/rebase--interactive.c > @@ -13,6 +13,28 @@ 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 edit_todo_file(unsigned flags) > +{ > + const char *todo_file = rebase_path_todo(); > + struct todo_list todo_list = TODO_LIST_INIT, > + new_todo = TODO_LIST_INIT; > + > + if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) > + return error_errno(_("could not read '%s'."), todo_file); > + > + strbuf_stripspace(&todo_list.buf, 1); > + if (!edit_todo_list(the_repository, &todo_list, > + &new_todo, NULL, NULL, flags) && > + todo_list_write_to_file(the_repository, &new_todo, todo_file, NULL, NULL, > + -1, flags & ~(TODO_LIST_SHORTEN_IDS)) < 0) > + return error_errno(_("could not write '%s'"), todo_file); If edit_todo_list() fails then the function returns 0. I think you need to do if (edit_todo_list() || todo_list_write_file()) return error... todo_list_write_file() forwards the return value of write_message() which is 0/-1 so there is no need for the '< 0' > + > + todo_list_release(&todo_list); > + todo_list_release(&new_todo); > + > + return 0; > +} > + > static int get_revision_ranges(const char *upstream, const char *onto, > const char **head_hash, > char **revisions, char **shortrevisions) > @@ -242,7 +264,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) > break; > } > case EDIT_TODO: > - ret = edit_todo_list(the_repository, flags); > + ret = edit_todo_file(flags); > break; > case SHOW_CURRENT_PATCH: { > struct child_process cmd = CHILD_PROCESS_INIT; > diff --git a/rebase-interactive.c b/rebase-interactive.c > index 807f8370db..3301efbe52 100644 > --- a/rebase-interactive.c > +++ b/rebase-interactive.c > @@ -87,35 +87,37 @@ void append_todo_help(unsigned keep_empty, int command_count, > } > } > > -int edit_todo_list(struct repository *r, unsigned flags) > +int edit_todo_list(struct repository *r, struct todo_list *todo_list, > + struct todo_list *new_todo, const char *shortrevisions, > + const char *shortonto, unsigned flags) > { > 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); > - > - strbuf_stripspace(&todo_list.buf, 1); > - todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list); > - if (todo_list_write_to_file(r, &todo_list, todo_file, NULL, NULL, -1, > - flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP)) { > - todo_list_release(&todo_list); > - return -1; > + unsigned initial = shortrevisions && shortonto; > + > + if (initial) { > + todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto, > + -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP); This has lost the error handling when we cannot write the file > + > + if (copy_file(rebase_path_todo_backup(), todo_file, 0666)) > + return error(_("could not copy '%s' to '%s'."), todo_file, > + rebase_path_todo_backup()); > + } else { > + todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list); > + todo_list_write_to_file(r, todo_list, todo_file, NULL, NULL, -1, > + flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP); error handling again > } > > - strbuf_reset(&todo_list.buf); > - if (launch_sequence_editor(todo_file, &todo_list.buf, NULL)) { > - todo_list_release(&todo_list); > - return -1; > - } > + if (launch_sequence_editor(todo_file, &new_todo->buf, NULL)) > + return -2; > > - if (!todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) > - res = todo_list_write_to_file(r, &todo_list, todo_file, NULL, NULL, -1, > - flags & ~(TODO_LIST_SHORTEN_IDS)); > + strbuf_stripspace(&new_todo->buf, 1); > + if (initial && new_todo->buf.len == 0) > + return -3; > > - todo_list_release(&todo_list); > - return res; > + if (!initial) > + todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo); error handling. Also why don't we try parse the file for the initial edit - is it done somewhere else? Best Wishes Phillip > + > + return 0; > } > > define_commit_slab(commit_seen, unsigned char); > diff --git a/rebase-interactive.h b/rebase-interactive.h > index 0e5925e3aa..44dbb06311 100644 > --- a/rebase-interactive.h > +++ b/rebase-interactive.h > @@ -8,7 +8,9 @@ struct todo_list; > void append_todo_help(unsigned keep_empty, int command_count, > const char *shortrevisions, const char *shortonto, > struct strbuf *buf); > -int edit_todo_list(struct repository *r, unsigned flags); > +int edit_todo_list(struct repository *r, struct todo_list *todo_list, > + struct todo_list *new_todo, const char *shortrevisions, > + const char *shortonto, unsigned flags); > int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo); > > #endif > diff --git a/sequencer.c b/sequencer.c > index 92de982bc4..8f47f0cf39 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -55,8 +55,7 @@ static GIT_PATH_FUNC(rebase_path, "rebase-merge") > * file and written to the tail of 'done'. > */ > GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo") > -static GIT_PATH_FUNC(rebase_path_todo_backup, > - "rebase-merge/git-rebase-todo.backup") > +GIT_PATH_FUNC(rebase_path_todo_backup, "rebase-merge/git-rebase-todo.backup") > > /* > * The rebase command lines that have already been processed. A line > diff --git a/sequencer.h b/sequencer.h > index c5bee8124c..68acab980b 100644 > --- a/sequencer.h > +++ b/sequencer.h > @@ -10,6 +10,7 @@ struct repository; > const char *git_path_commit_editmsg(void); > const char *git_path_seq_dir(void); > const char *rebase_path_todo(void); > +const char *rebase_path_todo_backup(void); > > #define APPEND_SIGNOFF_DEDUP (1u << 0) > >
Hi Phillip, Le 01/02/2019 à 12:03, Phillip Wood a écrit : >> } >> - strbuf_reset(&todo_list.buf); >> - if (launch_sequence_editor(todo_file, &todo_list.buf, NULL)) { >> - todo_list_release(&todo_list); >> - return -1; >> - } >> + if (launch_sequence_editor(todo_file, &new_todo->buf, NULL)) >> + return -2; >> - if (!todo_list_parse_insn_buffer(r, todo_list.buf.buf, >> &todo_list)) >> - res = todo_list_write_to_file(r, &todo_list, todo_file, NULL, >> NULL, -1, >> - flags & ~(TODO_LIST_SHORTEN_IDS)); >> + strbuf_stripspace(&new_todo->buf, 1); >> + if (initial && new_todo->buf.len == 0) >> + return -3; >> - todo_list_release(&todo_list); >> - return res; >> + if (!initial) >> + todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo); > > error handling. Also why don't we try parse the file for the initial > edit - is it done somewhere else? > Yes, it’s done in complete_action(). -- Alban > Best Wishes > > Phillip > >> + >> + return 0; >> } >> define_commit_slab(commit_seen, unsigned char); >> diff --git a/rebase-interactive.h b/rebase-interactive.h >> index 0e5925e3aa..44dbb06311 100644 >> --- a/rebase-interactive.h >> +++ b/rebase-interactive.h >> @@ -8,7 +8,9 @@ struct todo_list; >> void append_todo_help(unsigned keep_empty, int command_count, >> const char *shortrevisions, const char *shortonto, >> struct strbuf *buf); >> -int edit_todo_list(struct repository *r, unsigned flags); >> +int edit_todo_list(struct repository *r, struct todo_list *todo_list, >> + struct todo_list *new_todo, const char *shortrevisions, >> + const char *shortonto, unsigned flags); >> int todo_list_check(struct todo_list *old_todo, struct todo_list >> *new_todo); >> #endif >> diff --git a/sequencer.c b/sequencer.c >> index 92de982bc4..8f47f0cf39 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -55,8 +55,7 @@ static GIT_PATH_FUNC(rebase_path, "rebase-merge") >> * file and written to the tail of 'done'. >> */ >> GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo") >> -static GIT_PATH_FUNC(rebase_path_todo_backup, >> - "rebase-merge/git-rebase-todo.backup") >> +GIT_PATH_FUNC(rebase_path_todo_backup, >> "rebase-merge/git-rebase-todo.backup") >> /* >> * The rebase command lines that have already been processed. A line >> diff --git a/sequencer.h b/sequencer.h >> index c5bee8124c..68acab980b 100644 >> --- a/sequencer.h >> +++ b/sequencer.h >> @@ -10,6 +10,7 @@ struct repository; >> const char *git_path_commit_editmsg(void); >> const char *git_path_seq_dir(void); >> const char *rebase_path_todo(void); >> +const char *rebase_path_todo_backup(void); >> #define APPEND_SIGNOFF_DEDUP (1u << 0) >> >
Hi Phillip, I’ve just reread this message and have a couple of additionnal comments. Le 01/02/2019 à 12:03, Phillip Wood a écrit : > Hi Alban > > This looks good apart from some missing error handling. > > On 29/01/2019 15:01, Alban Gruin wrote: >> edit_todo_list() is changed to work on a todo_list, and to handle the >> initial edition of the todo list (ie. making a backup of the todo >> list). >> >> It does not check for dropped commits yet, as todo_list_check() does not >> take the commits that have already been processed by the rebase (ie. the >> todo list is edited in the middle of a rebase session). >> >> Signed-off-by: Alban Gruin <alban.gruin@gmail.com> >> --- >> builtin/rebase--interactive.c | 24 +++++++++++++++++- >> rebase-interactive.c | 48 ++++++++++++++++++----------------- >> rebase-interactive.h | 4 ++- >> sequencer.c | 3 +-- >> sequencer.h | 1 + >> 5 files changed, 53 insertions(+), 27 deletions(-) >> >> diff --git a/builtin/rebase--interactive.c >> b/builtin/rebase--interactive.c >> index 2dbf8fc08b..645ac587f7 100644 >> --- a/builtin/rebase--interactive.c >> +++ b/builtin/rebase--interactive.c >> @@ -13,6 +13,28 @@ 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 edit_todo_file(unsigned flags) >> +{ >> + const char *todo_file = rebase_path_todo(); >> + struct todo_list todo_list = TODO_LIST_INIT, >> + new_todo = TODO_LIST_INIT; >> + >> + if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) >> + return error_errno(_("could not read '%s'."), todo_file); >> + >> + strbuf_stripspace(&todo_list.buf, 1); >> + if (!edit_todo_list(the_repository, &todo_list, >> + &new_todo, NULL, NULL, flags) && >> + todo_list_write_to_file(the_repository, &new_todo, todo_file, >> NULL, NULL, >> + -1, flags & ~(TODO_LIST_SHORTEN_IDS)) < 0) >> + return error_errno(_("could not write '%s'"), todo_file); > > If edit_todo_list() fails then the function returns 0. I think you need > to do > > if (edit_todo_list() || todo_list_write_file()) > return error... > > todo_list_write_file() forwards the return value of write_message() > which is 0/-1 so there is no need for the '< 0' > With your proposed condition, if edit_todo_list() fails, the error "could not write '%s'" will be shown, if I’m not mistaken. But in my version, if edit_todo_list() fails, the return value is 0. Perhaps I should write something like this instead: int res = 0; … res = edit_todo_list(); if (!res && todo_list_write_to_file()) return error; … return res; >> + >> + todo_list_release(&todo_list); >> + todo_list_release(&new_todo); >> + >> + return 0; >> +} >> + >> static int get_revision_ranges(const char *upstream, const char *onto, >> const char **head_hash, >> char **revisions, char **shortrevisions) >> @@ -242,7 +264,7 @@ int cmd_rebase__interactive(int argc, const char >> **argv, const char *prefix) >> break; >> } >> case EDIT_TODO: >> - ret = edit_todo_list(the_repository, flags); >> + ret = edit_todo_file(flags); >> break; >> case SHOW_CURRENT_PATCH: { >> struct child_process cmd = CHILD_PROCESS_INIT; >> diff --git a/rebase-interactive.c b/rebase-interactive.c >> index 807f8370db..3301efbe52 100644 >> --- a/rebase-interactive.c >> +++ b/rebase-interactive.c >> @@ -87,35 +87,37 @@ void append_todo_help(unsigned keep_empty, int >> command_count, >> } >> } >> -int edit_todo_list(struct repository *r, unsigned flags) >> +int edit_todo_list(struct repository *r, struct todo_list *todo_list, >> + struct todo_list *new_todo, const char *shortrevisions, >> + const char *shortonto, unsigned flags) >> { >> 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); >> - >> - strbuf_stripspace(&todo_list.buf, 1); >> - todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list); >> - if (todo_list_write_to_file(r, &todo_list, todo_file, NULL, NULL, >> -1, >> - flags | TODO_LIST_SHORTEN_IDS | >> TODO_LIST_APPEND_TODO_HELP)) { >> - todo_list_release(&todo_list); >> - return -1; >> + unsigned initial = shortrevisions && shortonto; >> + >> + if (initial) { >> + todo_list_write_to_file(r, todo_list, todo_file, >> shortrevisions, shortonto, >> + -1, flags | TODO_LIST_SHORTEN_IDS | >> TODO_LIST_APPEND_TODO_HELP); > > This has lost the error handling when we cannot write the file > >> + >> + if (copy_file(rebase_path_todo_backup(), todo_file, 0666)) >> + return error(_("could not copy '%s' to '%s'."), todo_file, >> + rebase_path_todo_backup()); >> + } else { >> + todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list); >> + todo_list_write_to_file(r, todo_list, todo_file, NULL, NULL, -1, >> + flags | TODO_LIST_SHORTEN_IDS | >> TODO_LIST_APPEND_TODO_HELP); > > error handling again > I agree for todo_list_write_to_file(), but todo_list_parse_insn_buffer() already shows an error, and here it should not return -- we want to edit the todo list to remove an error, but git would fail because the todo list has an error. -- Alban
Hi Alban On 06/02/2019 21:11, Alban Gruin wrote: > Hi Phillip, > > I’ve just reread this message and have a couple of additionnal comments. > > Le 01/02/2019 à 12:03, Phillip Wood a écrit : >> Hi Alban >> >> This looks good apart from some missing error handling. >> >> On 29/01/2019 15:01, Alban Gruin wrote: >>> edit_todo_list() is changed to work on a todo_list, and to handle the >>> initial edition of the todo list (ie. making a backup of the todo >>> list). >>> >>> It does not check for dropped commits yet, as todo_list_check() does not >>> take the commits that have already been processed by the rebase (ie. the >>> todo list is edited in the middle of a rebase session). >>> >>> Signed-off-by: Alban Gruin <alban.gruin@gmail.com> >>> --- >>> builtin/rebase--interactive.c | 24 +++++++++++++++++- >>> rebase-interactive.c | 48 ++++++++++++++++++----------------- >>> rebase-interactive.h | 4 ++- >>> sequencer.c | 3 +-- >>> sequencer.h | 1 + >>> 5 files changed, 53 insertions(+), 27 deletions(-) >>> >>> diff --git a/builtin/rebase--interactive.c >>> b/builtin/rebase--interactive.c >>> index 2dbf8fc08b..645ac587f7 100644 >>> --- a/builtin/rebase--interactive.c >>> +++ b/builtin/rebase--interactive.c >>> @@ -13,6 +13,28 @@ 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 edit_todo_file(unsigned flags) >>> +{ >>> + const char *todo_file = rebase_path_todo(); >>> + struct todo_list todo_list = TODO_LIST_INIT, >>> + new_todo = TODO_LIST_INIT; >>> + >>> + if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) >>> + return error_errno(_("could not read '%s'."), todo_file); >>> + >>> + strbuf_stripspace(&todo_list.buf, 1); >>> + if (!edit_todo_list(the_repository, &todo_list, >>> + &new_todo, NULL, NULL, flags) && >>> + todo_list_write_to_file(the_repository, &new_todo, todo_file, >>> NULL, NULL, >>> + -1, flags & ~(TODO_LIST_SHORTEN_IDS)) < 0) >>> + return error_errno(_("could not write '%s'"), todo_file); >> >> If edit_todo_list() fails then the function returns 0. I think you need >> to do >> >> if (edit_todo_list() || todo_list_write_file()) >> return error... >> >> todo_list_write_file() forwards the return value of write_message() >> which is 0/-1 so there is no need for the '< 0' >> > > With your proposed condition, if edit_todo_list() fails, the error > "could not write '%s'" will be shown, if I’m not mistaken. Yes, you're right but as edit_todo_list() will have already printed an error I decided it didn't matter too much, but it would be better to avoid it as you suggest. > But in my > version, if edit_todo_list() fails, the return value is 0. Perhaps I > should write something like this instead: > > int res = 0; > … > res = edit_todo_list(); > if (!res && todo_list_write_to_file()) > return error; If you did ret = error...; instead then we always free the todo lists before exiting the function. > … > return res; >>> + >>> + todo_list_release(&todo_list); >>> + todo_list_release(&new_todo); >>> + >>> + return 0; >>> +} >>> + >>> static int get_revision_ranges(const char *upstream, const char *onto, >>> const char **head_hash, >>> char **revisions, char **shortrevisions) >>> @@ -242,7 +264,7 @@ int cmd_rebase__interactive(int argc, const char >>> **argv, const char *prefix) >>> break; >>> } >>> case EDIT_TODO: >>> - ret = edit_todo_list(the_repository, flags); >>> + ret = edit_todo_file(flags); >>> break; >>> case SHOW_CURRENT_PATCH: { >>> struct child_process cmd = CHILD_PROCESS_INIT; >>> diff --git a/rebase-interactive.c b/rebase-interactive.c >>> index 807f8370db..3301efbe52 100644 >>> --- a/rebase-interactive.c >>> +++ b/rebase-interactive.c >>> @@ -87,35 +87,37 @@ void append_todo_help(unsigned keep_empty, int >>> command_count, >>> } >>> } >>> -int edit_todo_list(struct repository *r, unsigned flags) >>> +int edit_todo_list(struct repository *r, struct todo_list *todo_list, >>> + struct todo_list *new_todo, const char *shortrevisions, >>> + const char *shortonto, unsigned flags) >>> { >>> 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); >>> - >>> - strbuf_stripspace(&todo_list.buf, 1); >>> - todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list); >>> - if (todo_list_write_to_file(r, &todo_list, todo_file, NULL, NULL, >>> -1, >>> - flags | TODO_LIST_SHORTEN_IDS | >>> TODO_LIST_APPEND_TODO_HELP)) { >>> - todo_list_release(&todo_list); >>> - return -1; >>> + unsigned initial = shortrevisions && shortonto; >>> + >>> + if (initial) { >>> + todo_list_write_to_file(r, todo_list, todo_file, >>> shortrevisions, shortonto, >>> + -1, flags | TODO_LIST_SHORTEN_IDS | >>> TODO_LIST_APPEND_TODO_HELP); >> >> This has lost the error handling when we cannot write the file >> >>> + >>> + if (copy_file(rebase_path_todo_backup(), todo_file, 0666)) >>> + return error(_("could not copy '%s' to '%s'."), todo_file, >>> + rebase_path_todo_backup()); >>> + } else { >>> + todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list); >>> + todo_list_write_to_file(r, todo_list, todo_file, NULL, NULL, -1, >>> + flags | TODO_LIST_SHORTEN_IDS | >>> TODO_LIST_APPEND_TODO_HELP); >> >> error handling again >> > > I agree for todo_list_write_to_file(), but todo_list_parse_insn_buffer() > already shows an error, and here it should not return -- we want to edit > the todo list to remove an error, but git would fail because the todo > list has an error. Ah yes, that is what the original was doing Best Wishes Phillip > > -- Alban >
diff --git a/builtin/rebase--interactive.c b/builtin/rebase--interactive.c index 2dbf8fc08b..645ac587f7 100644 --- a/builtin/rebase--interactive.c +++ b/builtin/rebase--interactive.c @@ -13,6 +13,28 @@ 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 edit_todo_file(unsigned flags) +{ + const char *todo_file = rebase_path_todo(); + struct todo_list todo_list = TODO_LIST_INIT, + new_todo = TODO_LIST_INIT; + + if (strbuf_read_file(&todo_list.buf, todo_file, 0) < 0) + return error_errno(_("could not read '%s'."), todo_file); + + strbuf_stripspace(&todo_list.buf, 1); + if (!edit_todo_list(the_repository, &todo_list, + &new_todo, NULL, NULL, flags) && + todo_list_write_to_file(the_repository, &new_todo, todo_file, NULL, NULL, + -1, flags & ~(TODO_LIST_SHORTEN_IDS)) < 0) + return error_errno(_("could not write '%s'"), todo_file); + + todo_list_release(&todo_list); + todo_list_release(&new_todo); + + return 0; +} + static int get_revision_ranges(const char *upstream, const char *onto, const char **head_hash, char **revisions, char **shortrevisions) @@ -242,7 +264,7 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix) break; } case EDIT_TODO: - ret = edit_todo_list(the_repository, flags); + ret = edit_todo_file(flags); break; case SHOW_CURRENT_PATCH: { struct child_process cmd = CHILD_PROCESS_INIT; diff --git a/rebase-interactive.c b/rebase-interactive.c index 807f8370db..3301efbe52 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -87,35 +87,37 @@ void append_todo_help(unsigned keep_empty, int command_count, } } -int edit_todo_list(struct repository *r, unsigned flags) +int edit_todo_list(struct repository *r, struct todo_list *todo_list, + struct todo_list *new_todo, const char *shortrevisions, + const char *shortonto, unsigned flags) { 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); - - strbuf_stripspace(&todo_list.buf, 1); - todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list); - if (todo_list_write_to_file(r, &todo_list, todo_file, NULL, NULL, -1, - flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP)) { - todo_list_release(&todo_list); - return -1; + unsigned initial = shortrevisions && shortonto; + + if (initial) { + todo_list_write_to_file(r, todo_list, todo_file, shortrevisions, shortonto, + -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP); + + if (copy_file(rebase_path_todo_backup(), todo_file, 0666)) + return error(_("could not copy '%s' to '%s'."), todo_file, + rebase_path_todo_backup()); + } else { + todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list); + todo_list_write_to_file(r, todo_list, todo_file, NULL, NULL, -1, + flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP); } - strbuf_reset(&todo_list.buf); - if (launch_sequence_editor(todo_file, &todo_list.buf, NULL)) { - todo_list_release(&todo_list); - return -1; - } + if (launch_sequence_editor(todo_file, &new_todo->buf, NULL)) + return -2; - if (!todo_list_parse_insn_buffer(r, todo_list.buf.buf, &todo_list)) - res = todo_list_write_to_file(r, &todo_list, todo_file, NULL, NULL, -1, - flags & ~(TODO_LIST_SHORTEN_IDS)); + strbuf_stripspace(&new_todo->buf, 1); + if (initial && new_todo->buf.len == 0) + return -3; - todo_list_release(&todo_list); - return res; + if (!initial) + todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo); + + return 0; } define_commit_slab(commit_seen, unsigned char); diff --git a/rebase-interactive.h b/rebase-interactive.h index 0e5925e3aa..44dbb06311 100644 --- a/rebase-interactive.h +++ b/rebase-interactive.h @@ -8,7 +8,9 @@ struct todo_list; void append_todo_help(unsigned keep_empty, int command_count, const char *shortrevisions, const char *shortonto, struct strbuf *buf); -int edit_todo_list(struct repository *r, unsigned flags); +int edit_todo_list(struct repository *r, struct todo_list *todo_list, + struct todo_list *new_todo, const char *shortrevisions, + const char *shortonto, unsigned flags); int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo); #endif diff --git a/sequencer.c b/sequencer.c index 92de982bc4..8f47f0cf39 100644 --- a/sequencer.c +++ b/sequencer.c @@ -55,8 +55,7 @@ static GIT_PATH_FUNC(rebase_path, "rebase-merge") * file and written to the tail of 'done'. */ GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo") -static GIT_PATH_FUNC(rebase_path_todo_backup, - "rebase-merge/git-rebase-todo.backup") +GIT_PATH_FUNC(rebase_path_todo_backup, "rebase-merge/git-rebase-todo.backup") /* * The rebase command lines that have already been processed. A line diff --git a/sequencer.h b/sequencer.h index c5bee8124c..68acab980b 100644 --- a/sequencer.h +++ b/sequencer.h @@ -10,6 +10,7 @@ struct repository; const char *git_path_commit_editmsg(void); const char *git_path_seq_dir(void); const char *rebase_path_todo(void); +const char *rebase_path_todo_backup(void); #define APPEND_SIGNOFF_DEDUP (1u << 0)
edit_todo_list() is changed to work on a todo_list, and to handle the initial edition of the todo list (ie. making a backup of the todo list). It does not check for dropped commits yet, as todo_list_check() does not take the commits that have already been processed by the rebase (ie. the todo list is edited in the middle of a rebase session). Signed-off-by: Alban Gruin <alban.gruin@gmail.com> --- builtin/rebase--interactive.c | 24 +++++++++++++++++- rebase-interactive.c | 48 ++++++++++++++++++----------------- rebase-interactive.h | 4 ++- sequencer.c | 3 +-- sequencer.h | 1 + 5 files changed, 53 insertions(+), 27 deletions(-)