From: Phillip Wood Date: Mon, 09 Dec 2019 16:00:14 GMT Subject: Re: [PATCH v3 2/2] rebase-interactive: warn if commit is dropped with `rebase --edit-todo' Message-ID: <64aa4049-ee35-df4c-1e6c-80707f4f9070@gmail.com> In-Reply-To: Hi Alban and Junio On 04/12/2019 19:19, Junio C Hamano wrote: > Alban Gruin 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 >> --- >> 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