Re: [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 ##