| Message ID | 20191202234759.26201-3-alban.gruin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase -i: extend rebase.missingCommitsCheck | expand |
Alban Gruin <alban.gruin@gmail.com> writes: > When set to "warn" or "error", `rebase.missingCommitsCheck' would make > `rebase -i' warn if the user removed commits from the todo list to > prevent mistakes. Unfortunately, `rebase --edit-todo' and `rebase > --continue' don't take it into account. > > This adds the ability for `rebase --edit-todo' and `rebase --continue' > to check if commits were dropped by the user. As both edit_todo_list() > and complete_action() parse the todo list and check for dropped commits, > the code doing so in the latter is removed to reduce duplication. > `edit_todo_list_advice' is removed from sequencer.c as it is no longer > used there. > > This changes when a backup of the todo list is made. Until now, it was > saved only before the initial edit. Now, it is always performed before > the todo list is edited. Without this, sequencer_continue() (`rebase > --continue') could only compare the current todo list against the > original, unedited list. Before this change, this file was only used by > edit_todo_list() and `rebase -p' to create the backup before the initial > edit, and check_todo_list_from_file(), only used by `rebase -p' to check > for dropped commits after its own initial edit. > > Three tests are added to t3404. The tests for > `rebase.missingCommitsCheck = warn' and `rebase.missingCommitsCheck = > error' have a similar structure. First, we start a rebase with an > incorrect command on the first line. Then, we edit the todo list, > removing the first and the last lines. This demonstrates that > `--edit-todo' notices dropped commits, but not when the command is > incorrect. Then, we restore the original todo list, and edit it to > remove the last line. This demonstrates that if we add a commit after > the initial edit, then remove it, `--edit-todo' will notice that it has > been dropped. Then, the actual rebase takes place. In the third test, > it is also checked that `--continue' will refuse to resume the rebase if > commits were dropped. > > Signed-off-by: Alban Gruin <alban.gruin@gmail.com> > --- > rebase-interactive.c | 22 ++++++---- > sequencer.c | 24 +++++----- > t/t3404-rebase-interactive.sh | 83 +++++++++++++++++++++++++++++++++++ > 3 files changed, 110 insertions(+), 19 deletions(-) Let me see if I understand the primary idea behind this change by trying to paraphrase the log (read: this is not to suggest a better phrasing of the log message proposed in your message). * rebase-interactive.c::edit_todo_list() does not perform "did the user delete a pick, instead of turning pick into drop?" but after the end-user edits the file is the most logical place to do so. Let's do that there. * The sequencer used to perform "did the user delete a pick, instead of turning pick into drop?" check in complete_action(). We drop that call but for this particular codepath it does not make any behaviour difference due to the next item. * New code does the check in sequencer_continue(), which is called at the end of complete_action(), as well as many other places, like builtin/rebase.c, builtin/revert.c, and sequencer_skip(). Because the check is only done when we are running "rebase-i", this is safe---it only affects complete_action(). I hope I got it more-or-less correctly ;-) > diff --git a/rebase-interactive.c b/rebase-interactive.c > index ad5dd49c31..80b6a2e7a6 100644 > --- a/rebase-interactive.c > +++ b/rebase-interactive.c > @@ -97,7 +97,8 @@ 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(); > + const char *todo_file = rebase_path_todo(), > + *todo_backup = rebase_path_todo_backup(); > /* If the user is editing the todo list, we first try to parse > @@ -110,9 +111,9 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list, > -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP)) > return error_errno(_("could not write '%s'"), todo_file); > > - if (initial && copy_file(rebase_path_todo_backup(), todo_file, 0666)) > - return error(_("could not copy '%s' to '%s'."), todo_file, > - rebase_path_todo_backup()); > + unlink(todo_backup); > + if (copy_file(todo_backup, todo_file, 0666)) > + return error(_("could not copy '%s' to '%s'."), todo_file, todo_backup); We used to copy ONLY when initial is set and we left old todo_backup intact when !initial. That is no longer true after this change, but it is intended---we create an exact copy of what we would hand out to the end-user, so that we can compare it with the edited result to figure out what got changed. We unlink(2) unconditionally because the only effect we want to see here is that todo_backup does not exist before we call copy_file() that wants to do O_CREAT|O_EXCL. I wonder if we want to avoid unlink() when initial, and also if we want to do unlink_or_warn() when !initial (read: this is just "wondering" without thinking long enough to suggest that doing so would be better) > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 29a35840ed..9051c1e11d 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -1343,6 +1343,89 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' ' > test B = $(git cat-file commit HEAD^ | sed -ne \$p) > ' > > +test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = ignore' ' > + test_config rebase.missingCommitsCheck ignore && > + rebase_setup_and_clean missing-commit && > + set_fake_editor && > + FAKE_LINES="break 1 2 3 4 5" git rebase -i --root && > + FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual && OK, so we lost "pick 5" but with missing-check disabled, that should not trigger any annoying warning or error. > + git rebase --continue 2>actual && > + test D = $(git cat-file commit HEAD | sed -ne \$p) && > + test_i18ngrep \ > + "Successfully rebased and updated refs/heads/missing-commit" \ > + actual > +' > + > +test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = warn' ' > + cat >expect <<-EOF && > + error: invalid line 1: badcmd $(git rev-list --pretty=oneline --abbrev-commit -1 master~4) > + Warning: some commits may have been dropped accidentally. > + Dropped commits (newer to older): > + - $(git rev-list --pretty=oneline --abbrev-commit -1 master) > + To avoid this message, use "drop" to explicitly remove a commit. > + EOF > + tail -n4 expect >expect.2 && > + test_config rebase.missingCommitsCheck warn && > + rebase_setup_and_clean missing-commit && > + set_fake_editor && > + test_must_fail env FAKE_LINES="bad 1 2 3 4 5" \ > + git rebase -i --root && > + cp .git/rebase-merge/git-rebase-todo.backup orig && > + FAKE_LINES="2 3 4" git rebase --edit-todo 2>actual.2 && > + head -n5 actual.2 >actual && > + test_i18ncmp expect actual && OK, so we lost "pick 1" while discarding "bad", and we should notice the lossage? I see "head -n5" there, which means we are still getting "invalid line 1: badcmd", even though FAKE_LINES now got rid of "bad"? Puzzled...
Hi Alban and Junio On 04/12/2019 19:19, Junio C Hamano wrote: > Alban Gruin <alban.gruin@gmail.com> writes: > >> When set to "warn" or "error", `rebase.missingCommitsCheck' would make >> `rebase -i' warn if the user removed commits from the todo list to >> prevent mistakes. Unfortunately, `rebase --edit-todo' and `rebase >> --continue' don't take it into account. >> >> This adds the ability for `rebase --edit-todo' and `rebase --continue' >> to check if commits were dropped by the user. As both edit_todo_list() >> and complete_action() parse the todo list and check for dropped commits, >> the code doing so in the latter is removed to reduce duplication. >> `edit_todo_list_advice' is removed from sequencer.c as it is no longer >> used there. >> >> This changes when a backup of the todo list is made. Until now, it was >> saved only before the initial edit. Now, it is always performed before >> the todo list is edited. Without this, sequencer_continue() (`rebase >> --continue') could only compare the current todo list against the >> original, unedited list. Before this change, this file was only used by >> edit_todo_list() and `rebase -p' to create the backup before the initial >> edit, and check_todo_list_from_file(), only used by `rebase -p' to check >> for dropped commits after its own initial edit. >> >> Three tests are added to t3404. The tests for >> `rebase.missingCommitsCheck = warn' and `rebase.missingCommitsCheck = >> error' have a similar structure. First, we start a rebase with an >> incorrect command on the first line. Then, we edit the todo list, >> removing the first and the last lines. This demonstrates that >> `--edit-todo' notices dropped commits, but not when the command is >> incorrect. Then, we restore the original todo list, and edit it to >> remove the last line. This demonstrates that if we add a commit after >> the initial edit, then remove it, `--edit-todo' will notice that it has >> been dropped. Then, the actual rebase takes place. In the third test, >> it is also checked that `--continue' will refuse to resume the rebase if >> commits were dropped. >> >> Signed-off-by: Alban Gruin <alban.gruin@gmail.com> >> --- >> rebase-interactive.c | 22 ++++++---- >> sequencer.c | 24 +++++----- >> t/t3404-rebase-interactive.sh | 83 +++++++++++++++++++++++++++++++++++ >> 3 files changed, 110 insertions(+), 19 deletions(-) > > Let me see if I understand the primary idea behind this change by > trying to paraphrase the log (read: this is not to suggest a better > phrasing of the log message proposed in your message). > > * rebase-interactive.c::edit_todo_list() does not perform "did the > user delete a pick, instead of turning pick into drop?" but after > the end-user edits the file is the most logical place to do so. > Let's do that there. > > * The sequencer used to perform "did the user delete a pick, > instead of turning pick into drop?" check in complete_action(). > We drop that call but for this particular codepath it does not > make any behaviour difference due to the next item. > > * New code does the check in sequencer_continue(), which is called > at the end of complete_action(), as well as many other places, > like builtin/rebase.c, builtin/revert.c, and sequencer_skip(). > Because the check is only done when we are running "rebase-i", > this is safe---it only affects complete_action(). > > I hope I got it more-or-less correctly ;-) > >> diff --git a/rebase-interactive.c b/rebase-interactive.c >> index ad5dd49c31..80b6a2e7a6 100644 >> --- a/rebase-interactive.c >> +++ b/rebase-interactive.c >> @@ -97,7 +97,8 @@ 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(); >> + const char *todo_file = rebase_path_todo(), >> + *todo_backup = rebase_path_todo_backup(); >> /* If the user is editing the todo list, we first try to parse >> @@ -110,9 +111,9 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list, >> -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP)) >> return error_errno(_("could not write '%s'"), todo_file); >> >> - if (initial && copy_file(rebase_path_todo_backup(), todo_file, 0666)) >> - return error(_("could not copy '%s' to '%s'."), todo_file, >> - rebase_path_todo_backup()); >> + unlink(todo_backup); >> + if (copy_file(todo_backup, todo_file, 0666)) >> + return error(_("could not copy '%s' to '%s'."), todo_file, todo_backup); > > We used to copy ONLY when initial is set and we left old todo_backup > intact when !initial. That is no longer true after this change, but > it is intended---we create an exact copy of what we would hand out > to the end-user, so that we can compare it with the edited result > to figure out what got changed. I think it would be better to only create a new copy if the last edit was successful. As it stands if I edit the todo list and accidentally delete some lines and then edit the todo list again to try and fix it the second edit will succeed whether or not I reinserted the deleted lines. We could add this to the tests to check that a subsequent edit that does not fix the problem fails diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 969e12d281..8544d8ab2c 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1416,6 +1416,7 @@ test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = er test_i18ncmp expect actual && test_must_fail git rebase --continue 2>actual && test_i18ncmp expect actual && + test_must_fail git rebase --edit-todo && cp orig .git/rebase-merge/git-rebase-todo && test_must_fail env FAKE_LINES="1 2 3 4" \ git rebase --edit-todo 2>actual && > > We unlink(2) unconditionally because the only effect we want to see > here is that todo_backup does not exist before we call copy_file() > that wants to do O_CREAT|O_EXCL. I wonder if we want to avoid > unlink() when initial, and also if we want to do unlink_or_warn() > when !initial (read: this is just "wondering" without thinking long > enough to suggest that doing so would be better) > >> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh >> index 29a35840ed..9051c1e11d 100755 >> --- a/t/t3404-rebase-interactive.sh >> +++ b/t/t3404-rebase-interactive.sh >> @@ -1343,6 +1343,89 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' ' >> test B = $(git cat-file commit HEAD^ | sed -ne \$p) >> ' >> >> +test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = ignore' ' >> + test_config rebase.missingCommitsCheck ignore && >> + rebase_setup_and_clean missing-commit && >> + set_fake_editor && >> + FAKE_LINES="break 1 2 3 4 5" git rebase -i --root && >> + FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual && > > OK, so we lost "pick 5" but with missing-check disabled, that should > not trigger any annoying warning or error. > >> + git rebase --continue 2>actual && This clobbers actual which hasn't been used yet >> + test D = $(git cat-file commit HEAD | sed -ne \$p) && > >> + test_i18ngrep \ >> + "Successfully rebased and updated refs/heads/missing-commit" \ >> + actual >> +' >> + >> +test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = warn' ' >> + cat >expect <<-EOF && >> + error: invalid line 1: badcmd $(git rev-list --pretty=oneline --abbrev-commit -1 master~4) >> + Warning: some commits may have been dropped accidentally. >> + Dropped commits (newer to older): >> + - $(git rev-list --pretty=oneline --abbrev-commit -1 master) >> + To avoid this message, use "drop" to explicitly remove a commit. >> + EOF >> + tail -n4 expect >expect.2 && >> + test_config rebase.missingCommitsCheck warn && >> + rebase_setup_and_clean missing-commit && >> + set_fake_editor && >> + test_must_fail env FAKE_LINES="bad 1 2 3 4 5" \ >> + git rebase -i --root && >> + cp .git/rebase-merge/git-rebase-todo.backup orig && >> + FAKE_LINES="2 3 4" git rebase --edit-todo 2>actual.2 && >> + head -n5 actual.2 >actual && >> + test_i18ncmp expect actual && > > OK, so we lost "pick 1" while discarding "bad", and we should notice > the lossage? I see "head -n5" there, which means we are still > getting "invalid line 1: badcmd", even though FAKE_LINES now got rid > of "bad"? Puzzled... Is the bad there to stop the rebase so we can edit the todo list? If so it would be better to use 'break' instead. Best Wishes Phillip
Hi Alban On 02/12/2019 23:47, Alban Gruin wrote: > [...] > diff --git a/sequencer.c b/sequencer.c > index 181bb35f5f..75d5ad0496 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -4271,8 +4271,20 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts) > if (read_populate_opts(opts)) > return -1; > if (is_rebase_i(opts)) { > + struct todo_list backup = TODO_LIST_INIT; > + > if ((res = read_populate_todo(r, &todo_list, opts))) > goto release_todo_list; > + > + if (strbuf_read_file(&backup.buf, rebase_path_todo_backup(), 0) > 0) { > + todo_list_parse_insn_buffer(r, backup.buf.buf, &backup); > + res = todo_list_check(&backup, &todo_list); > + todo_list_release(&backup); This causes problems if the user edits the todo list and then later runs 'git rebase --continue' after resolving conflicts as the backup todo list has a bunch of commands that have already been processed but they are seen as missing This test fails test_expect_success 'rebase.missingCommitsCheck = error after resolving conflicts' ' test_config rebase.missingCommitsCheck error && ( set_fake_editor && FAKE_LINES="break 2 3 4" git rebase -i A E ) && git rebase --edit-todo && test_must_fail git rebase --continue && echo x >file1 && git add file1 && git rebase --continue ' I think it would be better to write a file if the check fails when editing the todo list and check for the presence of that file when continuing and error out if it exists. This would also allow --edit-todo to only remove the backup file if there are no outstanding errors from the last edit and so check that those errors are fixed by the second edit. I think we'd only want to do this if rebase.missingCommitsCheck is set to error. Best Wishes Phillip > + > + if (res) > + goto release_todo_list; > + } > + > if (commit_staged_changes(r, opts, &todo_list)) > return -1; > } else if (!file_exists(get_todo_path(opts))) > @@ -4986,12 +4998,6 @@ int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list, > return res; > } > > -static const char edit_todo_list_advice[] = > -N_("You can fix this with 'git rebase --edit-todo' " > -"and then run 'git rebase --continue'.\n" > -"Or you can abort the rebase with 'git rebase" > -" --abort'.\n"); > - > /* skip picking commits whose parents are unchanged */ > static int skip_unnecessary_picks(struct repository *r, > struct todo_list *todo_list, > @@ -5089,11 +5095,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla > todo_list_release(&new_todo); > > return error(_("nothing to do")); > - } > - > - if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) || > - todo_list_check(todo_list, &new_todo)) { > - fprintf(stderr, _(edit_todo_list_advice)); > + } else if (res == -4) { > checkout_onto(r, opts, onto_name, &onto->object.oid, orig_head); > todo_list_release(&new_todo); > > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 29a35840ed..9051c1e11d 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > @@ -1343,6 +1343,89 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' ' > test B = $(git cat-file commit HEAD^ | sed -ne \$p) > ' > > +test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = ignore' ' > + test_config rebase.missingCommitsCheck ignore && > + rebase_setup_and_clean missing-commit && > + set_fake_editor && > + FAKE_LINES="break 1 2 3 4 5" git rebase -i --root && > + FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual && > + git rebase --continue 2>actual && > + test D = $(git cat-file commit HEAD | sed -ne \$p) && > + test_i18ngrep \ > + "Successfully rebased and updated refs/heads/missing-commit" \ > + actual > +' > + > +test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = warn' ' > + cat >expect <<-EOF && > + error: invalid line 1: badcmd $(git rev-list --pretty=oneline --abbrev-commit -1 master~4) > + Warning: some commits may have been dropped accidentally. > + Dropped commits (newer to older): > + - $(git rev-list --pretty=oneline --abbrev-commit -1 master) > + To avoid this message, use "drop" to explicitly remove a commit. > + EOF > + tail -n4 expect >expect.2 && > + test_config rebase.missingCommitsCheck warn && > + rebase_setup_and_clean missing-commit && > + set_fake_editor && > + test_must_fail env FAKE_LINES="bad 1 2 3 4 5" \ > + git rebase -i --root && > + cp .git/rebase-merge/git-rebase-todo.backup orig && > + FAKE_LINES="2 3 4" git rebase --edit-todo 2>actual.2 && > + head -n5 actual.2 >actual && > + test_i18ncmp expect actual && > + cp orig .git/rebase-merge/git-rebase-todo && > + FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual.2 && > + head -n4 actual.2 >actual && > + test_i18ncmp expect.2 actual && > + git rebase --continue 2>actual && > + test D = $(git cat-file commit HEAD | sed -ne \$p) && > + test_i18ngrep \ > + "Successfully rebased and updated refs/heads/missing-commit" \ > + actual > +' > + > +test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = error' ' > + cat >expect <<-EOF && > + error: invalid line 1: badcmd $(git rev-list --pretty=oneline --abbrev-commit -1 master~4) > + Warning: some commits may have been dropped accidentally. > + Dropped commits (newer to older): > + - $(git rev-list --pretty=oneline --abbrev-commit -1 master) > + To avoid this message, use "drop" to explicitly remove a commit. > + > + Use '\''git config rebase.missingCommitsCheck'\'' to change the level of warnings. > + The possible behaviours are: ignore, warn, error. > + > + You can fix this with '\''git rebase --edit-todo'\'' and then run '\''git rebase --continue'\''. > + Or you can abort the rebase with '\''git rebase --abort'\''. > + EOF > + tail -n10 expect >expect.2 && > + test_config rebase.missingCommitsCheck error && > + rebase_setup_and_clean missing-commit && > + set_fake_editor && > + test_must_fail env FAKE_LINES="bad 1 2 3 4 5" \ > + git rebase -i --root && > + cp .git/rebase-merge/git-rebase-todo.backup orig && > + test_must_fail env FAKE_LINES="2 3 4" \ > + git rebase --edit-todo 2>actual && > + test_i18ncmp expect actual && > + test_must_fail git rebase --continue 2>actual && > + test_i18ncmp expect actual && > + cp orig .git/rebase-merge/git-rebase-todo && > + test_must_fail env FAKE_LINES="1 2 3 4" \ > + git rebase --edit-todo 2>actual && > + test_i18ncmp expect.2 actual && > + test_must_fail git rebase --continue 2>actual && > + test_i18ncmp expect.2 actual && > + cp orig .git/rebase-merge/git-rebase-todo && > + FAKE_LINES="1 2 3 4 drop 5" git rebase --edit-todo && > + git rebase --continue 2>actual && > + test D = $(git cat-file commit HEAD | sed -ne \$p) && > + test_i18ngrep \ > + "Successfully rebased and updated refs/heads/missing-commit" \ > + actual > +' > + > test_expect_success 'respects rebase.abbreviateCommands with fixup, squash and exec' ' > rebase_setup_and_clean abbrevcmd && > test_commit "first" file1.txt "first line" first && >
Hi Phillip, Le 09/12/2019 à 17:00, Phillip Wood a écrit : >>> diff --git a/rebase-interactive.c b/rebase-interactive.c >>> index ad5dd49c31..80b6a2e7a6 100644 >>> --- a/rebase-interactive.c >>> +++ b/rebase-interactive.c >>> @@ -97,7 +97,8 @@ 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(); >>> + const char *todo_file = rebase_path_todo(), >>> + *todo_backup = rebase_path_todo_backup(); >>> /* If the user is editing the todo list, we first try to parse >>> @@ -110,9 +111,9 @@ int edit_todo_list(struct repository *r, struct >>> todo_list *todo_list, >>> -1, flags | TODO_LIST_SHORTEN_IDS | >>> TODO_LIST_APPEND_TODO_HELP)) >>> return error_errno(_("could not write '%s'"), todo_file); >>> - if (initial && copy_file(rebase_path_todo_backup(), todo_file, >>> 0666)) >>> - return error(_("could not copy '%s' to '%s'."), todo_file, >>> - rebase_path_todo_backup()); >>> + unlink(todo_backup); >>> + if (copy_file(todo_backup, todo_file, 0666)) >>> + return error(_("could not copy '%s' to '%s'."), todo_file, >>> todo_backup); >> >> We used to copy ONLY when initial is set and we left old todo_backup >> intact when !initial. That is no longer true after this change, but >> it is intended---we create an exact copy of what we would hand out >> to the end-user, so that we can compare it with the edited result >> to figure out what got changed. > > I think it would be better to only create a new copy if the last edit > was successful. As it stands if I edit the todo list and accidentally > delete some lines and then edit the todo list again to try and fix it > the second edit will succeed whether or not I reinserted the deleted lines. > > We could add this to the tests to check that a subsequent edit that does > not fix the problem fails > > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > index 969e12d281..8544d8ab2c 100755 > --- a/t/t3404-rebase-interactive.sh > +++ b/t/t3404-rebase-interactive.sh > > @@ -1416,6 +1416,7 @@ test_expect_success 'rebase --edit-todo respects > rebase.missingCommitsCheck = er > test_i18ncmp expect actual && > test_must_fail git rebase --continue 2>actual && > test_i18ncmp expect actual && > + test_must_fail git rebase --edit-todo && > cp orig .git/rebase-merge/git-rebase-todo && > test_must_fail env FAKE_LINES="1 2 3 4" \ > git rebase --edit-todo 2>actual && > > In which case, if the check did not pass at the previous edit, the new todo list should be compared to the backup. As sequencer_continue() already does this, extract this to its own function in rebase-interactive.c. To keep track of this, a file is created on the disk (as you suggested in your other email.) At the next edit, if this file exists and no errors were found, it is deleted. The backup is only created if there is no errors in `todo_list' and in `new_todo'. This would guarantee that there is no errors in the backup, and that the edited list is always compared to a list exempt of errors. This approach also has the benefit to detect if a commit part of a badcmd was dropped. After some tweaks (ie. `expect' now lists 2 commits instead of one), this passes the test with the change you suggested, and the one you sent in your other email. >> >> We unlink(2) unconditionally because the only effect we want to see >> here is that todo_backup does not exist before we call copy_file() >> that wants to do O_CREAT|O_EXCL. I wonder if we want to avoid >> unlink() when initial, and also if we want to do unlink_or_warn() >> when !initial (read: this is just "wondering" without thinking long >> enough to suggest that doing so would be better) >> >>> diff --git a/t/t3404-rebase-interactive.sh >>> b/t/t3404-rebase-interactive.sh >>> index 29a35840ed..9051c1e11d 100755 >>> --- a/t/t3404-rebase-interactive.sh >>> +++ b/t/t3404-rebase-interactive.sh >>> @@ -1343,6 +1343,89 @@ test_expect_success 'rebase -i respects >>> rebase.missingCommitsCheck = error' ' >>> test B = $(git cat-file commit HEAD^ | sed -ne \$p) >>> ' >>> +test_expect_success 'rebase --edit-todo respects >>> rebase.missingCommitsCheck = ignore' ' >>> + test_config rebase.missingCommitsCheck ignore && >>> + rebase_setup_and_clean missing-commit && >>> + set_fake_editor && >>> + FAKE_LINES="break 1 2 3 4 5" git rebase -i --root && >>> + FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual && >> >> OK, so we lost "pick 5" but with missing-check disabled, that should >> not trigger any annoying warning or error. >> >>> + git rebase --continue 2>actual && > > This clobbers actual which hasn't been used yet > Good catch. >>> + test D = $(git cat-file commit HEAD | sed -ne \$p) && >> >>> + test_i18ngrep \ >>> + "Successfully rebased and updated refs/heads/missing-commit" \ >>> + actual >>> +' >>> + >>> +test_expect_success 'rebase --edit-todo respects >>> rebase.missingCommitsCheck = warn' ' >>> + cat >expect <<-EOF && >>> + error: invalid line 1: badcmd $(git rev-list --pretty=oneline >>> --abbrev-commit -1 master~4) >>> + Warning: some commits may have been dropped accidentally. >>> + Dropped commits (newer to older): >>> + - $(git rev-list --pretty=oneline --abbrev-commit -1 master) >>> + To avoid this message, use "drop" to explicitly remove a commit. >>> + EOF >>> + tail -n4 expect >expect.2 && >>> + test_config rebase.missingCommitsCheck warn && >>> + rebase_setup_and_clean missing-commit && >>> + set_fake_editor && >>> + test_must_fail env FAKE_LINES="bad 1 2 3 4 5" \ >>> + git rebase -i --root && >>> + cp .git/rebase-merge/git-rebase-todo.backup orig && >>> + FAKE_LINES="2 3 4" git rebase --edit-todo 2>actual.2 && >>> + head -n5 actual.2 >actual && >>> + test_i18ncmp expect actual && >> >> OK, so we lost "pick 1" while discarding "bad", and we should notice >> the lossage? I see "head -n5" there, which means we are still >> getting "invalid line 1: badcmd", even though FAKE_LINES now got rid >> of "bad"? Puzzled... > > Is the bad there to stop the rebase so we can edit the todo list? If so > it would be better to use 'break' instead. > No, it was here to show that we can detect dropped commits, even if the todo list has an error. > Best Wishes > > Phillip Cheers, Alban
Hi Alban On 09/01/2020 21:13, Alban Gruin wrote: > Hi Phillip, > > Le 09/12/2019 à 17:00, Phillip Wood a écrit : >>>> diff --git a/rebase-interactive.c b/rebase-interactive.c >>>> index ad5dd49c31..80b6a2e7a6 100644 >>>> --- a/rebase-interactive.c >>>> +++ b/rebase-interactive.c >>>> @@ -97,7 +97,8 @@ 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(); >>>> + const char *todo_file = rebase_path_todo(), >>>> + *todo_backup = rebase_path_todo_backup(); >>>> /* If the user is editing the todo list, we first try to parse >>>> @@ -110,9 +111,9 @@ int edit_todo_list(struct repository *r, struct >>>> todo_list *todo_list, >>>> -1, flags | TODO_LIST_SHORTEN_IDS | >>>> TODO_LIST_APPEND_TODO_HELP)) >>>> return error_errno(_("could not write '%s'"), todo_file); >>>> - if (initial && copy_file(rebase_path_todo_backup(), todo_file, >>>> 0666)) >>>> - return error(_("could not copy '%s' to '%s'."), todo_file, >>>> - rebase_path_todo_backup()); >>>> + unlink(todo_backup); >>>> + if (copy_file(todo_backup, todo_file, 0666)) >>>> + return error(_("could not copy '%s' to '%s'."), todo_file, >>>> todo_backup); >>> >>> We used to copy ONLY when initial is set and we left old todo_backup >>> intact when !initial. That is no longer true after this change, but >>> it is intended---we create an exact copy of what we would hand out >>> to the end-user, so that we can compare it with the edited result >>> to figure out what got changed. >> >> I think it would be better to only create a new copy if the last edit >> was successful. As it stands if I edit the todo list and accidentally >> delete some lines and then edit the todo list again to try and fix it >> the second edit will succeed whether or not I reinserted the deleted lines. >> >> We could add this to the tests to check that a subsequent edit that does >> not fix the problem fails >> >> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh >> index 969e12d281..8544d8ab2c 100755 >> --- a/t/t3404-rebase-interactive.sh >> +++ b/t/t3404-rebase-interactive.sh >> >> @@ -1416,6 +1416,7 @@ test_expect_success 'rebase --edit-todo respects >> rebase.missingCommitsCheck = er >> test_i18ncmp expect actual && >> test_must_fail git rebase --continue 2>actual && >> test_i18ncmp expect actual && >> + test_must_fail git rebase --edit-todo && >> cp orig .git/rebase-merge/git-rebase-todo && >> test_must_fail env FAKE_LINES="1 2 3 4" \ >> git rebase --edit-todo 2>actual && >> >> > > In which case, if the check did not pass at the previous edit, the new > todo list should be compared to the backup. As sequencer_continue() > already does this, extract this to its own function in > rebase-interactive.c. To keep track of this, a file is created on the > disk (as you suggested in your other email.) At the next edit, if this > file exists and no errors were found, it is deleted. The backup is only > created if there is no errors in `todo_list' and in `new_todo'. > > This would guarantee that there is no errors in the backup, and that the > edited list is always compared to a list exempt of errors. > > This approach also has the benefit to detect if a commit part of a > badcmd was dropped. > > After some tweaks (ie. `expect' now lists 2 commits instead of one), > this passes the test with the change you suggested, and the one you sent > in your other email. That sounds good. I'm not sure how it passes the test in my other email though, if sequencer_continue() compares the todo list to the backup wont it still fail when continuing after conflicts as the backup is out of date? Best Wishes Phillip >>> >>> We unlink(2) unconditionally because the only effect we want to see >>> here is that todo_backup does not exist before we call copy_file() >>> that wants to do O_CREAT|O_EXCL. I wonder if we want to avoid >>> unlink() when initial, and also if we want to do unlink_or_warn() >>> when !initial (read: this is just "wondering" without thinking long >>> enough to suggest that doing so would be better) >>> >>>> diff --git a/t/t3404-rebase-interactive.sh >>>> b/t/t3404-rebase-interactive.sh >>>> index 29a35840ed..9051c1e11d 100755 >>>> --- a/t/t3404-rebase-interactive.sh >>>> +++ b/t/t3404-rebase-interactive.sh >>>> @@ -1343,6 +1343,89 @@ test_expect_success 'rebase -i respects >>>> rebase.missingCommitsCheck = error' ' >>>> test B = $(git cat-file commit HEAD^ | sed -ne \$p) >>>> ' >>>> +test_expect_success 'rebase --edit-todo respects >>>> rebase.missingCommitsCheck = ignore' ' >>>> + test_config rebase.missingCommitsCheck ignore && >>>> + rebase_setup_and_clean missing-commit && >>>> + set_fake_editor && >>>> + FAKE_LINES="break 1 2 3 4 5" git rebase -i --root && >>>> + FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual && >>> >>> OK, so we lost "pick 5" but with missing-check disabled, that should >>> not trigger any annoying warning or error. >>> >>>> + git rebase --continue 2>actual && >> >> This clobbers actual which hasn't been used yet >> > > Good catch. > >>>> + test D = $(git cat-file commit HEAD | sed -ne \$p) && >>> >>>> + test_i18ngrep \ >>>> + "Successfully rebased and updated refs/heads/missing-commit" \ >>>> + actual >>>> +' >>>> + >>>> +test_expect_success 'rebase --edit-todo respects >>>> rebase.missingCommitsCheck = warn' ' >>>> + cat >expect <<-EOF && >>>> + error: invalid line 1: badcmd $(git rev-list --pretty=oneline >>>> --abbrev-commit -1 master~4) >>>> + Warning: some commits may have been dropped accidentally. >>>> + Dropped commits (newer to older): >>>> + - $(git rev-list --pretty=oneline --abbrev-commit -1 master) >>>> + To avoid this message, use "drop" to explicitly remove a commit. >>>> + EOF >>>> + tail -n4 expect >expect.2 && >>>> + test_config rebase.missingCommitsCheck warn && >>>> + rebase_setup_and_clean missing-commit && >>>> + set_fake_editor && >>>> + test_must_fail env FAKE_LINES="bad 1 2 3 4 5" \ >>>> + git rebase -i --root && >>>> + cp .git/rebase-merge/git-rebase-todo.backup orig && >>>> + FAKE_LINES="2 3 4" git rebase --edit-todo 2>actual.2 && >>>> + head -n5 actual.2 >actual && >>>> + test_i18ncmp expect actual && >>> >>> OK, so we lost "pick 1" while discarding "bad", and we should notice >>> the lossage? I see "head -n5" there, which means we are still >>> getting "invalid line 1: badcmd", even though FAKE_LINES now got rid >>> of "bad"? Puzzled... >> >> Is the bad there to stop the rebase so we can edit the todo list? If so >> it would be better to use 'break' instead. >> > > No, it was here to show that we can detect dropped commits, even if the > todo list has an error. > >> Best Wishes >> >> Phillip > > Cheers, > Alban >
Hi Phillip, Le 10/01/2020 à 18:13, Phillip Wood a écrit : > Hi Alban > > On 09/01/2020 21:13, Alban Gruin wrote: >> Hi Phillip, >> >> Le 09/12/2019 à 17:00, Phillip Wood a écrit : >>>>> diff --git a/rebase-interactive.c b/rebase-interactive.c >>>>> index ad5dd49c31..80b6a2e7a6 100644 >>>>> --- a/rebase-interactive.c >>>>> +++ b/rebase-interactive.c >>>>> @@ -97,7 +97,8 @@ 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(); >>>>> + const char *todo_file = rebase_path_todo(), >>>>> + *todo_backup = rebase_path_todo_backup(); >>>>> /* If the user is editing the todo list, we first try to parse >>>>> @@ -110,9 +111,9 @@ int edit_todo_list(struct repository *r, struct >>>>> todo_list *todo_list, >>>>> -1, flags | TODO_LIST_SHORTEN_IDS | >>>>> TODO_LIST_APPEND_TODO_HELP)) >>>>> return error_errno(_("could not write '%s'"), todo_file); >>>>> - if (initial && copy_file(rebase_path_todo_backup(), todo_file, >>>>> 0666)) >>>>> - return error(_("could not copy '%s' to '%s'."), todo_file, >>>>> - rebase_path_todo_backup()); >>>>> + unlink(todo_backup); >>>>> + if (copy_file(todo_backup, todo_file, 0666)) >>>>> + return error(_("could not copy '%s' to '%s'."), todo_file, >>>>> todo_backup); >>>> >>>> We used to copy ONLY when initial is set and we left old todo_backup >>>> intact when !initial. That is no longer true after this change, but >>>> it is intended---we create an exact copy of what we would hand out >>>> to the end-user, so that we can compare it with the edited result >>>> to figure out what got changed. >>> >>> I think it would be better to only create a new copy if the last edit >>> was successful. As it stands if I edit the todo list and accidentally >>> delete some lines and then edit the todo list again to try and fix it >>> the second edit will succeed whether or not I reinserted the deleted >>> lines. >>> >>> We could add this to the tests to check that a subsequent edit that does >>> not fix the problem fails >>> >>> diff --git a/t/t3404-rebase-interactive.sh >>> b/t/t3404-rebase-interactive.sh >>> index 969e12d281..8544d8ab2c 100755 >>> --- a/t/t3404-rebase-interactive.sh >>> +++ b/t/t3404-rebase-interactive.sh >>> >>> @@ -1416,6 +1416,7 @@ test_expect_success 'rebase --edit-todo respects >>> rebase.missingCommitsCheck = er >>> test_i18ncmp expect actual && >>> test_must_fail git rebase --continue 2>actual && >>> test_i18ncmp expect actual && >>> + test_must_fail git rebase --edit-todo && >>> cp orig .git/rebase-merge/git-rebase-todo && >>> test_must_fail env FAKE_LINES="1 2 3 4" \ >>> git rebase --edit-todo 2>actual && >>> >>> >> >> In which case, if the check did not pass at the previous edit, the new >> todo list should be compared to the backup. As sequencer_continue() >> already does this, extract this to its own function in >> rebase-interactive.c. To keep track of this, a file is created on the >> disk (as you suggested in your other email.) At the next edit, if this >> file exists and no errors were found, it is deleted. The backup is only >> created if there is no errors in `todo_list' and in `new_todo'. >> >> This would guarantee that there is no errors in the backup, and that the >> edited list is always compared to a list exempt of errors. >> >> This approach also has the benefit to detect if a commit part of a >> badcmd was dropped. >> >> After some tweaks (ie. `expect' now lists 2 commits instead of one), >> this passes the test with the change you suggested, and the one you sent >> in your other email. > > That sounds good. I'm not sure how it passes the test in my other email > though, if sequencer_continue() compares the todo list to the backup > wont it still fail when continuing after conflicts as the backup is out > of date? > I changed sequencer_continue() to check the todo list only if the file indicating an error exists. I still have to rewrite the commit message, then I’ll re-send this series. Cheers, Alban > Best Wishes > > Phillip >
Hi Alban On 10/01/2020 21:31, Alban Gruin wrote: > Hi Phillip, > > Le 10/01/2020 à 18:13, Phillip Wood a écrit : >> Hi Alban >> >> On 09/01/2020 21:13, Alban Gruin wrote: >>> Hi Phillip, >>> [...] >>> >>> In which case, if the check did not pass at the previous edit, the new >>> todo list should be compared to the backup. As sequencer_continue() >>> already does this, extract this to its own function in >>> rebase-interactive.c. To keep track of this, a file is created on the >>> disk (as you suggested in your other email.) At the next edit, if this >>> file exists and no errors were found, it is deleted. The backup is only >>> created if there is no errors in `todo_list' and in `new_todo'. >>> >>> This would guarantee that there is no errors in the backup, and that the >>> edited list is always compared to a list exempt of errors. >>> >>> This approach also has the benefit to detect if a commit part of a >>> badcmd was dropped. >>> >>> After some tweaks (ie. `expect' now lists 2 commits instead of one), >>> this passes the test with the change you suggested, and the one you sent >>> in your other email. >> >> That sounds good. I'm not sure how it passes the test in my other email >> though, if sequencer_continue() compares the todo list to the backup >> wont it still fail when continuing after conflicts as the backup is out >> of date? >> > > I changed sequencer_continue() to check the todo list only if the file > indicating an error exists. That makes sense > I still have to rewrite the commit message, then I’ll re-send this series. Excellent, I look forward to reading them Best Wishes Phillip > Cheers, > Alban > >> Best Wishes >> >> Phillip >>
diff --git a/rebase-interactive.c b/rebase-interactive.c index ad5dd49c31..80b6a2e7a6 100644 --- a/rebase-interactive.c +++ b/rebase-interactive.c @@ -97,7 +97,8 @@ 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(); + const char *todo_file = rebase_path_todo(), + *todo_backup = rebase_path_todo_backup(); unsigned initial = shortrevisions && shortonto; /* If the user is editing the todo list, we first try to parse @@ -110,9 +111,9 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list, -1, flags | TODO_LIST_SHORTEN_IDS | TODO_LIST_APPEND_TODO_HELP)) return error_errno(_("could not write '%s'"), todo_file); - if (initial && copy_file(rebase_path_todo_backup(), todo_file, 0666)) - return error(_("could not copy '%s' to '%s'."), todo_file, - rebase_path_todo_backup()); + unlink(todo_backup); + if (copy_file(todo_backup, todo_file, 0666)) + return error(_("could not copy '%s' to '%s'."), todo_file, todo_backup); if (launch_sequence_editor(todo_file, &new_todo->buf, NULL)) return -2; @@ -121,10 +122,13 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list, if (initial && new_todo->buf.len == 0) return -3; - /* For the initial edit, the todo list gets parsed in - * complete_action(). */ - if (!initial) - return todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo); + if (todo_list_parse_insn_buffer(r, new_todo->buf.buf, new_todo)) { + fprintf(stderr, _(edit_todo_list_advice)); + return -4; + } + + if (todo_list_check(todo_list, new_todo)) + return -4; return 0; } @@ -189,6 +193,8 @@ int todo_list_check(struct todo_list *old_todo, struct todo_list *new_todo) "the level of warnings.\n" "The possible behaviours are: ignore, warn, error.\n\n")); + fprintf(stderr, _(edit_todo_list_advice)); + leave_check: clear_commit_seen(&commit_seen); return res; diff --git a/sequencer.c b/sequencer.c index 181bb35f5f..75d5ad0496 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4271,8 +4271,20 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts) if (read_populate_opts(opts)) return -1; if (is_rebase_i(opts)) { + struct todo_list backup = TODO_LIST_INIT; + if ((res = read_populate_todo(r, &todo_list, opts))) goto release_todo_list; + + if (strbuf_read_file(&backup.buf, rebase_path_todo_backup(), 0) > 0) { + todo_list_parse_insn_buffer(r, backup.buf.buf, &backup); + res = todo_list_check(&backup, &todo_list); + todo_list_release(&backup); + + if (res) + goto release_todo_list; + } + if (commit_staged_changes(r, opts, &todo_list)) return -1; } else if (!file_exists(get_todo_path(opts))) @@ -4986,12 +4998,6 @@ int todo_list_write_to_file(struct repository *r, struct todo_list *todo_list, return res; } -static const char edit_todo_list_advice[] = -N_("You can fix this with 'git rebase --edit-todo' " -"and then run 'git rebase --continue'.\n" -"Or you can abort the rebase with 'git rebase" -" --abort'.\n"); - /* skip picking commits whose parents are unchanged */ static int skip_unnecessary_picks(struct repository *r, struct todo_list *todo_list, @@ -5089,11 +5095,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla todo_list_release(&new_todo); return error(_("nothing to do")); - } - - if (todo_list_parse_insn_buffer(r, new_todo.buf.buf, &new_todo) || - todo_list_check(todo_list, &new_todo)) { - fprintf(stderr, _(edit_todo_list_advice)); + } else if (res == -4) { checkout_onto(r, opts, onto_name, &onto->object.oid, orig_head); todo_list_release(&new_todo); diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 29a35840ed..9051c1e11d 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1343,6 +1343,89 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' ' test B = $(git cat-file commit HEAD^ | sed -ne \$p) ' +test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = ignore' ' + test_config rebase.missingCommitsCheck ignore && + rebase_setup_and_clean missing-commit && + set_fake_editor && + FAKE_LINES="break 1 2 3 4 5" git rebase -i --root && + FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual && + git rebase --continue 2>actual && + test D = $(git cat-file commit HEAD | sed -ne \$p) && + test_i18ngrep \ + "Successfully rebased and updated refs/heads/missing-commit" \ + actual +' + +test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = warn' ' + cat >expect <<-EOF && + error: invalid line 1: badcmd $(git rev-list --pretty=oneline --abbrev-commit -1 master~4) + Warning: some commits may have been dropped accidentally. + Dropped commits (newer to older): + - $(git rev-list --pretty=oneline --abbrev-commit -1 master) + To avoid this message, use "drop" to explicitly remove a commit. + EOF + tail -n4 expect >expect.2 && + test_config rebase.missingCommitsCheck warn && + rebase_setup_and_clean missing-commit && + set_fake_editor && + test_must_fail env FAKE_LINES="bad 1 2 3 4 5" \ + git rebase -i --root && + cp .git/rebase-merge/git-rebase-todo.backup orig && + FAKE_LINES="2 3 4" git rebase --edit-todo 2>actual.2 && + head -n5 actual.2 >actual && + test_i18ncmp expect actual && + cp orig .git/rebase-merge/git-rebase-todo && + FAKE_LINES="1 2 3 4" git rebase --edit-todo 2>actual.2 && + head -n4 actual.2 >actual && + test_i18ncmp expect.2 actual && + git rebase --continue 2>actual && + test D = $(git cat-file commit HEAD | sed -ne \$p) && + test_i18ngrep \ + "Successfully rebased and updated refs/heads/missing-commit" \ + actual +' + +test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = error' ' + cat >expect <<-EOF && + error: invalid line 1: badcmd $(git rev-list --pretty=oneline --abbrev-commit -1 master~4) + Warning: some commits may have been dropped accidentally. + Dropped commits (newer to older): + - $(git rev-list --pretty=oneline --abbrev-commit -1 master) + To avoid this message, use "drop" to explicitly remove a commit. + + Use '\''git config rebase.missingCommitsCheck'\'' to change the level of warnings. + The possible behaviours are: ignore, warn, error. + + You can fix this with '\''git rebase --edit-todo'\'' and then run '\''git rebase --continue'\''. + Or you can abort the rebase with '\''git rebase --abort'\''. + EOF + tail -n10 expect >expect.2 && + test_config rebase.missingCommitsCheck error && + rebase_setup_and_clean missing-commit && + set_fake_editor && + test_must_fail env FAKE_LINES="bad 1 2 3 4 5" \ + git rebase -i --root && + cp .git/rebase-merge/git-rebase-todo.backup orig && + test_must_fail env FAKE_LINES="2 3 4" \ + git rebase --edit-todo 2>actual && + test_i18ncmp expect actual && + test_must_fail git rebase --continue 2>actual && + test_i18ncmp expect actual && + cp orig .git/rebase-merge/git-rebase-todo && + test_must_fail env FAKE_LINES="1 2 3 4" \ + git rebase --edit-todo 2>actual && + test_i18ncmp expect.2 actual && + test_must_fail git rebase --continue 2>actual && + test_i18ncmp expect.2 actual && + cp orig .git/rebase-merge/git-rebase-todo && + FAKE_LINES="1 2 3 4 drop 5" git rebase --edit-todo && + git rebase --continue 2>actual && + test D = $(git cat-file commit HEAD | sed -ne \$p) && + test_i18ngrep \ + "Successfully rebased and updated refs/heads/missing-commit" \ + actual +' + test_expect_success 'respects rebase.abbreviateCommands with fixup, squash and exec' ' rebase_setup_and_clean abbrevcmd && test_commit "first" file1.txt "first line" first &&
When set to "warn" or "error", `rebase.missingCommitsCheck' would make `rebase -i' warn if the user removed commits from the todo list to prevent mistakes. Unfortunately, `rebase --edit-todo' and `rebase --continue' don't take it into account. This adds the ability for `rebase --edit-todo' and `rebase --continue' to check if commits were dropped by the user. As both edit_todo_list() and complete_action() parse the todo list and check for dropped commits, the code doing so in the latter is removed to reduce duplication. `edit_todo_list_advice' is removed from sequencer.c as it is no longer used there. This changes when a backup of the todo list is made. Until now, it was saved only before the initial edit. Now, it is always performed before the todo list is edited. Without this, sequencer_continue() (`rebase --continue') could only compare the current todo list against the original, unedited list. Before this change, this file was only used by edit_todo_list() and `rebase -p' to create the backup before the initial edit, and check_todo_list_from_file(), only used by `rebase -p' to check for dropped commits after its own initial edit. Three tests are added to t3404. The tests for `rebase.missingCommitsCheck = warn' and `rebase.missingCommitsCheck = error' have a similar structure. First, we start a rebase with an incorrect command on the first line. Then, we edit the todo list, removing the first and the last lines. This demonstrates that `--edit-todo' notices dropped commits, but not when the command is incorrect. Then, we restore the original todo list, and edit it to remove the last line. This demonstrates that if we add a commit after the initial edit, then remove it, `--edit-todo' will notice that it has been dropped. Then, the actual rebase takes place. In the third test, it is also checked that `--continue' will refuse to resume the rebase if commits were dropped. Signed-off-by: Alban Gruin <alban.gruin@gmail.com> --- rebase-interactive.c | 22 ++++++---- sequencer.c | 24 +++++----- t/t3404-rebase-interactive.sh | 83 +++++++++++++++++++++++++++++++++++ 3 files changed, 110 insertions(+), 19 deletions(-)