[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 [off-list ref]
      
       ## 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

-- 
gitgitgadget