| Message ID | 20230323162235.995574-9-oswald.buddenhagen@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sequencer refactoring | expand |
Hi Oswald On 23/03/2023 16:22, Oswald Buddenhagen wrote: > When the user butchers the todo file during rebase -i setup, the > --continue which would follow --edit-todo would have skipped the last > steps of the setup. Notably, this would bypass the fast-forward over > untouched picks (though the actual picking loop would still fast-forward > the commits, one by one). > > Fix this by splitting off the tail of complete_action() to a new > start_rebase() function and call that from sequencer_continue() when no > commands have been executed yet. > > More or less as a side effect, we no longer checkout `onto` before exiting > when the todo file is bad. I think the implications of this change deserve to be discussed in the commit message. Three things spring to mind but there may be others I haven't thought of - Previously when rebase stopped and handed control back to the user HEAD would have already been detached. This patch changes that meaning we can have an active rebase of a branch while that branch is checked out. What does "git status" show in this case? What does the shell prompt show? Will it confuse users? - Previously if the user created a commit before running "rebase --continue" we'd rebase on to that commit. Now that commit will be silently dropped. - Previously if the user checkout out another commit before running "rebase --continue" we'd rebase on to that commit. Now we we rebase on to the original "onto" commit. > This makes aborting cheaper and will simplify > things in a later change. Given that we're stopping so the user can fix the problem and continue the rebase I don't think optimizing for aborting is a convincing reason for this change on its own. > diff --git a/builtin/revert.c b/builtin/revert.c > index 62986a7b1b..00d3e19c62 100644 > --- a/builtin/revert.c > +++ b/builtin/revert.c > @@ -231,7 +231,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) > return ret; > } > if (cmd == 'c') > - return sequencer_continue(the_repository, opts); > + return sequencer_continue(the_repository, opts, > + 0, NULL, NULL, NULL); It's a bit unfortunate that we have to start passing all these extra parameters, could the sequencer read them itself in read_populate_opts()? > -int sequencer_continue(struct repository *r, struct replay_opts *opts) > +static int start_rebase(struct repository *r, struct replay_opts *opts, unsigned flags, > + const char *onto_name, const struct object_id *onto, > + const struct object_id *orig_head, struct todo_list *todo_list); It would be nice to avoid this forward declaration. I think you could do that by adding a preparatory patch that moves either checkout_onto() or sequencer_continue() > @@ -6142,49 +6154,52 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla > > return error(_("nothing to do")); > } else if (res == EDIT_TODO_INCORRECT) { > - checkout_onto(r, opts, onto_name, onto, orig_head); > todo_list_release(&new_todo); > > return -1; > } > > - /* Expand the commit IDs */ > - todo_list_to_strbuf(r, &new_todo, &buf2, -1, 0); > - strbuf_swap(&new_todo.buf, &buf2); > - strbuf_release(&buf2); > - new_todo.total_nr -= new_todo.nr; > - if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0) > - BUG("invalid todo list after expanding IDs:\n%s", > - new_todo.buf.buf); I don't think we need to move this code. If start_rebase() is called from sequencer_continue() the initial edit of the todo list failed and has been fixed by running "git rebase --edit-todo". In that case the oids have already been expanded on disc. > - if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &onto)) { > - todo_list_release(&new_todo); > - return error(_("could not skip unnecessary pick commands")); > - } > - > - if (todo_list_write_to_file(r, &new_todo, todo_file, NULL, NULL, -1, > - flags & ~(TODO_LIST_SHORTEN_IDS), action)) { > - todo_list_release(&new_todo); > - return error_errno(_("could not write '%s'"), todo_file); > - } > - > - res = -1; > - > - if (checkout_onto(r, opts, onto_name, onto, orig_head)) > - goto cleanup; > - > - if (require_clean_work_tree(r, "rebase", NULL, 1, 1)) > - goto cleanup; > - > - todo_list_write_total_nr(&new_todo); > - res = pick_commits(r, &new_todo, opts); > - > -cleanup: > + res = start_rebase(r, opts, flags, onto_name, onto, orig_head, &new_todo); > todo_list_release(&new_todo); > > return res; > } > > +test_expect_success 'continue after bad first command' ' > + test_when_finished "git rebase --abort ||:" && > + git checkout primary^0 && If you want a specific commit it's better to use a tag name as those are fixed whereas the branches get rebased all over the place in this test file. > + git reflog expire --expire=all HEAD && Is this really necessary, can you pass -n to "git reflog" below? > + ( > + set_fake_editor && > + test_must_fail env FAKE_LINES="bad 1 pick 1 pick 2 reword 3" \ > + git rebase -i HEAD~3 && > + test_cmp_rev HEAD primary && > + FAKE_LINES="pick 2 pick 3 reword 4" git rebase --edit-todo && > + FAKE_COMMIT_MESSAGE="E_reworded" git rebase --continue > + ) && > + git reflog > reflog && > + test $(grep -c fast-forward reflog) = 1 && Using test_line_count would make test failures easier to debug. > + test_cmp_rev HEAD~1 primary~1 && > + test "$(git log -1 --format=%B)" = "E_reworded" It is slightly more work, but please use test_cmp for things like this as it makes it so much easier to debug test failures. Best Wishes Phillip > +' > + > +test_expect_success 'abort after bad first command' ' > + test_when_finished "git rebase --abort ||:" && > + git checkout primary^0 && > + ( > + set_fake_editor && > + test_must_fail env FAKE_LINES="bad 1 pick 1 pick 2 reword 3" \ > + git rebase -i HEAD~3 > + ) && > + git rebase --abort && > + test_cmp_rev HEAD primary > +' > + > test_expect_success 'tabs and spaces are accepted in the todolist' ' > rebase_setup_and_clean indented-comment && > write_script add-indent.sh <<-\EOF &&
On Sun, Mar 26, 2023 at 03:28:01PM +0100, Phillip Wood wrote: >On 23/03/2023 16:22, Oswald Buddenhagen wrote: >> When the user butchers the todo file during rebase -i setup, the >> --continue which would follow --edit-todo would have skipped the last >> steps of the setup. Notably, this would bypass the fast-forward over >> untouched picks (though the actual picking loop would still fast-forward >> the commits, one by one). >> >> Fix this by splitting off the tail of complete_action() to a new >> start_rebase() function and call that from sequencer_continue() when no >> commands have been executed yet. >> >> More or less as a side effect, we no longer checkout `onto` before exiting >> when the todo file is bad. > >I think the implications of this change deserve to be discussed in the >commit message. Three things spring to mind but there may be others I >haven't thought of > > - Previously when rebase stopped and handed control back to the user > HEAD would have already been detached. This patch changes that > meaning we can have an active rebase of a branch while that branch is > checked out. What does "git status" show in this case? What does the > shell prompt show? Will it confuse users? > the failed state is identical to the "still editing the initial todo" state as far as "git status" and the shell prompt are concerned. this seems reasonable. i'll add it to the commit message. > - Previously if the user created a commit before running "rebase > --continue" we'd rebase on to that commit. Now that commit will be > silently dropped. > this is arguably a problem, but not much different from the pre-existing behavior of changes to HEAD done during the initial todo edit being lost. to avoid that, we'd need to lock HEAD while editing the todo. is that realistic at all? on top of that, i should verify HEAD against orig-head in start_rebase(). though the only way for the user to get out of that situation is saving the todo contents and --abort'ing (and we must take care not the touch HEAD). this is somewhat similar to the abysmal situation of the final update-ref failing if the target ref has been modified while being rebased. we'd need to lock that ref for the entire duration of the rebase to avoid that. > - Previously if the user checkout out another commit before running > "rebase --continue" we'd rebase on to that commit. Now we we rebase > on to the original "onto" commit. > this can be subsumed into the above case. > > This makes aborting cheaper and will simplify > > things in a later change. > >Given that we're stopping so the user can fix the problem and continue >the rebase I don't think optimizing for aborting is a convincing reason >for this change on its own. > this is all part of the "More or less as a side effect" paragraph, so this isn't a relevant objection. >> diff --git a/builtin/revert.c b/builtin/revert.c >> index 62986a7b1b..00d3e19c62 100644 >> --- a/builtin/revert.c >> +++ b/builtin/revert.c >> @@ -231,7 +231,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) >> return ret; >> } >> if (cmd == 'c') >> - return sequencer_continue(the_repository, opts); >> + return sequencer_continue(the_repository, opts, >> + 0, NULL, NULL, NULL); > >It's a bit unfortunate that we have to start passing all these extra >parameters, could the sequencer read them itself in read_populate_opts()? > that wouldn't help in this case, as these are dummy values which aren't going to be used. but more broadly, the whole state management is a total mess. i have this notes-to-self patch on top of my local branch: --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -476,6 +476,7 @@ static const char *state_dir_path(const char *filename, struct rebase_options *o } /* Initialize the rebase options from the state directory. */ +// FIXME: this is partly redundant with the sequencer's read_populate_opts(). static int read_basic_state(struct rebase_options *opts) { struct strbuf head_name = STRBUF_INIT; @@ -552,6 +553,7 @@ static int read_basic_state(struct rebase_options *opts) return 0; } +// This is written only by the apply backend static int rebase_write_basic_state(struct rebase_options *opts) { write_file(state_dir_path("head-name", opts), "%s", >> -int sequencer_continue(struct repository *r, struct replay_opts *opts) >> +static int start_rebase(struct repository *r, struct replay_opts *opts, unsigned flags, >> + const char *onto_name, const struct object_id *onto, >> + const struct object_id *orig_head, struct todo_list *todo_list); > >It would be nice to avoid this forward declaration. I think you could do >that by adding a preparatory patch that moves either checkout_onto() or >sequencer_continue() > i went for the "minimal churn" approach. but more broadly, the code distribution between rebase.c and sequencer.c needs a *major* re-think. moving these functions into place could be part of that effort. >> + git reflog expire --expire=all HEAD && > >Is this really necessary, can you pass -n to "git reflog" below? > starting from a clean slate makes it more straight-forward to make it reliable. i don't see any real downsides to the approach. >> + git reflog > reflog && >> + test $(grep -c fast-forward reflog) = 1 && > >Using test_line_count would make test failures easier to debug. > that's calling for a new test_filtered_line_count function which would have quite some users. for the time being, both grep + test_line_count and grep -c are rather prevalent, in this file the latter in particular. >> + test_cmp_rev HEAD~1 primary~1 && >> + test "$(git log -1 --format=%B)" = "E_reworded" > >It is slightly more work, but please use test_cmp for things like this >as it makes it so much easier to debug test failures. > fair enough, but the precedents again speak a different language. regards
Hi Oswald On 26/04/2023 16:34, Oswald Buddenhagen wrote: > On Sun, Mar 26, 2023 at 03:28:01PM +0100, Phillip Wood wrote: >> On 23/03/2023 16:22, Oswald Buddenhagen wrote: >>> When the user butchers the todo file during rebase -i setup, the >>> --continue which would follow --edit-todo would have skipped the last >>> steps of the setup. Notably, this would bypass the fast-forward over >>> untouched picks (though the actual picking loop would still fast-forward >>> the commits, one by one). >>> >>> Fix this by splitting off the tail of complete_action() to a new >>> start_rebase() function and call that from sequencer_continue() when no >>> commands have been executed yet. >>> >>> More or less as a side effect, we no longer checkout `onto` before >>> exiting >>> when the todo file is bad. >> >> I think the implications of this change deserve to be discussed in the >> commit message. Three things spring to mind but there may be others I >> haven't thought of >> >> - Previously when rebase stopped and handed control back to the user >> HEAD would have already been detached. This patch changes that >> meaning we can have an active rebase of a branch while that branch is >> checked out. What does "git status" show in this case? What does the >> shell prompt show? Will it confuse users? >> > the failed state is identical to the "still editing the initial todo" > state as far as "git status" and the shell prompt are concerned. this > seems reasonable. i'll add it to the commit message. When you do that please mention what "git status" and the shell prompt actually print in this case. Ideally "git status" should mention that the todo list needs to be edited if there are still errors in it, though it would not surprise me if it is not that helpful at the moment. >> - Previously if the user created a commit before running "rebase >> --continue" we'd rebase on to that commit. Now that commit will be >> silently dropped. >> > this is arguably a problem, but not much different from the pre-existing > behavior of changes to HEAD done during the initial todo edit being lost. I think there is a significant difference in that we're moving from a situation where we lose commits that are created while rebase is running to one where we're losing commits created while rebase is stopped. If a user tries to create a commit while rebase is running then they're asking for trouble. I don't think creating commits when rebase is stopped is unreasonable in the same way. > to avoid that, we'd need to lock HEAD while editing the todo. is that > realistic at all? I don't think it is practical to lock HEAD while git is not running. We could just check HEAD has not changed when the rebase continues after the user has fixed the todo list as you suggest below. > on top of that, i should verify HEAD against orig-head in > start_rebase(). though the only way for the user to get out of that > situation is saving the todo contents and --abort'ing (and we must take > care not the touch HEAD). I think in that case it wouldn't be terrible to lose the edited todo list as it is a bit of a corner case. The simplest thing to do would be to print an error and remove .git/rebase-merge. > this is somewhat similar to the abysmal situation of the final > update-ref failing if the target ref has been modified while being > rebased. we'd need to lock that ref for the entire duration of the > rebase to avoid that. "abysmal" is rather harsh - it would also be bad to overwrite the ref in that case. I think it in relatively hard to get into that situation though as "git checkout" wont checkout a branch that is being updated by a rebase. >> - Previously if the user checkout out another commit before running >> "rebase --continue" we'd rebase on to that commit. Now we we rebase >> on to the original "onto" commit. >> > this can be subsumed into the above case. Meaning check and error out if HEAD has changed? >> > This makes aborting cheaper and will simplify >> > things in a later change. >> >> Given that we're stopping so the user can fix the problem and continue >> the rebase I don't think optimizing for aborting is a convincing >> reason for this change on its own. >> > this is all part of the "More or less as a side effect" paragraph, so > this isn't a relevant objection. I'm simply saying that we should not be optimizing for "rebase --abort" in this case. Do you think we should? >>> diff --git a/builtin/revert.c b/builtin/revert.c >>> index 62986a7b1b..00d3e19c62 100644 >>> --- a/builtin/revert.c >>> +++ b/builtin/revert.c >>> @@ -231,7 +231,8 @@ static int run_sequencer(int argc, const char >>> **argv, struct replay_opts *opts) >>> return ret; >>> } >>> if (cmd == 'c') >>> - return sequencer_continue(the_repository, opts); >>> + return sequencer_continue(the_repository, opts, >>> + 0, NULL, NULL, NULL); >> >> It's a bit unfortunate that we have to start passing all these extra >> parameters, could the sequencer read them itself in read_populate_opts()? >> > that wouldn't help in this case, as these are dummy values which aren't > going to be used. If we only need to pass these when rebasing maybe we should have separate wrappers for continuing a rebase and a cherry-pick/revert. If we don't always need these parameters when continuing a rebase we could have a separate function when these parameters are required and leave the signature of sequencer_continue() unchanged. > but more broadly, the whole state management is a total mess. For historic reasons there are separate functions to write the state for the "merge" backed and the "apply" backend. That is not ideal but it is hardly a "total mess". The code for reading the state files is more contrived than the code that writes them. I do have some patches to try and reduce the duplication when reading the state files. >>> -int sequencer_continue(struct repository *r, struct replay_opts *opts) >>> +static int start_rebase(struct repository *r, struct replay_opts >>> *opts, unsigned flags, >>> + const char *onto_name, const struct object_id *onto, >>> + const struct object_id *orig_head, struct todo_list >>> *todo_list); >> >> It would be nice to avoid this forward declaration. I think you could >> do that by adding a preparatory patch that moves either >> checkout_onto() or sequencer_continue() >> > i went for the "minimal churn" approach. There is a balance to be had, but we don't want to build up a lot of forward declarations over time just because it is easier than moving the function in a preparatory patch. A simple patch to move a function is easy to review with --color-moved. >>> + git reflog > reflog && >>> + test $(grep -c fast-forward reflog) = 1 && >> >> Using test_line_count would make test failures easier to debug. >> > that's calling for a new test_filtered_line_count function which would > have quite some users. > for the time being, both grep + test_line_count and grep -c are rather > prevalent, in this file the latter in particular. The style of our tests has evolved over time. When adding new tests it is better to focus on making them easy to debug. I don't think you need to add a new function here, just grep fast-forward reflog >filtered-reflog test_line_count = 1 filtered-reflog >>> + test_cmp_rev HEAD~1 primary~1 && >>> + test "$(git log -1 --format=%B)" = "E_reworded" >> >> It is slightly more work, but please use test_cmp for things like this >> as it makes it so much easier to debug test failures. >> > fair enough, but the precedents again speak a different language. Yes older tests tend to be written in a style that is harder to debug. Best Wishes Phillip
On Wed, May 17, 2023 at 01:13:28PM +0100, Phillip Wood wrote: >On 26/04/2023 16:34, Oswald Buddenhagen wrote: >> the failed state is identical to the "still editing the initial todo" >> state as far as "git status" and the shell prompt are concerned. this >> seems reasonable. i'll add it to the commit message. > >When you do that please mention what "git status" and the shell prompt >actually print in this case. > i'll go with "This seems reasonable, irrespective of the actual presentation (which could be improved)". >Ideally "git status" should mention that the todo list needs to be >edited if there are still errors in it, though it would not surprise me >if it is not that helpful at the moment. > that would require actually validating the todo instead of just printing it. or maybe the presence of the backup file could be used to make reliable inferences. have fun! ;) >>> - Previously if the user created a commit before running "rebase >>> --continue" we'd rebase on to that commit. Now that commit will be >>> silently dropped. >>> >> this is arguably a problem, but not much different from the pre-existing >> behavior of changes to HEAD done during the initial todo edit being lost. > >I think there is a significant difference in that we're moving from a >situation where we lose commits that are created while rebase is running >to one where we're losing commits created while rebase is stopped. If a >user tries to create a commit while rebase is running then they're >asking for trouble. I don't think creating commits when rebase is >stopped is unreasonable in the same way. > i think that this is a completely meaningless distinction. a rebase is "running" while the state directory exists. having multiple terminals open is the norm, and when havoc ensues it doesn't matter to the user whether one of the terminals had an editor launched by git open at the time. >> to avoid that, we'd need to lock HEAD while editing the todo. is that >> realistic at all? > >I don't think it is practical to lock HEAD while git is not running. > what measure of "practical" are you applying? i'm assuming that no persistent locking infra exists currently. but i don't see a reason why it _couldn't_ - having some functions to populate and query .git/locked-refs/** in the right places doesn't seem like a fundamentally hard problem. >We could just check HEAD has not changed when the rebase continues >after the user has fixed the todo list as you suggest below. > that's a good safeguard which i intend to implement, but when it triggers, the user will have to deal with the conflict. it would be much nicer to avoid it in the first place. >> on top of that, i should verify HEAD against orig-head in >> start_rebase(). though the only way for the user to get out of that >> situation is saving the todo contents and --abort'ing (and we must >> take care not the touch HEAD). > >I think in that case it wouldn't be terrible to lose the edited todo >list as it is a bit of a corner case. > actually, yes, it would be. that's why i posted a patch that avoids it. >> this is somewhat similar to the abysmal situation of the final >> update-ref failing if the target ref has been modified while being >> rebased. we'd need to lock that ref for the entire duration of the >> rebase to avoid that. > >"abysmal" is rather harsh - it would also be bad to overwrite the ref in >that case. I think it in relatively hard to get into that situation >though as "git checkout" wont checkout a branch that is being updated by >a rebase. > i have no clue how it happened (certainly something to do with many open terminals), but i actually got into that situation shortly before writing that mail, and i assure you that "abysmal" is absolutely not an overstatement. i mean, what do you expect a user to think when presented with two diverging heads when trying to finish a rebase? >>> - Previously if the user checkout out another commit before running >>> "rebase --continue" we'd rebase on to that commit. Now we we rebase >>> on to the original "onto" commit. >>> >> this can be subsumed into the above case. > >Meaning check and error out if HEAD has changed? > yes >>> > This makes aborting cheaper and will simplify >>> > things in a later change. >>> >>> Given that we're stopping so the user can fix the problem and continue >>> the rebase I don't think optimizing for aborting is a convincing >>> reason for this change on its own. >>> >> this is all part of the "More or less as a side effect" paragraph, so >> this isn't a relevant objection. > >I'm simply saying that we should not be optimizing for "rebase --abort" >in this case. Do you think we should? > you're missing the point. the optimization isn't something anyone aimed for. regards
diff --git a/builtin/rebase.c b/builtin/rebase.c index e703b29835..61e5363ac7 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -333,7 +333,9 @@ static int run_sequencer_rebase(struct rebase_options *opts) case ACTION_CONTINUE: { struct replay_opts replay_opts = get_replay_opts(opts); - ret = sequencer_continue(the_repository, &replay_opts); + ret = sequencer_continue(the_repository, &replay_opts, flags, + opts->onto_name, &opts->onto->object.oid, + &opts->orig_head->object.oid); replay_opts_release(&replay_opts); break; } diff --git a/builtin/revert.c b/builtin/revert.c index 62986a7b1b..00d3e19c62 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -231,7 +231,8 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts) return ret; } if (cmd == 'c') - return sequencer_continue(the_repository, opts); + return sequencer_continue(the_repository, opts, + 0, NULL, NULL, NULL); if (cmd == 'a') return sequencer_rollback(the_repository, opts); if (cmd == 's') diff --git a/sequencer.c b/sequencer.c index aef42122f1..0b4d16b8e8 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3369,7 +3369,7 @@ int sequencer_skip(struct repository *r, struct replay_opts *opts) if (!is_directory(git_path_seq_dir())) return 0; - return sequencer_continue(r, opts); + return sequencer_continue(r, opts, 0, NULL, NULL, NULL); give_advice: error(_("there is nothing to skip")); @@ -5096,7 +5096,13 @@ static int commit_staged_changes(struct repository *r, return 0; } -int sequencer_continue(struct repository *r, struct replay_opts *opts) +static int start_rebase(struct repository *r, struct replay_opts *opts, unsigned flags, + const char *onto_name, const struct object_id *onto, + const struct object_id *orig_head, struct todo_list *todo_list); + +int sequencer_continue(struct repository *r, struct replay_opts *opts, unsigned flags, + const char *onto_name, const struct object_id *onto, + const struct object_id *orig_head) { struct todo_list todo_list = TODO_LIST_INIT; int res; @@ -5117,6 +5123,13 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts) unlink(rebase_path_dropped()); } + if (!todo_list.done_nr) { + res = start_rebase(r, opts, flags, + onto_name, onto, + orig_head, &todo_list); + goto release_todo_list; + } + opts->reflog_message = reflog_message(opts, "continue", NULL); if (commit_staged_changes(r, opts, &todo_list)) { res = -1; @@ -6096,9 +6109,8 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla enum rebase_action action) { char shortonto[GIT_MAX_HEXSZ + 1]; - const char *todo_file = rebase_path_todo(); struct todo_list new_todo = TODO_LIST_INIT; - struct strbuf *buf = &todo_list->buf, buf2 = STRBUF_INIT; + struct strbuf *buf = &todo_list->buf; int res; find_unique_abbrev_r(shortonto, onto, DEFAULT_ABBREV); @@ -6142,49 +6154,52 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla return error(_("nothing to do")); } else if (res == EDIT_TODO_INCORRECT) { - checkout_onto(r, opts, onto_name, onto, orig_head); todo_list_release(&new_todo); return -1; } - /* Expand the commit IDs */ - todo_list_to_strbuf(r, &new_todo, &buf2, -1, 0); - strbuf_swap(&new_todo.buf, &buf2); - strbuf_release(&buf2); - new_todo.total_nr -= new_todo.nr; - if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) < 0) - BUG("invalid todo list after expanding IDs:\n%s", - new_todo.buf.buf); - - if (opts->allow_ff && skip_unnecessary_picks(r, &new_todo, &onto)) { - todo_list_release(&new_todo); - return error(_("could not skip unnecessary pick commands")); - } - - if (todo_list_write_to_file(r, &new_todo, todo_file, NULL, NULL, -1, - flags & ~(TODO_LIST_SHORTEN_IDS), action)) { - todo_list_release(&new_todo); - return error_errno(_("could not write '%s'"), todo_file); - } - - res = -1; - - if (checkout_onto(r, opts, onto_name, onto, orig_head)) - goto cleanup; - - if (require_clean_work_tree(r, "rebase", NULL, 1, 1)) - goto cleanup; - - todo_list_write_total_nr(&new_todo); - res = pick_commits(r, &new_todo, opts); - -cleanup: + res = start_rebase(r, opts, flags, onto_name, onto, orig_head, &new_todo); todo_list_release(&new_todo); return res; } +static int start_rebase(struct repository *r, struct replay_opts *opts, unsigned flags, + const char *onto_name, const struct object_id *onto, + const struct object_id *orig_head, struct todo_list *todo_list) +{ + const char *todo_file = rebase_path_todo(); + struct strbuf buf2 = STRBUF_INIT; + + /* Expand the commit IDs */ + todo_list_to_strbuf(r, todo_list, &buf2, -1, 0); + strbuf_swap(&todo_list->buf, &buf2); + strbuf_release(&buf2); + todo_list->total_nr -= todo_list->nr; + if (todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list) < 0) + BUG("invalid todo list after expanding IDs:\n%s", + todo_list->buf.buf); + + if (opts->allow_ff && skip_unnecessary_picks(r, todo_list, &onto)) + return error(_("could not skip unnecessary pick commands")); + + if (todo_list_write_to_file(r, todo_list, todo_file, NULL, NULL, -1, + flags & ~(TODO_LIST_SHORTEN_IDS | + TODO_LIST_APPEND_TODO_HELP), + ACTION_CONTINUE)) + return error_errno(_("could not write '%s'"), todo_file); + + if (checkout_onto(r, opts, onto_name, onto, orig_head)) + return -1; + + if (require_clean_work_tree(r, "rebase", NULL, 1, 1)) + return -1; + + todo_list_write_total_nr(todo_list); + return pick_commits(r, todo_list, opts); +} + struct subject2item_entry { struct hashmap_entry entry; int i; diff --git a/sequencer.h b/sequencer.h index 24bf71d5db..33bcff89e0 100644 --- a/sequencer.h +++ b/sequencer.h @@ -158,7 +158,9 @@ void todo_list_filter_update_refs(struct repository *r, void sequencer_init_config(struct replay_opts *opts); int sequencer_pick_revisions(struct repository *repo, struct replay_opts *opts); -int sequencer_continue(struct repository *repo, struct replay_opts *opts); +int sequencer_continue(struct repository *repo, struct replay_opts *opts, unsigned flags, + const char *onto_name, const struct object_id *onto, + const struct object_id *orig_head); int sequencer_rollback(struct repository *repo, struct replay_opts *opts); int sequencer_skip(struct repository *repo, struct replay_opts *opts); void replay_opts_release(struct replay_opts *opts); diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index c625aad10a..dd47f0bbce 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1597,6 +1597,37 @@ test_expect_success 'static check of bad command' ' test C = $(git cat-file commit HEAD^ | sed -ne \$p) ' + +test_expect_success 'continue after bad first command' ' + test_when_finished "git rebase --abort ||:" && + git checkout primary^0 && + git reflog expire --expire=all HEAD && + ( + set_fake_editor && + test_must_fail env FAKE_LINES="bad 1 pick 1 pick 2 reword 3" \ + git rebase -i HEAD~3 && + test_cmp_rev HEAD primary && + FAKE_LINES="pick 2 pick 3 reword 4" git rebase --edit-todo && + FAKE_COMMIT_MESSAGE="E_reworded" git rebase --continue + ) && + git reflog > reflog && + test $(grep -c fast-forward reflog) = 1 && + test_cmp_rev HEAD~1 primary~1 && + test "$(git log -1 --format=%B)" = "E_reworded" +' + +test_expect_success 'abort after bad first command' ' + test_when_finished "git rebase --abort ||:" && + git checkout primary^0 && + ( + set_fake_editor && + test_must_fail env FAKE_LINES="bad 1 pick 1 pick 2 reword 3" \ + git rebase -i HEAD~3 + ) && + git rebase --abort && + test_cmp_rev HEAD primary +' + test_expect_success 'tabs and spaces are accepted in the todolist' ' rebase_setup_and_clean indented-comment && write_script add-indent.sh <<-\EOF &&
When the user butchers the todo file during rebase -i setup, the --continue which would follow --edit-todo would have skipped the last steps of the setup. Notably, this would bypass the fast-forward over untouched picks (though the actual picking loop would still fast-forward the commits, one by one). Fix this by splitting off the tail of complete_action() to a new start_rebase() function and call that from sequencer_continue() when no commands have been executed yet. More or less as a side effect, we no longer checkout `onto` before exiting when the todo file is bad. This makes aborting cheaper and will simplify things in a later change. Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> --- builtin/rebase.c | 4 +- builtin/revert.c | 3 +- sequencer.c | 89 ++++++++++++++++++++--------------- sequencer.h | 4 +- t/t3404-rebase-interactive.sh | 31 ++++++++++++ 5 files changed, 91 insertions(+), 40 deletions(-)