[PATCH v3 0/9] sequencer: do not record dropped commits as rewritten

Thanks to everyone who commented on v2. I've dropped patch 2 which Andrei pointed out was pointless and tried to make the remaining commit messages clearer as requested by Oswald.

If a commit gets dropped because its changes are already upstream then we should not record it as rewritten. As well as confusing any post-rewrite hooks this means we end up copying the notes from the dropped commit to the commit that was picked immediately before the one that was dropped.

This series is structured as follows:

Patch 1 restores some test coverage that was lost when the default rebase backend was changed.

Patches 2 & 3 fix the return value of do_pick_commit() when an external command fails (this is in preparation for patch 8).

Patches 4-7 try and simplify the control flow in pick_one_commit() in preparation for patch 8.

Patch 8 changes the return type of do_pick_commit() to an enum.

Patch 9 adds a new member to the enum from patch 8 for commits that are dropped when they become empty and uses that to stop them from being recorded as rewritten.

Cover letter for v2:

Thanks to everyone who commented on v1. I've squashed the fixups that Junio had in "seen", squashed patches 8 & 9 together as suggested by Oswald and expanded the commit message, and added Uwe's Tested-by: trailer to the final patch. Oswald suggested extended the use of the enum which I think is a good idea in the long-term but I punted on that for now because I think it would be fairly invasive and this series has enough refactoring in it already.

base-commit: 6c3d7b73556db708feb3b16232fab1efc4353428
Published-As: https://github.com/phillipwood/git/releases/tag/pw%2Frebase-drop-notes-with-commit%2Fv3
View-Changes-At: https://github.com/phillipwood/git/compare/6c3d7b735...2ef36b9ee
Fetch-It-Via: git fetch https://github.com/phillipwood/git pw/rebase-drop-notes-with-commit/v3

Phillip Wood (9):
  t3400: restore coverage for note copying with apply backend
  sequencer: be more careful with external merge
  sequencer: never reschedule on failed commit
  sequencer: remove unnecessary "or" in pick_one_commit()
  sequencer: simplify handling of fixup with conflicts
  sequencer: remove unnecessary condition in pick_one_commit()
  sequencer: simplify pick_one_commit()
  sequencer: use an enum to represent result of picking a commit
  sequencer: do not record dropped commits as rewritten

 sequencer.c                   | 124 +++++++++++++++++++++++++---------
 t/t3400-rebase.sh             |  16 ++++-
 t/t3404-rebase-interactive.sh |  11 +++
 t/t5407-post-rewrite-hook.sh  |  23 +++++++
 4 files changed, 140 insertions(+), 34 deletions(-)

Range-diff against v2:
 1:  65af2ac07a2 !  1:  c4705066ee0 t3400: restore coverage for note copying with apply backend
    @@ Metadata
      ## Commit message ##
         t3400: restore coverage for note copying with apply backend
     
    -    Now that the merge backend is the default we have lost coverage for
    +    Now that the merge backend is the default, we have lost coverage for
         "git rebase --apply" copying notes. Fix this by replacing "-m" with
         "--apply" as the previous test which uses the default backend now
         checks the merge backend.
 2:  02670f57e7d <  -:  ----------- sequencer: move definition of is_final_fixup()
 3:  3d79362332c !  2:  947bb77e44f sequencer: be more careful with external merge
    @@ Commit message
     
         If an external merge strategy cannot merge (for example because it
         would overwrite an untracked file) it exits with a non-zero exit
    -    code other than 1. This should be treated differently to a merge
    -    with conflicts which is signalled by an exit code of 1 because as
    -    the merge failed we need to reschedule the last pick. The caller
    +    code other than 1. This should be treated differently from a merge
    +    with conflicts, which is signaled by an exit code of 1, because, as
    +    the merge failed, we need to reschedule the last pick. The caller
         expects us to return -1 in this case. Also reschedule without trying
         to merge if the commit message cannot be written as that prevents us
         from successfully picking the commit.
 4:  fc89e77c6e8 =  3:  bff5f319e91 sequencer: never reschedule on failed commit
 5:  26eef6c0958 =  4:  e785433ad3d sequencer: remove unnecessary "or" in pick_one_commit()
 6:  26dc48951ce !  5:  134d8f7e935 sequencer: simplify handing of fixup with conflicts
    @@ Metadata
     Author: Phillip Wood <phillip.wood@dunelm.org.uk>
     
      ## Commit message ##
    -    sequencer: simplify handing of fixup with conflicts
    +    sequencer: simplify handling of fixup with conflicts
     
         Commit e032abd5a0 (rebase: fix rewritten list for failed pick,
    -    2023-09-06) introduced an early return when res == -1, so if we enter
    -    this conditional block then res is positive. After the last couple
    -    of commits the only possible positive value is 1 so we can simplify
    -    the code by removing the conditional call to intend_to_amend() and
    -    call it error_with_patch() instead.
    +    2023-09-06) introduced an early return when res == -1, so if
    +    we enter this conditional block then res is positive. After the
    +    last couple of commits the only possible positive value is 1. That
    +    means we can simplify the code by removing the conditional call to
    +    intend_to_amend() and have error_failed_squash() request that it is
    +    called in error_with_patch() instead.
     
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
 7:  71ed717d322 =  6:  e3091dee633 sequencer: remove unnecessary condition in pick_one_commit()
 8:  e8b7fa4c59e !  7:  7c1642b0a49 sequencer: simplify pick_one_commit()
    @@ Metadata
      ## Commit message ##
         sequencer: simplify pick_one_commit()
     
    -    Unless we're rebasing all we do in pick_one_commit() is call
    +    Unless we're rebasing, all we do in pick_one_commit() is call
         do_pick_commit() and return its result. Simplify the code by returning
    -    early if we're not rebasing so that we don't have to continually call
    +    early if we're not rebasing so that we don't have to repeatedly call
         is_rebase_i() in the rest of the function. Note that there are a couple
         of conditions that do not call is_rebase_i() but they check for either
         an "edit" or a "fixup" command, both of which imply we're rebasing.
     
         The only block that does not return early is the one guarded by
         "!res". Move the return into that block to make it clear that after
    -    recording the commit as rewritten all we do is return from the function.
    +    recording the commit as rewritten, all we do is return from the
    +    function.
     
         As the conditional blocks are all mutually exclusive (either the
         conditions are mutually exclusive, or an earlier conditional block
 9:  4fb641afb3c !  8:  0a146d57266 sequencer: use an enum to represent result of picking a commit
    @@ Metadata
      ## Commit message ##
         sequencer: use an enum to represent result of picking a commit
     
    -    Rather than using an integer where -1 is an error, 0 is success and
    -    1 means there were conflicts use an enum. This is clearer and lets
    +    Rather than using an integer where -1 is an error, 0 is success and 1
    +    indicates there were conflicts, use an enum. This is clearer and lets
         us add a separate return value for commits that are dropped because
         they become empty in the next commit.
     
10:  c89234dd949 !  9:  2ef36b9ee5a sequencer: do not record dropped commits as rewritten
    @@ Commit message
     
         If a commit gets dropped because its changes are already upstream
         then we should not record it as rewritten. As well as confusing any
    -    post-rewrite hooks this means we end up copying the notes from the
    +    post-rewrite hooks, it means we end up copying the notes from the
         dropped commit to the commit that was picked immediately before the
         one that was dropped.
     
    -    While we do not want to record the dropped commit is rewritten, if
    +    While we do not want to record the dropped commit as rewritten, if
         it is the final commit in a chain of fixups then we need to flush
         the list of rewritten commits. The behavior of an "edit" command
         where the commit is dropped is changed so that "rebase --continue"