[PATCH v4 0/5] commit: refuse to amend during conflict resolution

["Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com>]

Changes since v3:

 * Just a clarifying rename, but applied in 3 places:
   * FROM_REBASE_EMPTY -> FROM_REBASE_NOW_EMPTY
   * is_from_rebase_empty() -> is_from_rebase_now_empty()
   * ONGOING_REBASE_EMPTY -> ONGOING_REBASE_NOW_EMPTY

Changes since v2:

 * Two new preparatory patches:
   * Rename FROM_REBASE_PICK and is_from_rebase() to point out they are
     about empty commits
   * Allow a partial commit when a rebase pick becomes empty
 * Tweaked the error message for attempted amend on now-dropped empty commit
   (suggestions for further improvements welcome)
 * Used the path accessor functions within sequencer.c to simplify the new
   helper function

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 (5):
  commit: clarify FROM_REBASE_PICK and is_from_rebase() names
  commit: allow a partial commit when a rebase pick becomes empty
  commit: reword the empty-commit rebase amend error
  commit: refuse to amend during conflict resolution
  commit: refuse partial commits during conflict resolution

 builtin/commit.c                |  65 +++++++++++-----
 sequencer.c                     |  59 ++++++++++++++-
 sequencer.h                     |  24 ++++++
 t/t3404-rebase-interactive.sh   | 128 +++++++++++++++++++++++++++++++-
 t/t3507-cherry-pick-conflict.sh |  22 ++++++
 t/t4151-am-abort.sh             |  22 ++++++
 wt-status.h                     |   6 +-
 7 files changed, 299 insertions(+), 27 deletions(-)


base-commit: 2c3adbb2c475981e340c79fdc5e7f4f9b5d9054e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2389%2Fnewren%2Frefuse-amend-during-conflicts-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2389/newren/refuse-amend-during-conflicts-v4
Pull-Request: https://github.com/git/git/pull/2389

Range-diff vs v3:

 1:  7e198a20fa ! 1:  bd361679b9 commit: clarify FROM_REBASE_PICK and is_from_rebase() names
     @@ Commit message
          specifically about hitting a commit that becomes empty when rebasing.
          Clarify their names now.
      
     -    While at it, change `whence == FROM_REBASE_EMPTY` to use
     -    `is_from_rebase_empty(whence)`.
     +    While at it, change `whence == FROM_REBASE_NOW_EMPTY` to use
     +    `is_from_rebase_now_empty(whence)`.
      
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
     @@ builtin/commit.c: static const char *prepare_index(const char **argv, const char
       		else if (is_from_cherry_pick(whence))
       			die(_("cannot do a partial commit during a cherry-pick."));
      -		else if (is_from_rebase(whence))
     -+		else if (is_from_rebase_empty(whence))
     ++		else if (is_from_rebase_now_empty(whence))
       			die(_("cannot do a partial commit during a rebase."));
       	}
       
     @@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const cha
       	else if (whence == FROM_MERGE)
       		hook_arg1 = "merge";
      -	else if (is_from_cherry_pick(whence) || whence == FROM_REBASE_PICK) {
     -+	else if (is_from_cherry_pick(whence) || is_from_rebase_empty(whence)) {
     ++	else if (is_from_cherry_pick(whence) || is_from_rebase_now_empty(whence)) {
       		hook_arg1 = "commit";
       		hook_arg2 = "CHERRY_PICK_HEAD";
       	}
     @@ builtin/commit.c: static int prepare_to_commit(const char *index_file, const cha
       			fputs(_(empty_amend_advice), stderr);
       		else if (is_from_cherry_pick(whence) ||
      -			 whence == FROM_REBASE_PICK) {
     -+			 is_from_rebase_empty(whence)) {
     ++			 is_from_rebase_now_empty(whence)) {
       			fputs(_(empty_cherry_pick_advice), stderr);
       			if (whence == FROM_CHERRY_PICK_SINGLE)
       				fputs(_(empty_cherry_pick_advice_single), stderr);
     @@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *ar
       		else if (is_from_cherry_pick(whence))
       			die(_("You are in the middle of a cherry-pick -- cannot amend."));
      -		else if (whence == FROM_REBASE_PICK)
     -+		else if (is_from_rebase_empty(whence))
     ++		else if (is_from_rebase_now_empty(whence))
       			die(_("You are in the middle of a rebase -- cannot amend."));
       	}
       	if (fixup_message && squash_message)
     @@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *ar
       		use_message = "HEAD";
       	if (!use_message && !is_from_cherry_pick(whence) &&
      -	    !is_from_rebase(whence) && renew_authorship)
     -+	    !is_from_rebase_empty(whence) && renew_authorship)
     ++	    !is_from_rebase_now_empty(whence) && renew_authorship)
       		die(_("--reset-author can be used only with -C, -c or --amend."));
       	if (use_message) {
       		use_message_buffer = read_commit_message(use_message);
     @@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *ar
       		}
       	}
      -	if ((is_from_cherry_pick(whence) || whence == FROM_REBASE_PICK) &&
     -+	if ((is_from_cherry_pick(whence) || is_from_rebase_empty(whence)) &&
     ++	if ((is_from_cherry_pick(whence) || is_from_rebase_now_empty(whence)) &&
       	    !renew_authorship) {
       		author_message = "CHERRY_PICK_HEAD";
       		author_message_buffer = read_commit_message(author_message);
     @@ builtin/commit.c: int cmd_commit(int argc,
       			reflog_msg = is_from_cherry_pick(whence)
       					? "commit (cherry-pick)"
      -					: is_from_rebase(whence)
     -+					: is_from_rebase_empty(whence)
     ++					: is_from_rebase_now_empty(whence)
       					? "commit (rebase)"
       					: "commit";
       		commit_list_insert(current_head, &parents);
     @@ sequencer.c: int sequencer_determine_whence(struct repository *r, enum commit_wh
       		    !repo_get_oid(r, "CHERRY_PICK_HEAD", &cherry_pick_head) &&
       		    oideq(&rebase_head, &cherry_pick_head))
      -			*whence = FROM_REBASE_PICK;
     -+			*whence = FROM_REBASE_EMPTY;
     ++			*whence = FROM_REBASE_NOW_EMPTY;
       		else
       			*whence = FROM_CHERRY_PICK_SINGLE;
       
     @@ wt-status.h: enum commit_whence {
       	FROM_CHERRY_PICK_SINGLE, /* commit came from cherry-pick */
       	FROM_CHERRY_PICK_MULTI, /* commit came from a sequence of cherry-picks */
      -	FROM_REBASE_PICK /* commit came from a pick/reword/edit */
     -+	FROM_REBASE_EMPTY /* rebase applied a pick that became empty */
     ++	FROM_REBASE_NOW_EMPTY /* rebase applied a pick that became empty */
       };
       
       static inline int is_from_cherry_pick(enum commit_whence whence)
     @@ wt-status.h: static inline int is_from_cherry_pick(enum commit_whence whence)
       }
       
      -static inline int is_from_rebase(enum commit_whence whence)
     -+static inline int is_from_rebase_empty(enum commit_whence whence)
     ++static inline int is_from_rebase_now_empty(enum commit_whence whence)
       {
      -	return whence == FROM_REBASE_PICK;
     -+	return whence == FROM_REBASE_EMPTY;
     ++	return whence == FROM_REBASE_NOW_EMPTY;
       }
       
       struct wt_status_change_data {
 2:  e169303619 ! 2:  a0b9900437 commit: allow a partial commit when a rebase pick becomes empty
     @@ builtin/commit.c: static const char *prepare_index(const char **argv, const char
       			die(_("cannot do a partial commit during a merge."));
       		else if (is_from_cherry_pick(whence))
       			die(_("cannot do a partial commit during a cherry-pick."));
     --		else if (is_from_rebase_empty(whence))
     +-		else if (is_from_rebase_now_empty(whence))
      -			die(_("cannot do a partial commit during a rebase."));
       	}
       
 3:  0850a999da ! 3:  c4511a9887 commit: reword the empty-commit rebase amend error
     @@ builtin/commit.c
      @@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *argv[],
       		else if (is_from_cherry_pick(whence))
       			die(_("You are in the middle of a cherry-pick -- cannot amend."));
     - 		else if (is_from_rebase_empty(whence))
     + 		else if (is_from_rebase_now_empty(whence))
      -			die(_("You are in the middle of a rebase -- cannot amend."));
      +			die(_("The now-empty commit has been dropped -- cannot amend."));
       	}
 4:  9f80d8a00d ! 4:  e77b34c1cf commit: refuse to amend during conflict resolution
     @@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *ar
      -		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 (is_from_rebase_empty(whence))
     -+		case ONGOING_REBASE_EMPTY:
     +-		else if (is_from_rebase_now_empty(whence))
     ++		case ONGOING_REBASE_NOW_EMPTY:
       			die(_("The now-empty commit has been dropped -- cannot amend."));
      +		case ONGOING_REVERT:
      +			die(_("You are in the middle of a revert -- cannot amend."));
     @@ sequencer.c: int sequencer_determine_whence(struct repository *r, enum commit_wh
      +	case FROM_CHERRY_PICK_SINGLE:
      +	case FROM_CHERRY_PICK_MULTI:
      +		return ONGOING_CHERRY_PICK;
     -+	case FROM_REBASE_EMPTY:
     -+		return ONGOING_REBASE_EMPTY;
     ++	case FROM_REBASE_NOW_EMPTY:
     ++		return ONGOING_REBASE_NOW_EMPTY;
      +	case FROM_COMMIT:
      +		break;
      +	}
     @@ sequencer.h: int sequencer_get_last_command(struct repository* r,
      +	ONGOING_NONE = 0,
      +	ONGOING_MERGE,
      +	ONGOING_CHERRY_PICK,
     -+	ONGOING_REBASE_EMPTY,
     ++	ONGOING_REBASE_NOW_EMPTY,
      +	ONGOING_REVERT,
      +	ONGOING_AM,
      +	ONGOING_REBASE_CONFLICT
 5:  050b9e8a52 ! 5:  b93b26ed9f commit: refuse partial commits during conflict resolution
     @@ builtin/commit.c: static const char *prepare_index(const char **argv, const char
      +		die(_("cannot do a partial commit during a merge."));
      +	case ONGOING_CHERRY_PICK:
      +		die(_("cannot do a partial commit during a cherry-pick."));
     -+	case ONGOING_REBASE_EMPTY:
     ++	case ONGOING_REBASE_NOW_EMPTY:
      +		/*
      +		 * A pick that became empty is not a conflict, and creating
      +		 * a new commit (partial or not) poses no problem.

-- 
gitgitgadget