From: Elijah Newren via GitGitGadget Date: Fri, 28 Aug 2026 07:44:40 GMT Subject: [PATCH v3 0/5] commit: refuse to amend during conflict resolution Message-ID: In-Reply-To: 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 ) 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-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2389/newren/refuse-amend-during-conflicts-v3 Pull-Request: https://github.com/git/git/pull/2389 Range-diff vs v2: -: ---------- > 1: 7e198a20fa commit: clarify FROM_REBASE_PICK and is_from_rebase() names -: ---------- > 2: e169303619 commit: allow a partial commit when a rebase pick becomes empty 1: 65c48ed3cb ! 3: 0850a999da commit: reword the empty-commit rebase errors @@ Metadata Author: Elijah Newren ## Commit message ## - commit: reword the empty-commit rebase errors + commit: reword the empty-commit rebase amend error When a rebase applies a commit that becomes empty, it stops and asks the user to decide whether to keep it or drop it. HEAD still points at the - previously-applied commit at that point, so either amending or creating - a partial commit is refused, with one of the following messages: + previously-applied commit at that point, so amending is refused, with: You are in the middle of a rebase -- cannot amend. - cannot do a partial commit during a rebase. - Neither message hints that the real problem is a commit that became - empty, and "during a rebase" is overly broad besides -- amending and - partial commits are fine at an `edit` or `break` stop. Reword both to - describe the actual situation. + That message would suggest that amending is not allowed during an 'edit' + or 'break' stop, which is misleading, plus it lacks the specificity that + might help the user know why their particular case is a problem: the + commit they intended to amend became empty and was dropped, so amending + would affect the wrong commit. Reword the error accordingly. Signed-off-by: Elijah Newren ## builtin/commit.c ## -@@ builtin/commit.c: static const char *prepare_index(const char **argv, const char *prefix, - else if (is_from_cherry_pick(whence)) - die(_("cannot do a partial commit during a cherry-pick.")); - else if (is_from_rebase(whence)) -- die(_("cannot do a partial commit during a rebase.")); -+ die(_("cannot do a partial commit while resolving a commit that became empty.")); - } - - if (list_paths(&partial, !current_head ? NULL : "HEAD", &pathspec)) @@ 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 (whence == FROM_REBASE_PICK) + else if (is_from_rebase_empty(whence)) - die(_("You are in the middle of a rebase -- cannot amend.")); -+ die(_("You are resolving a commit that became empty -- cannot amend.")); ++ die(_("The now-empty commit has been dropped -- cannot amend.")); } if (fixup_message && squash_message) die(_("options '%s' and '%s' cannot be used together"), "--squash", "--fixup"); ## t/t3404-rebase-interactive.sh ## -@@ t/t3404-rebase-interactive.sh: test_expect_success 'correct error message for partial commit after empty pick' - ) && - echo x >file1 && - test_must_fail git commit file1 2>err && -- test_grep "cannot do a partial commit during a rebase." err -+ test_grep "cannot do a partial commit while resolving a commit that became empty." err - ' - - test_expect_success 'correct error message for commit --amend after empty pick' ' @@ t/t3404-rebase-interactive.sh: test_expect_success 'correct error message for commit --amend after empty pick' ) && echo x>file1 && test_must_fail git commit -a --amend 2>err && - test_grep "middle of a rebase -- cannot amend." err -+ test_grep "resolving a commit that became empty -- cannot amend." err ++ test_grep "now-empty commit has been dropped -- cannot amend." err ' test_expect_success 'todo has correct onto hash' ' 2: 4a1461e527 ! 4: 9f80d8a00d 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 (whence == FROM_REBASE_PICK) +- else if (is_from_rebase_empty(whence)) + case ONGOING_REBASE_EMPTY: - die(_("You are resolving a commit that became empty -- cannot amend.")); + die(_("The now-empty commit has been dropped -- cannot amend.")); + case ONGOING_REVERT: + die(_("You are in the middle of a revert -- cannot amend.")); + case ONGOING_AM: @@ builtin/commit.c: static int parse_and_validate_options(int argc, const char *ar die(_("options '%s' and '%s' cannot be used together"), "--squash", "--fixup"); ## sequencer.c ## +@@ sequencer.c: static GIT_PATH_FUNC(rebase_path_author_script, "rebase-merge/author-script") + * command is processed, this file is deleted. + */ + static GIT_PATH_FUNC(rebase_path_amend, "rebase-merge/amend") ++/* ++ * The apply ("am") backend keeps its state in the rebase-apply directory; ++ * the "applying" file within it marks a plain `git am` (as opposed to an ++ * apply-based rebase). ++ */ ++static GIT_PATH_FUNC(apply_dir, "rebase-apply") ++static GIT_PATH_FUNC(apply_path_applying, "rebase-apply/applying") + /* + * When we stop at a given patch via the "edit" command, this file contains + * the commit object name of the corresponding patch. @@ sequencer.c: int sequencer_determine_whence(struct repository *r, enum commit_whence *whence) return 0; } @@ sequencer.c: int sequencer_determine_whence(struct repository *r, enum commit_wh +enum ongoing_operation sequencer_ongoing_operation(struct repository *r, + enum commit_whence whence) +{ -+ char *path; -+ int found; -+ + /* + * The merge, cherry-pick, and (empty) rebase-pick stops are already + * distinguished by 'whence'. @@ 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_PICK: ++ case FROM_REBASE_EMPTY: + return ONGOING_REBASE_EMPTY; + case FROM_COMMIT: + break; @@ sequencer.c: int sequencer_determine_whence(struct repository *r, enum commit_wh + return ONGOING_REVERT; + + /* In the middle of an `am`? */ -+ path = repo_git_path(r, "rebase-apply/applying"); -+ found = file_exists(path); -+ free(path); -+ if (found) ++ if (file_exists(apply_path_applying())) + return ONGOING_AM; + + /* @@ sequencer.c: int sequencer_determine_whence(struct repository *r, enum commit_wh + * 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"); -+ -+ found = file_exists(stopped_sha) && !file_exists(amend_marker); -+ free(stopped_sha); -+ free(amend_marker); -+ } -+ if (found) ++ if (file_exists(apply_dir()) || ++ (file_exists(rebase_path_stopped_sha()) && ++ !file_exists(rebase_path_amend()))) + return ONGOING_REBASE_CONFLICT; + + return ONGOING_NONE; @@ sequencer.h: int sequencer_get_last_command(struct repository* r, ## t/t3404-rebase-interactive.sh ## @@ t/t3404-rebase-interactive.sh: test_expect_success 'correct error message for commit --amend after empty pick' - test_grep "resolving a commit that became empty -- cannot amend." err + test_grep "now-empty commit has been dropped -- cannot amend." err ' +test_expect_success 'commit --amend is refused at a rebase conflict stop' ' 3: e0be8cdf63 ! 5: 050b9e8a52 commit: refuse partial commits during conflict resolution @@ Commit message Similar to the previous commit, just as `git commit --amend` is a foot-gun during conflict resolution, so is a partial commit (`git commit `). Recording a conflict resolution is about capturing the state - of the entire tree on top of HEAD, not a subset of paths. For many - years we have rejected partial commits in the middle of + of the entire tree on top of HEAD, not a subset of paths. For many years + we have rejected partial commits in the middle of - a merge - a cherry-pick - - a rebase that stopped at a pick - but, just like amending, this was never extended to the other - operations that can also leave conflicts to resolve: + but, just like amending, this was never extended to the other operations + that can also leave conflicts to resolve: - an `am` operation - a revert - a rebase that stopped for conflict resolution Reuse sequencer_ongoing_operation(), introduced for the analogous - `--amend` check, to detect all of these and refuse the partial commit. + `--amend` check, to detect these and refuse the partial commit. A rebase + that stopped because a pick became empty is not conflict resolution and, + as an earlier patch established, is deliberately left permitted. Signed-off-by: Elijah Newren @@ 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(whence)) -- die(_("cannot do a partial commit while resolving a commit that became empty.")); + switch (sequencer_ongoing_operation(the_repository, whence)) { + case ONGOING_NONE: + break; @@ builtin/commit.c: static const char *prepare_index(const char **argv, const char + case ONGOING_CHERRY_PICK: + die(_("cannot do a partial commit during a cherry-pick.")); + case ONGOING_REBASE_EMPTY: -+ die(_("cannot do a partial commit while resolving a commit that became empty.")); ++ /* ++ * A pick that became empty is not a conflict, and creating ++ * a new commit (partial or not) poses no problem. ++ */ ++ break; + case ONGOING_REVERT: + die(_("cannot do a partial commit during a revert.")); + case ONGOING_AM: -- gitgitgadget