[PATCH v2 00/10] sequencer: do not record dropped commits as rewritten

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.

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.

Patch 2 moves a function so it can be called without a forward declaration in Patch 11.

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

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

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

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

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

Phillip Wood (10):
  t3400: restore coverage for note copying with apply backend
  sequencer: move definition of is_final_fixup()
  sequencer: be more careful with external merge
  sequencer: never reschedule on failed commit
  sequencer: remove unnecessary "or" in pick_one_commit()
  sequencer: simplify handing 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                   | 154 +++++++++++++++++++++++-----------
 t/t3400-rebase.sh             |  16 +++-
 t/t3404-rebase-interactive.sh |  11 +++
 t/t5407-post-rewrite-hook.sh  |  23 +++++
 4 files changed, 155 insertions(+), 49 deletions(-)

Range-diff against v1:
 1:  65af2ac07a2 =  1:  65af2ac07a2 t3400: restore coverage for note copying with apply backend
 2:  02670f57e7d =  2:  02670f57e7d sequencer: move definition of is_final_fixup()
 3:  16fba1e823b !  3:  3d79362332c sequencer: be more careful with external merge
    @@ sequencer.c: static int do_pick_commit(struct repository *r,
     +					opts->xopts.nr, opts->xopts.v,
      					common, oid_to_hex(&head), remotes);
     +		/*
    -+		 * If the there were conflicts, try_merge_command() returns 1,
    ++		 * If there were conflicts, try_merge_command() returns 1,
     +		 * any other no-zero return code means that either the merge
     +		 * command could not be run, or it failed to merge.
     +		 */
 4:  3ffd06d6509 !  4:  fc89e77c6e8 sequencer: never reschedule on failed commit
    @@ sequencer.c: static int do_pick_commit(struct repository *r,
      			*check_todo = 1;
      		}
     +		/*
    -+		 * If "git commit" failed to run than res == -1 but we dont
    ++		 * If "git commit" failed to run then res == -1, but we don't
     +		 * want reschedule the last command because the picking the
     +		 * commit was successful.
     +		 */
 5:  cb286ac70d7 !  5:  26eef6c0958 sequencer: remove unnecessary "or" in pick_one_commit()
    @@ Commit message
     
         If error_with_patch(..., res, ...) succeeds then it returns "res", if
         it fails then it returns -1. This means that or-ing the return value
    -    with "res" is pointless the result is the same as the return value.
    +    with "res" is pointless as the result is the same as the return value.
     
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
 6:  1585d47e2ea =  6:  26dc48951ce sequencer: simplify handing of fixup with conflicts
 7:  4386ca67d10 =  7:  71ed717d322 sequencer: remove unnecessary condition in pick_one_commit()
 8:  f51751fa3ec !  8:  e8b7fa4c59e sequencer: simplify pick_one_commit()
    @@ Commit message
         sequencer: simplify pick_one_commit()
     
         Unless we're rebasing all we do in pick_one_commit() is call
    -    do_pick_commit() and return its result. Simplify the code by returing
    +    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
         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.
     
         As the conditional blocks are all mutually exclusive (either the
         conditions are mutually exclusive, or an earlier conditional block
         that would match a later one contains a "return" statement) chain
         them together with "else if" to make that clear.
    +
    +    While we could remove "res" from the conditions below "if (!res)"
    +    they are left alone because, when we start using an enum in the next
    +    commit, it makes it clear that these clauses are handling cases where
    +    there are conflicts.
     
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
    @@ sequencer.c: static int pick_one_commit(struct repository *r,
      		record_in_rewritten(&item->commit->object.oid,
      				    peek_command(todo_list, 1));
     -	if (res && is_fixup(item->command)) {
    ++		return 0;
     +	} else if (res && is_fixup(item->command)) {
      		return error_failed_squash(r, item->commit, opts,
      					   item->arg_len, arg);
    @@ sequencer.c: static int pick_one_commit(struct repository *r,
      		int to_amend = 0;
      		struct object_id oid;
      
    +@@ sequencer.c: static int pick_one_commit(struct repository *r,
    + 		return error_with_patch(r, item->commit, arg, item->arg_len,
    + 					opts, res, to_amend);
    + 	}
    +-	return res;
    ++
    ++	BUG("Unhandled return value from do_pick_commit()");
    + }
    + 
    + static int pick_commits(struct repository *r,
 9:  2541a4d6e3d <  -:  ----------- sequencer: return early from pick_one_commit() on success
10:  e4050ead27f =  9:  4fb641afb3c sequencer: use an enum to represent result of picking a commit
11:  26551f2687b ! 10:  c89234dd949 sequencer: do not record dropped commits as rewritten
    @@ Commit message
         when rewording a fast-forwarded commit.
     
         Reported-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
    +    Tested-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
         Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
     
      ## sequencer.c ##