[PATCH v2 0/6] rebase -i: impove handling of failed commands

["Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com>]

This series fixes several bugs in the way we handle a commit cannot be
picked because it would overwrite an untracked file.

 * after a failed pick "git rebase --continue" will happily commit any
   staged changes even though no commit was picked.

 * the commit of the failed pick is recorded as rewritten even though no
   commit was picked.

 * the "done" file used by "git status" to show the recently executed
   commands contains an incorrect entry.

Thanks for the comments on V1, this series has now grown somewhat.
Previously I was worried that refactoring would change the behavior, but
having thought about it the current behavior is wrong and should be changed.

Changes since V1:

Rebased onto master to avoid a conflict with
ab/remove-implicit-use-of-the-repository

 * Patches 1-3 are new preparatory changes
 * Patches 4 & 5 are new and fix the first two issues listed above.
 * Patch 6 is the old patch 1 which has been rebased and the commit message
   reworded. It fixes the last issues listed above.

Phillip Wood (6):
  rebase -i: move unlink() calls
  rebase -i: remove patch file after conflict resolution
  sequencer: factor out part of pick_commits()
  rebase --continue: refuse to commit after failed command
  rebase: fix rewritten list for failed pick
  rebase -i: fix adding failed command to the todo list

 sequencer.c                   | 170 ++++++++++++++++++----------------
 t/t3404-rebase-interactive.sh |  49 +++++++---
 t/t3430-rebase-merges.sh      |  35 +++++--
 t/t5407-post-rewrite-hook.sh  |  11 +++
 4 files changed, 165 insertions(+), 100 deletions(-)


base-commit: 9c6990cca24301ae8f82bf6291049667a0aef14b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1492%2Fphillipwood%2Frebase-dont-write-done-when-rescheduling-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1492/phillipwood/rebase-dont-write-done-when-rescheduling-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1492

Range-diff vs v1:

 -:  ----------- > 1:  3dfb2c6903b rebase -i: move unlink() calls
 -:  ----------- > 2:  227aea031b5 rebase -i: remove patch file after conflict resolution
 -:  ----------- > 3:  31bb644e769 sequencer: factor out part of pick_commits()
 -:  ----------- > 4:  9356d14b09a rebase --continue: refuse to commit after failed command
 -:  ----------- > 5:  f8e64c1b631 rebase: fix rewritten list for failed pick
 1:  dc51a7499bc ! 6:  a836b049b90 rebase -i: do not update "done" when rescheduling command
     @@ Metadata
      Author: Phillip Wood <phillip.wood@dunelm.org.uk>
      
       ## Commit message ##
     -    rebase -i: do not update "done" when rescheduling command
     +    rebase -i: fix adding failed command to the todo list
      
     -    As the sequencer executes todo commands it appends them to
     -    .git/rebase-merge/done. This file is used by "git status" to show the
     -    recently executed commands. Unfortunately when a command is rescheduled
     +    When rebasing commands are moved from the todo list in "git-rebase-todo"
     +    to the "done" file (which is used by "git status" to show the recently
     +    executed commands) just before they are executed. This means that if a
     +    command fails because it would overwrite an untracked file it has to be
     +    added back into the todo list before the rebase stops for the user to
     +    fix the problem.
     +
     +    Unfortunately when a failed command is added back into the todo list
          the command preceding it is erroneously appended to the "done" file.
     -    This means that when rebase stops after rescheduling "pick B" the "done"
     +    This means that when rebase stops after "pick B" fails the "done"
          file contains
      
                  pick A
     @@ Commit message
                  pick A
                  pick B
      
     -    Fix this by not updating the "done" file when adding a rescheduled
     -    command back into the "todo" file. A couple of the existing tests are
     +    Fix this by not updating the "done" file when adding a failed command
     +    back into the "git-rebase-todo" file. A couple of the existing tests are
          modified to improve their coverage as none of them trigger this bug or
          check the "done" file.
      
     -    Note that the rescheduled command will still be appended to the "done"
     -    file again when it is successfully executed. Arguably it would be better
     -    not to do that but fixing it would be more involved.
     -
          Reported-by: Stefan Haller <lists@haller-berlin.de>
          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
      
     @@ sequencer.c: static int pick_commits(struct repository *r,
       		int check_todo = 0;
       
      -		if (save_todo(todo_list, opts))
     -+		if (save_todo(todo_list, opts, 0))
     ++		if (save_todo(todo_list, opts, reschedule))
       			return -1;
       		if (is_rebase_i(opts)) {
       			if (item->command != TODO_COMMENT) {
     -@@ sequencer.c: static int pick_commits(struct repository *r,
     - 							    todo_list->current),
     - 				       get_item_line(todo_list,
     - 						     todo_list->current));
     --				todo_list->current--;
     --				if (save_todo(todo_list, opts))
     -+				if (save_todo(todo_list, opts, 1))
     - 					return -1;
     - 			}
     - 			if (item->command == TODO_EDIT) {
      @@ sequencer.c: static int pick_commits(struct repository *r,
       			       get_item_line_length(todo_list,
       						    todo_list->current),
       			       get_item_line(todo_list, todo_list->current));
      -			todo_list->current--;
      -			if (save_todo(todo_list, opts))
     -+			if (save_todo(todo_list, opts, 1))
     ++			if (save_todo(todo_list, opts, reschedule))
       				return -1;
       			if (item->commit)
     - 				return error_with_patch(r,
     + 				write_rebase_head(&item->commit->object.oid);
      
       ## t/t3404-rebase-interactive.sh ##
      @@ t/t3404-rebase-interactive.sh: test_expect_success 'todo count' '
     @@ t/t3404-rebase-interactive.sh: test_expect_success 'todo count' '
      +	head -n3 todo >expect &&
      +	test_cmp expect .git/rebase-merge/done &&
      +	rm file2 &&
     + 	test_path_is_missing .git/rebase-merge/author-script &&
     + 	test_path_is_missing .git/rebase-merge/patch &&
     + 	test_path_is_missing .git/MERGE_MSG &&
     +@@ t/t3404-rebase-interactive.sh: test_expect_success 'rebase -i commits that overwrite untracked files (pick)' '
     + 	grep "error: you have staged changes in your working tree" err &&
     + 	git reset --hard HEAD &&
       	git rebase --continue &&
      -	test_cmp_rev HEAD I
      +	test_cmp_rev HEAD D &&

-- 
gitgitgadget