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

> 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.
>