Re: [PATCH v2 0/3] commit: refuse to amend during conflict resolution

> Both git commit --amend and a partial commit (git commit <paths>) are
> foot-guns while the user is in the middle of an operation that resolves
> conflicts on top of HEAD: recording a conflict resolution is about capturing
> the state of the whole tree as a new commit, not about rewriting HEAD or
> committing a subset of paths.
> 
> Historically we only rejected these during a merge or a cherry-pick or when
> resolving an empty pick during a rebase. The same hazard exists for am,
> revert, and rebase conflict stops, none of which were covered. This series
> extends the refusal to all of them.
> 
> The three patches:
> 
>   1. reword the two pre-existing "empty commit" rebase messages, which were
>      misleadingly generic
>   2. refuse git commit --amend during these additional operations
>   3. refuse partial commits during the same operations.

> Elijah Newren (3):
>    commit: reword the empty-commit rebase errors
>    commit: refuse to amend during conflict resolution
>    commit: refuse partial commits during conflict resolution
> 
>   builtin/commit.c                |  51 +++++++++----
>   sequencer.c                     |  65 +++++++++++++++++
>   sequencer.h                     |  24 ++++++
>   t/t3404-rebase-interactive.sh   | 125 +++++++++++++++++++++++++++++++-
>   t/t3507-cherry-pick-conflict.sh |  22 ++++++
>   t/t4151-am-abort.sh             |  22 ++++++
>   6 files changed, 293 insertions(+), 16 deletions(-)
> 
> 
> base-commit: 2c3adbb2c475981e340c79fdc5e7f4f9b5d9054e
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2389%2Fnewren%2Frefuse-amend-during-conflicts-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2389/newren/refuse-amend-during-conflicts-v2
> Pull-Request: https://github.com/git/git/pull/2389
> 
> Range-diff vs v1:
> 
>   -:  ---------- > 1:  65c48ed3cb commit: reword the empty-commit rebase errors
>   1:  a3d6b059c6 ! 2:  4a1461e527 commit: refuse to amend during conflict resolution
>       @@ Commit message
>            Signed-off-by: Elijah Newren <newren@gmail.com>
>        
>         ## builtin/commit.c ##
>       -@@
>       - #include "path.h"
>       - #include "preload-index.h"
>       - #include "read-cache.h"
>       -+#include "refs.h"
>       - #include "repository.h"
>       - #include "string-list.h"
>       - #include "rerere.h"
>        @@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *argv[],
>       - 		else if (whence == FROM_REBASE_PICK)
>       - 			die(_("You are in the middle of a rebase -- cannot amend."));
>       + 		use_editor = 0;
>       +
>       + 	/* Sanity check options */
>       +-	if (amend && !current_head)
>       +-		die(_("You have nothing to amend."));
>       +-	if (amend && whence != FROM_COMMIT) {
>       +-		if (whence == FROM_MERGE)
>       ++	if (amend) {
>       ++		if (!current_head)
>       ++			die(_("You have nothing to amend."));
>       ++		/*
>       ++		 * Refuse to amend in the middle of any operation that is
>       ++		 * meant to record its result as a new commit on top of HEAD
>       ++		 * rather than by rewriting HEAD.
>       ++		 */
>       ++		switch (sequencer_ongoing_operation(s->repo, whence)) {
>       ++		case ONGOING_NONE:
>       ++			break;
>       ++		case ONGOING_MERGE:
>       + 			die(_("You are in the middle of a merge -- cannot amend."));
>       +-		else if (is_from_cherry_pick(whence))
>       ++		case ONGOING_CHERRY_PICK:
>       + 			die(_("You are in the middle of a cherry-pick -- cannot amend."));
>       +-		else if (whence == FROM_REBASE_PICK)
>       ++		case ONGOING_REBASE_EMPTY:
>       + 			die(_("You are resolving a commit that became empty -- cannot amend."));
>       ++		case ONGOING_REVERT:
>       ++			die(_("You are in the middle of a revert -- cannot amend."));
>       ++		case ONGOING_AM:
>       ++			die(_("You are in the middle of an am session -- cannot amend."));
>       ++		case ONGOING_REBASE_CONFLICT:
>       ++			die(_("You are resolving conflicts during a rebase -- cannot amend."));
>       ++		}
>         	}
>       -+	if (amend && whence == FROM_COMMIT) {
>       -+		char *applying, *apply_dir, *stopped_sha, *amend_marker;
>       -+		int in_am, conflicted_stop;
>       + 	if (fixup_message && squash_message)
>       + 		die(_("options '%s' and '%s' cannot be used together"), "--squash", "--fixup");
>       +
>       + ## sequencer.c ##
>       +@@ sequencer.c: int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)
>       + 	return 0;
>       + }
>       +
>       ++enum ongoing_operation sequencer_ongoing_operation(struct repository *r,
>       ++						   enum commit_whence whence)
>       ++{
>       ++	char *path;
>       ++	int found;
>        +
>       -+		/* Check middle of revert */
>       -+		if (refs_ref_exists(get_main_ref_store(the_repository),
>       -+				    "REVERT_HEAD"))
>       -+			die(_("You are in the middle of a revert -- cannot amend."));
>       ++	/*
>       ++	 * The merge, cherry-pick, and (empty) rebase-pick stops are already
>       ++	 * distinguished by 'whence'.
>       ++	 */
>       ++	switch (whence) {
>       ++	case FROM_MERGE:
>       ++		return ONGOING_MERGE;
>       ++	case FROM_CHERRY_PICK_SINGLE:
>       ++	case FROM_CHERRY_PICK_MULTI:
>       ++		return ONGOING_CHERRY_PICK;
>       ++	case FROM_REBASE_PICK:
>       ++		return ONGOING_REBASE_EMPTY;
>       ++	case FROM_COMMIT:
>       ++		break;
>       ++	}
>        +
>       -+		/* Check middle of `am` */
>       -+		applying = repo_git_path(the_repository,
>       -+					 "rebase-apply/applying");
>       -+		in_am = file_exists(applying);
>       ++	/*
>       ++	 * 'whence' is FROM_COMMIT, but we may still be in the middle of an
>       ++	 * operation that records its result on top of HEAD; detect those
>       ++	 * from their on-disk state.
>       ++	 */
>        +
>       -+		free(applying);
>       -+		if (in_am)
>       -+			die(_("You are in the middle of an am session -- cannot amend."));
>       ++	/* In the middle of a revert? */
>       ++	if (refs_ref_exists(get_main_ref_store(r), "REVERT_HEAD"))
>       ++		return ONGOING_REVERT;
>        +
>       -+		/* Check middle of rebase specifically stopped for conflicts */
>       -+		apply_dir = repo_git_path(the_repository,
>       -+					  "rebase-apply");
>       -+		stopped_sha = repo_git_path(the_repository,
>       -+					    "rebase-merge/stopped-sha");
>       -+		amend_marker = repo_git_path(the_repository,
>       -+					     "rebase-merge/amend");
>       -+		/*
>       -+		 * The apply backend only ever stops for conflicts; the
>       -+		 * merge backend writes stopped-sha but omits `amend`,
>       -+		 * which it writes only at a clean edit/reword stop.
>       -+		 */
>       -+		conflicted_stop =
>       -+			file_exists(apply_dir) ||
>       -+			(file_exists(stopped_sha) && !file_exists(amend_marker));
>       ++	/* In the middle of an `am`? */
>       ++	path = repo_git_path(r, "rebase-apply/applying");
>       ++	found = file_exists(path);
>       ++	free(path);
>       ++	if (found)
>       ++		return ONGOING_AM;
>       ++
>       ++	/*
>       ++	 * In the middle of a rebase that stopped for conflict resolution?
>       ++	 * The apply backend only ever stops for conflicts, so the presence
>       ++	 * of its state directory is enough.  The merge backend writes
>       ++	 * stopped-sha whenever it hands control back to the user, but omits
>       ++	 * `amend` unless it stopped with HEAD already pointing at the commit
>       ++	 * to be amended (a clean edit/reword stop); its absence therefore
>       ++	 * marks a conflicted stop.
>       ++	 */
>       ++	path = repo_git_path(r, "rebase-apply");
>       ++	found = file_exists(path);
>       ++	free(path);
>       ++	if (!found) {
>       ++		char *stopped_sha = repo_git_path(r, "rebase-merge/stopped-sha");
>       ++		char *amend_marker = repo_git_path(r, "rebase-merge/amend");
>        +
>       -+		free(apply_dir);
>       ++		found = file_exists(stopped_sha) && !file_exists(amend_marker);
>        +		free(stopped_sha);
>        +		free(amend_marker);
>       -+		if (conflicted_stop)
>       -+			die(_("You are resolving conflicts during a rebase -- cannot amend."));
>        +	}
>       - 	if (fixup_message && squash_message)
>       - 		die(_("options '%s' and '%s' cannot be used together"), "--squash", "--fixup");
>       - 	die_for_incompatible_opt4(!!use_message, "-C",
>       ++	if (found)
>       ++		return ONGOING_REBASE_CONFLICT;
>       ++
>       ++	return ONGOING_NONE;
>       ++}
>       ++
>       + int sequencer_get_update_refs_state(const char *wt_dir,
>       + 				    struct string_list *refs)
>       + {
>       +
>       + ## sequencer.h ##
>       +@@ sequencer.h: int sequencer_get_last_command(struct repository* r,
>       + 			       enum replay_action *action);
>       + int sequencer_determine_whence(struct repository *r, enum commit_whence *whence);
>       +
>       ++/*
>       ++ * An in-progress operation that records its result (often a conflict
>       ++ * resolution) as a new commit on top of HEAD, during which amending
>       ++ * HEAD via "git commit --amend" is almost always a mistake.
>       ++ */
>       ++enum ongoing_operation {
>       ++	ONGOING_NONE = 0,
>       ++	ONGOING_MERGE,
>       ++	ONGOING_CHERRY_PICK,
>       ++	ONGOING_REBASE_EMPTY,
>       ++	ONGOING_REVERT,
>       ++	ONGOING_AM,
>       ++	ONGOING_REBASE_CONFLICT
>       ++};
>       ++
>       ++/*
>       ++ * Return which in-progress operation, if any, is underway; see enum
>       ++ * ongoing_operation.  'whence' is the origin already computed for the
>       ++ * pending commit.
>       ++ */
>       ++enum ongoing_operation sequencer_ongoing_operation(struct repository *r,
>       ++						   enum commit_whence whence);
>       ++
>       + /**
>       +  * Append the set of ref-OID pairs that are currently stored for the 'git
>       +  * rebase --update-refs' feature if such a rebase is currently happening.
>        
>         ## t/t3404-rebase-interactive.sh ##
>        @@ t/t3404-rebase-interactive.sh: test_expect_success 'correct error message for commit --amend after empty pick'
>       - 	test_grep "middle of a rebase -- cannot amend." err
>       + 	test_grep "resolving a commit that became empty -- cannot amend." err
>         '
>         
>        +test_expect_success 'commit --amend is refused at a rebase conflict stop' '
>   -:  ---------- > 3:  e0be8cdf63 commit: refuse partial commits during conflict resolution
>