| Message ID | 20191202234759.26201-1-alban.gruin@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | rebase -i: extend rebase.missingCommitsCheck | expand |
Alban Gruin <alban.gruin@gmail.com> writes: > To prevent mistakes when editing a branch, rebase features a knob, > rebase.missingCommitsCheck, to warn the user if a commit was dropped. > Unfortunately, this check is only effective for the initial edit, which > means that if you edit the todo list at a later point of the rebase and > drop a commit, no warnings or errors would be issued. > ... > rebase-interactive.c | 57 ++++++++++++++++++++---- > rebase-interactive.h | 2 + > sequencer.c | 53 ++++++---------------- > sequencer.h | 1 - > t/t3404-rebase-interactive.sh | 83 +++++++++++++++++++++++++++++++++++ > 5 files changed, 147 insertions(+), 49 deletions(-) This passes the self-test when tested by itself, but when merged near the tip of 'pu', it breaks t3404.116, it seems.
Hi Junio, Le 04/12/2019 à 22:51, Junio C Hamano a écrit : > Alban Gruin <alban.gruin@gmail.com> writes: > >> To prevent mistakes when editing a branch, rebase features a knob, >> rebase.missingCommitsCheck, to warn the user if a commit was dropped. >> Unfortunately, this check is only effective for the initial edit, which >> means that if you edit the todo list at a later point of the rebase and >> drop a commit, no warnings or errors would be issued. >> ... >> rebase-interactive.c | 57 ++++++++++++++++++++---- >> rebase-interactive.h | 2 + >> sequencer.c | 53 ++++++---------------- >> sequencer.h | 1 - >> t/t3404-rebase-interactive.sh | 83 +++++++++++++++++++++++++++++++++++ >> 5 files changed, 147 insertions(+), 49 deletions(-) > > This passes the self-test when tested by itself, but when merged > near the tip of 'pu', it breaks t3404.116, it seems. > After a quick investigation, it comes from pw/post-commit-from-sequencer. Since then, tests are expected to setup the editor and run the commands using it in a subshell. So the fix is straightforward. Perhaps I should take ag/sequencer-todo-updates, merge pw/post-commit-from-sequencer, rebase this series onto the result, fix the issue, and reroll the series? Cheers, Alban
Hi Alban On 05/12/2019 23:15, Alban Gruin wrote: > Hi Junio, > > Le 04/12/2019 à 22:51, Junio C Hamano a écrit : >> Alban Gruin <alban.gruin@gmail.com> writes: >> >>> To prevent mistakes when editing a branch, rebase features a knob, >>> rebase.missingCommitsCheck, to warn the user if a commit was dropped. >>> Unfortunately, this check is only effective for the initial edit, which >>> means that if you edit the todo list at a later point of the rebase and >>> drop a commit, no warnings or errors would be issued. >>> ... >>> rebase-interactive.c | 57 ++++++++++++++++++++---- >>> rebase-interactive.h | 2 + >>> sequencer.c | 53 ++++++---------------- >>> sequencer.h | 1 - >>> t/t3404-rebase-interactive.sh | 83 +++++++++++++++++++++++++++++++++++ >>> 5 files changed, 147 insertions(+), 49 deletions(-) >> >> This passes the self-test when tested by itself, but when merged >> near the tip of 'pu', it breaks t3404.116, it seems. >> > > After a quick investigation, it comes from > pw/post-commit-from-sequencer. Since then, tests are expected to setup > the editor and run the commands using it in a subshell. So the fix is > straightforward. > > Perhaps I should take ag/sequencer-todo-updates, merge > pw/post-commit-from-sequencer, rebase this series onto the result, fix > the issue, and reroll the series? If the issue is just using a subshell to set the editor then (assuming you're only adding new tests) I don't think you need to rebase - just change your tests and it should be fine when Junio merges it into pu. I'm sorry I've not looked at the latest version yet, I'll try and get round to it next week. Best Wishes Phillip > Cheers, > Alban >
Hi Alban, On Fri, 6 Dec 2019, Phillip Wood wrote: > On 05/12/2019 23:15, Alban Gruin wrote: > > > > Le 04/12/2019 à 22:51, Junio C Hamano a écrit : > > > Alban Gruin <alban.gruin@gmail.com> writes: > > > > > > > To prevent mistakes when editing a branch, rebase features a knob, > > > > rebase.missingCommitsCheck, to warn the user if a commit was dropped. > > > > Unfortunately, this check is only effective for the initial edit, which > > > > means that if you edit the todo list at a later point of the rebase and > > > > drop a commit, no warnings or errors would be issued. > > > > ... > > > > rebase-interactive.c | 57 ++++++++++++++++++++---- > > > > rebase-interactive.h | 2 + > > > > sequencer.c | 53 ++++++---------------- > > > > sequencer.h | 1 - > > > > t/t3404-rebase-interactive.sh | 83 +++++++++++++++++++++++++++++++++++ > > > > 5 files changed, 147 insertions(+), 49 deletions(-) > > > > > > This passes the self-test when tested by itself, but when merged > > > near the tip of 'pu', it breaks t3404.116, it seems. > > > > > > > After a quick investigation, it comes from > > pw/post-commit-from-sequencer. Since then, tests are expected to setup > > the editor and run the commands using it in a subshell. So the fix is > > straightforward. > > > > Perhaps I should take ag/sequencer-todo-updates, merge > > pw/post-commit-from-sequencer, rebase this series onto the result, fix > > the issue, and reroll the series? > > If the issue is just using a subshell to set the editor then (assuming you're > only adding new tests) I don't think you need to rebase - just change your > tests and it should be fine when Junio merges it into pu. I'm sorry I've not > looked at the latest version yet, I'll try and get round to it next week. Just squash this in: -- snipsnap -- Subject: [PATCH] fixup??? rebase-interactive: warn if commit is dropped with `rebase --edit-todo' This is required to appease the interaction with `pw/post-commit-from-sequencer`. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> --- t/t3404-rebase-interactive.sh | 74 +++++++++++++++++++---------------- 1 file changed, 40 insertions(+), 34 deletions(-) diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index e3f64bc2a59..72718d0d839 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1453,10 +1453,12 @@ test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' ' 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 && + ( + 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" \ @@ -1474,18 +1476,20 @@ test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = wa 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 && + ( + 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" \ @@ -1509,24 +1513,26 @@ test_expect_success 'rebase --edit-todo respects rebase.missingCommitsCheck = er 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 && + ( + 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" \ -- 2.24.0.windows.2.611.ge9aced84530
To prevent mistakes when editing a branch, rebase features a knob, rebase.missingCommitsCheck, to warn the user if a commit was dropped. Unfortunately, this check is only effective for the initial edit, which means that if you edit the todo list at a later point of the rebase and drop a commit, no warnings or errors would be issued. This adds the ability to check if commits were dropped when editing the todo list with `--edit-todo', and when resuming a rebase. The first patch moves moves check_todo_list_from_file() and `edit_todo_list_advice' from sequencer.c to rebase-interactive.c so the latter can be used by edit_todo_list() and todo_list_check(). The second patch adds the check to `--edit-todo' and `--continue' and tests. This is based on 393adf7a6f ("sequencer: directly call pick_commits() from complete_action()", 2019-11-24). The tip of this series is tagged as "edit-todo-drop-v3" at https://github.com/agrn/git. Changes since v2: - Merged patches 1 and 2. - Reinstated support for `git rebase --continue'. - Print `edit_todo_list_advice' if a dropped commit was detected by `--edit-todo' or `--continue'. Alban Gruin (2): sequencer: move check_todo_list_from_file() to rebase-interactive.c rebase-interactive: warn if commit is dropped with `rebase --edit-todo' rebase-interactive.c | 57 ++++++++++++++++++++---- rebase-interactive.h | 2 + sequencer.c | 53 ++++++---------------- sequencer.h | 1 - t/t3404-rebase-interactive.sh | 83 +++++++++++++++++++++++++++++++++++ 5 files changed, 147 insertions(+), 49 deletions(-) Diff-intervalle contre v2: -: ---------- > 1: 996045a300 sequencer: move check_todo_list_from_file() to rebase-interactive.c 1: 6974b6c8f2 ! 2: ba6d27e5b4 t3404: demonstrate that --edit-todo does not check for dropped commits @@ Metadata Author: Alban Gruin <alban.gruin@gmail.com> ## Commit message ## - t3404: demonstrate that --edit-todo does not check for dropped commits + rebase-interactive: warn if commit is dropped with `rebase --edit-todo' - When set to "warn" or "error", `rebase.missingCommitCheck' would make - rebase -i warn if the user removed commits from the todo list to prevent - mistakes. Unfortunately, rebase --edit-todo don't take it into account. + 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 three tests to t3404 to demonstrate this. The first one is - not broken, as when `rebase.missingCommitsCheck' is not set, nothing in - particular must be done towards dropped commits. The two others are - broken, demonstrating the problem. + 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. - 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. + 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 ## +@@ rebase-interactive.c: 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 +@@ rebase-interactive.c: 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; +@@ rebase-interactive.c: 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; + } +@@ rebase-interactive.c: 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; + + ## sequencer.c ## +@@ sequencer.c: 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))) +@@ sequencer.c: 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, +@@ sequencer.c: 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); + + ## t/t3404-rebase-interactive.sh ## @@ t/t3404-rebase-interactive.sh: test_expect_success 'rebase -i respects rebase.missingCommitsCheck = error' ' test B = $(git cat-file commit HEAD^ | sed -ne \$p) @@ t/t3404-rebase-interactive.sh: test_expect_success 'rebase -i respects rebase.mi + actual +' + -+test_expect_failure 'rebase --edit-todo respects rebase.missingCommitsCheck = warn' ' ++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. @@ t/t3404-rebase-interactive.sh: test_expect_success 'rebase -i respects rebase.mi + actual +' + -+test_expect_failure 'rebase --edit-todo respects rebase.missingCommitsCheck = error' ' ++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 -n4 expect >expect.2 && ++ tail -n10 expect >expect.2 && + test_config rebase.missingCommitsCheck error && + rebase_setup_and_clean missing-commit && + set_fake_editor && @@ t/t3404-rebase-interactive.sh: test_expect_success 'rebase -i respects rebase.mi + 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.2 && -+ head -n5 actual.2 >actual && ++ 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.2 && -+ head -n4 actual.2 >actual && ++ 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 && 2: a4a700ce8b < -: ---------- rebase-interactive: warn if commit is dropped with --edit-todo