diff --git a/docs/src/commands/README.md b/docs/src/commands/README.md index 232c83a..1c62bb3 100644 --- a/docs/src/commands/README.md +++ b/docs/src/commands/README.md @@ -60,6 +60,9 @@ git loom commit -m "wip" -- --no-verify git loom add zz -- -f ``` +`fold` takes one too, for the commit it makes itself — see +[fold](fold.md#git-options). + Before the separator loom parses strictly, so an option it doesn't define is an error rather than a guess — the message tells you to move it after the `--`. That also means a flag keeps loom's meaning on loom's side of the separator and @@ -78,4 +81,5 @@ there is nobody there to close an editor.) `show` and `diff` are always uncaptured; displaying is all they do. The other commands don't take a `--`: they either render their own output or -drive a rebase, where there is no single git command to forward to. +only rebase, where there is no single git command to forward to. A few of them +commit too — `split`, `absorb` and `reword` — and aren't covered yet. diff --git a/docs/src/commands/fold.md b/docs/src/commands/fold.md index 8a50bf8..8a013fe 100644 --- a/docs/src/commands/fold.md +++ b/docs/src/commands/fold.md @@ -28,6 +28,47 @@ When only a target is given, currently staged files are folded into the target c | `--above ` | Move the source commit(s) directly above (newer than) this commit. | | `--below ` | Move the source commit(s) directly below (older than) this commit. | +### Git Options + +The forms that commit take a `--` separator — see +[Passing Options to Git](README.md#passing-options-to-git): + +```bash +git loom fold src/auth.rs ab -- --no-verify +git loom fold -p ab -- --no-verify +``` + +They reach the amend, or the `fixup!` commit loom makes for a non-HEAD target — +the two places your commit hooks run. The rebase that rewrites the rest runs no +hooks, so a fold that moves whole commits around (fixup, move, uncommit a +commit, `--create`, `--above`/`--below`) takes no arguments after `--` and says +so. + +Loom's own arguments go last, so a boolean git resolves last-wins keeps the +value loom asked for: a forwarded `--no-amend` or `--edit` has no effect. + +Everything else reaches git as written, gaps included, because `--` is for +someone who knows what they are asking git to do: + +- a message source — `-m`, `-F`, `-c`/`-C`, `--fixup`, `--squash` — rewords + every commit loom makes, which for a HEAD target is the target itself, and on + a move between two commits is *both* of them; use [reword](reword.md) + instead; +- a pathspec restricts the commit and brings `--only` semantics with it, so git + commits the working tree's copy of that path rather than what is staged; +- `-a` and `-i` sweep in tracked changes you did not name, and a fold into an + older commit buries them there. That includes your *own staged* files: loom + moves them out of the index for the duration and leaves them in the working + tree, which is where `-a` finds them; +- a second `--` becomes a pathspec and takes loom's own arguments with it, so + git fails on them. + +One thing loom does not leave to you: `--dry-run` and the status formats make +git print without committing at all, which would leave a fold rewriting history +around a commit that was never made. Loom checks what git actually did and +takes the whole attempt back — HEAD, its own staging, and your other staged +files — rather than going on. + ## Type Dispatch The action depends on the types of the arguments, detected automatically: diff --git a/skills/git-loom/SKILL.md b/skills/git-loom/SKILL.md index 12ec775..9249a70 100644 --- a/skills/git-loom/SKILL.md +++ b/skills/git-loom/SKILL.md @@ -59,7 +59,7 @@ names, paths, and hashes also work. `status -a` includes hidden branches. |---|---| | Stage | `git loom add `; list files, never `zz`. | | Commit | `git loom commit -b -m "" `; a new branch name creates it. Use `-i` for integration. Name files, or omit them to commit exactly the staged set; never `zz`. | -| Amend/fixup | `git loom fold `; for staged changes, `git loom fold `. | +| Amend/fixup | `git loom fold `; for staged changes, `git loom fold `. Git options for the amend follow `--` (`-- --no-verify`). | | Auto-fixup | `git loom absorb`; `-n` dry-runs. | | Move commits | `git loom fold ... `; `-c` creates a new branch and rejects an existing name. `--above ` / `--below ` moves next to a commit, in any branch or the same one. | | Uncommit | `git loom fold zz`. | diff --git a/specs/007-fold.md b/specs/007-fold.md index ca76a2f..fa2147a 100644 --- a/specs/007-fold.md +++ b/specs/007-fold.md @@ -7,6 +7,7 @@ ```bash git-loom fold git-loom fold ... +git-loom fold ... -- ... git-loom fold --create ... git-loom fold ... --above git-loom fold ... --below @@ -23,6 +24,7 @@ With one argument, fold the current index into that target. With two or more, th - `--hunks ` (repeated) with `--hunks-from `: supply a commit-source `-p` selection by id instead of picking it. Requires `-p`, and each flag requires the other. - `zz`: reserved `Unstaged` target/source representing the working directory/all its changes. - `commit_sid:index` (for example `fa:0`): `CommitFile` shown by `git loom status -f`. +- `-- ...`: forward untouched to the `git commit` fold runs itself — the amend, or the `fixup!` commit for a non-HEAD target (for example `--no-verify`, `-q`; Spec 021). A whole-commit form takes none. Loom's own arguments come last, so a boolean git resolves last-wins (`--no-amend`, `--edit`) has no effect; a message source (`-m`, `-F`, `-c`/`-C`, `--fixup`, `--squash`) still rewords every commit fold makes — both of them on the forms that move a file or hunks between two commits, a pathspec still restricts it with `--only` semantics, and `-a`/`-i` still sweep in changes fold was not given. Before anything is rewritten, the fixup path verifies that git left a new commit on HEAD and, with forwarded arguments, that it holds a different tree; an amend carrying forwarded arguments verifies that HEAD's tree changed. Any failure takes the commit attempt back whole. ## Resolution and dispatch @@ -64,6 +66,11 @@ Errors are verbatim; `⏎` marks a line break within a message. | `--above`/`--below` target among the sources | `Source and target are the same commit` | | Single source already directly above/below target | ``Commit `` is already directly above `` `` (or `below`) | | Several sources already in place | ``Commits are already in place above `` `` (or `below`) | +| `--` arguments on a whole-commit form | `` runs no `git commit`, so it takes no arguments after `--` `` | +| Forwarded argument left no fixup commit | ``` `git commit` left no new commit on HEAD, so nothing was folded⏎An argument after `--` stopped it from committing ``` (second line only with forwarded arguments) | +| Forwarded argument left the fixup commit empty | ``` `git commit` made an empty `fixup!` commit, so nothing was folded⏎An argument after `--` kept the staged changes out of it ``` | +| Amend with `--` arguments changed nothing | ``` `git commit --amend` left the commit as it was, so nothing was amended⏎Either an argument after `--` kept git from committing, or what was staged already matched the commit ``` | +| Amend committed on top instead (git stopped resolving `--amend` last-wins) | ``` `git commit --amend` committed on top of the target instead of amending it ``` | ## File/current-change amendments diff --git a/specs/021-git-args.md b/specs/021-git-args.md index c0b3d04..ca161db 100644 --- a/specs/021-git-args.md +++ b/specs/021-git-args.md @@ -35,11 +35,17 @@ itself uses. The cost is one extra `--`. | `diff` | `git diff` | | `commit`| `git commit` | | `add` | `git add` | +| `fold` | `git commit` | + +`fold` drives a rebase like the commands below, but it also makes a commit of +its own along the way — the amend, or the `fixup!` commit for a non-HEAD target +— and that one runs the user's commit hooks. See [Fold Commits Too](#fold-commits-too). Every other command either renders its output itself (`status`, `tui`, `trace`) -or drives a rebase through Weave (`fold`, `absorb`, `split`, `swap`, `drop`, -`reword`, `branch`, `update`, `init`), where there is no single git command the -arguments could belong to. +or drives a rebase through Weave (`absorb`, `split`, `swap`, `drop`, `reword`, +`branch`, `update`, `init`), where there is no single git command the arguments +could belong to. Some of those also make a commit of their own — `split`, +`absorb` and `reword` — and are not covered yet. ## What Happens @@ -61,7 +67,9 @@ diagnostic. A forwarded `--` reaches git as written too, but only `show` leaves it alone: `diff` appends a `--` of its own when the user named file targets, and `add` always appends one, so a second separator lands in a command line that already -has one and git rejects the result. +has one and git rejects the result. `commit` and `fold` place loom's own +arguments after the forwarded ones, so a forwarded `--` turns those into +pathspecs and git fails on them instead. They are placed **after the revisions loom resolved and before any `--` pathspec loom builds itself**, so a forwarded option is still read as an option @@ -73,6 +81,7 @@ git-loom show ab -- -- README.md # git show -- README.md git-loom diff f1 -- --stat # git diff --stat -- file1.txt git-loom commit -m x -- -S # git commit -S -m x git-loom add f1 -- -f # git add -f -- file1.txt +git-loom fold f1 ab -- -n # git commit -n --amend --no-edit --allow-empty ``` Because the tokens are never inspected, an option's value may be attached or @@ -102,6 +111,75 @@ agent on a pty — the failure `commit`'s own agent-mode guard exists to prevent uncaptured, forwarded arguments or not; capturing them would swallow the user's pager and colors along with everything else. +`fold` does not step back either, for a different reason: it reads what git +did and rewrites history on that answer (below), which an uncaptured run +cannot report. A forwarded `--dry-run` therefore prints into the trace log and +ends in an error rather than on the terminal. + +### Fold Commits Too + +The forms that make a commit take the separator: amending files, staged +changes or `zz` into a commit, moving a file's changes between commits or back +to the working tree, and every `-p` form, whose amend runs at a rebase pause. A +whole-commit form only rebases — fixup, move, uncommit, `-c`, +`--above`/`--below` — and rejects the separator: *"\ runs no `git +commit`, so it takes no arguments after `--`"*. + +Nothing is inspected here either. Loom's own arguments are placed **after** the +forwarded ones, so a boolean git resolves last-wins keeps the value loom asked +for: `--no-amend` loses to `--amend`, and `--edit` to `--no-edit`. + +That is all ordering buys. Git does not resolve a message source last-wins, so +`-m`, `-F`, `-c`/`-C`, `--fixup` and `--squash` all reword every commit fold +makes — the target itself when it is HEAD, which is `reword`'s job, and the +`fixup!` commit otherwise, where the squash throws the message away. Moving a +file or hunks *between* two commits amends both, so a message source rewords +the source commit as well as the target. A pathspec +restricts the commit and takes `--only` semantics with it, committing the +working tree's copy of that path rather than the index's. `-a`/`--all` and +`-i`/`--include` go the other way and sweep in tracked changes fold was never +given; on a non-HEAD target that buries them in a rewritten historical commit. +Fold's own unstaging feeds them: the user's other staged files are moved out of +the index and left in the working tree for the duration, which is exactly where +`-a` picks them up. +These are the price of forwarding verbatim, and the reason `--` is for someone +who knows what they are asking git to do. + +What loom does not leave to the user is a commit that never happened. +`--dry-run` and the status formats make git print and exit 0 without +committing, which would leave fold rewriting history around nothing, so both +commit paths check what git actually did before anything is rewritten (Data +Safety): + +- The fixup commit, on two counts. Unless HEAD is now a new commit whose parent + is the HEAD it was made on, nothing is squashed and the index goes back — + *"`git commit` left no new commit on HEAD, so nothing was folded"*. Without it + a forwarded `--amend` squashes the user's own HEAD commit into the target and + loses it. That parent alone does not prove the commit holds anything: `--only` + with no pathspec commits none of the index and `--allow-empty` lets the result + through, so the tree is compared as well — *"`git commit` made an empty + `fixup!` commit, so nothing was folded"*. Squashing that rewrites the target + with nothing in it and reports the fold as done. +- Every amend that carries forwarded arguments. A fold amend normally has + something to commit, so HEAD's tree has to come out different — *"`git commit + --amend` left the commit as it was, so nothing was amended"*. The message + names both causes rather than blaming the arguments: staging a change and + then putting the file back reaches the same amend with nothing in it, and + without `--` that case passes silently. The tree is read from + HEAD on both sides, because what a hook stages is the commit's business: an + index the amend should have emptied fails on a `post-commit` hook, and a tree + written from the index beforehand fails on a `pre-commit` one. Not HEAD's + hash either, which an amend that changes nothing within the same second + reproduces. Without forwarded arguments the check does not run: git cannot be + told to do anything but amend then. + +Either way the commit attempt is taken back whole — HEAD, loom's own staging, +and the user's other staged files — before the error is reported. + +**What changes:** nothing. + +**What stays the same:** everything. + ### When the Command Doesn't Take Them `--` is not defined on the other commands, so the tokens after it are rejected @@ -131,6 +209,7 @@ git-loom diff -- -a # git's --text, which loom's own -a shadows git-loom diff ab..d0 -- --name-only git-loom commit -m "wip" -- --no-verify git-loom add zz -- -f # stage an ignored file too +git-loom fold zz ab -- --no-verify # skip the pre-commit hook on the amend ``` ## Design Notes @@ -147,4 +226,5 @@ happen to collide. They go to the git command the loom command wraps, and nothing else. `loom commit -- --author=…` shapes the commit loom creates, not the rebase that -relocates it onto the feature branch. +relocates it onto the feature branch, and `loom fold -- --author=…` shapes the +amend, not the commits the rebase replays over it. diff --git a/src/core/test_helpers.rs b/src/core/test_helpers.rs index 98986c2..444b765 100644 --- a/src/core/test_helpers.rs +++ b/src/core/test_helpers.rs @@ -608,6 +608,20 @@ impl TestRepo { crate::git::run_git(self.workdir().as_path(), &["config", key, value]).unwrap(); } + /// Install an executable git hook and point `core.hooksPath` at this repo, + /// which the user's own config may have aimed elsewhere. + #[cfg(unix)] + pub fn install_hook(&self, name: &str, body: &str) { + use std::os::unix::fs::PermissionsExt; + + let hooks = self.repo.path().join("hooks"); + std::fs::create_dir_all(&hooks).unwrap(); + let hook = hooks.join(name); + std::fs::write(&hook, format!("#!/bin/sh\n{body}")).unwrap(); + std::fs::set_permissions(&hook, std::fs::Permissions::from_mode(0o755)).unwrap(); + self.set_config("core.hooksPath", hooks.to_str().unwrap()); + } + /// Get porcelain status output. pub fn status_porcelain(&self) -> String { crate::git::run_git_stdout(self.workdir().as_path(), &["status", "--porcelain"]).unwrap() diff --git a/src/fold.rs b/src/fold.rs index 684cb3a..3fcded5 100644 --- a/src/fold.rs +++ b/src/fold.rs @@ -77,6 +77,7 @@ pub fn run( anchor: Option, hunks: HunkArgs, args: Vec, + git_args: Vec, theme: &graph::Theme, ) -> Result<()> { if args.is_empty() { @@ -86,22 +87,26 @@ pub fn run( ); } + let git_opts: Vec<&str> = git_args.iter().map(String::as_str).collect(); + let repo = repo::open_repo()?; if let Some(anchor) = anchor { + no_git_args(&git_opts, "moving commits next to another")?; return run_relative(&repo, &args, anchor); } if create { + no_git_args(&git_opts, "moving commits to a new branch")?; return run_create(&repo, &args); } if patch { - return run_patch_fold(&repo, &args, &hunks, theme); + return run_patch_fold(&repo, &args, &hunks, &git_opts, theme); } if args.len() == 1 { - return run_staged(&repo, &args[0]); + return run_staged(&repo, &args[0], &git_opts); } // Last argument is the target, everything else is a source @@ -148,12 +153,14 @@ pub fn run( match classify(&resolved_sources, &resolved_target)? { FoldOp::FilesIntoCommit { files, commit } => { - fold_files_into_commit(&repo, &files, &commit, false) + fold_files_into_commit(&repo, &files, &commit, false, &git_opts) } FoldOp::CommitIntoCommit { source, target } => { + no_git_args(&git_opts, "folding a commit into another")?; fold_commit_into_commit(&repo, &source, &target) } FoldOp::CommitsToBranch { commits, branch } => { + no_git_args(&git_opts, "moving commits to a branch")?; // Order and de-duplicate first: the same commit named twice is one // commit, and it should keep the resumable single-commit path // rather than be treated as a stack because of a repeated argument. @@ -166,18 +173,30 @@ pub fn run( move_commits_and_report(workdir, &repo, &commits, &branch, None) } } - FoldOp::CommitToUnstaged { commit } => fold_commit_to_unstaged(&repo, &commit), + FoldOp::CommitToUnstaged { commit } => { + no_git_args(&git_opts, "uncommitting a commit")?; + fold_commit_to_unstaged(&repo, &commit) + } FoldOp::CommitFileToUnstaged { commit, path } => { - fold_commit_file_to_unstaged(&repo, &commit, &path) + fold_commit_file_to_unstaged(&repo, &commit, &path, &git_opts) } FoldOp::CommitFileToCommit { source_commit, path, target_commit, - } => fold_commit_file_to_commit(&repo, &source_commit, &path, &target_commit), + } => fold_commit_file_to_commit(&repo, &source_commit, &path, &target_commit, &git_opts), } } +/// A fold that only rebases runs no `git commit`, so a forwarded argument has +/// nothing to reach (Spec 021). `what` names the operation. +fn no_git_args(git_opts: &[&str], what: &str) -> Result<()> { + if git_opts.is_empty() { + return Ok(()); + } + bail!("{what} runs no `git commit`, so it takes no arguments after `--`"); +} + /// Create a new branch and move the source commit(s) into it. /// /// `args` must be `[..., ]` — one or more commits @@ -636,6 +655,7 @@ fn run_patch_fold( repo: &Repository, args: &[String], hunks: &HunkArgs, + git_opts: &[&str], theme: &graph::Theme, ) -> Result<()> { let workdir = repo::require_workdir(repo, COMMAND)?; @@ -659,6 +679,7 @@ fn run_patch_fold( workdir, &source_hash, &picker, + git_opts, theme, ); } @@ -681,6 +702,7 @@ fn run_patch_fold( &target_hash, target_arg, &picker, + git_opts, theme, ); } @@ -738,7 +760,7 @@ fn run_patch_fold( if staged.is_empty() { bail!("No hunks selected"); } - fold_files_into_commit(repo, &staged, &commit_hash, true) + fold_files_into_commit(repo, &staged, &commit_hash, true, git_opts) } /// The selected entries of this file that travel in the hunk patch. A @@ -772,6 +794,7 @@ fn apply_and_amend_path( path: &str, gitlink: bool, reverse: bool, + git_opts: &[&str], ) -> Result<()> { match (gitlink, reverse) { (true, true) => git::apply_cached_patch_reverse(workdir, patch)?, @@ -785,7 +808,7 @@ fn apply_and_amend_path( git::stage_path(workdir, path)?; } } - git::commit_amend_no_edit(workdir) + git::commit_amend_no_edit(workdir, git_opts) } /// How a whole-file pick has to be applied. @@ -853,6 +876,7 @@ fn apply_and_amend( patch: &str, whole_files: &[PickedWholeFile], reverse: bool, + git_opts: &[&str], ) -> Result<()> { if !patch.is_empty() { if reverse { @@ -884,7 +908,7 @@ fn apply_and_amend( git::stage_path(workdir, &file.path)?; } } - git::commit_amend_no_edit(workdir) + git::commit_amend_no_edit(workdir, git_opts) } fn is_whole_file(whole_files: &[PickedWholeFile], path: &str) -> bool { @@ -961,6 +985,7 @@ fn build_movable_patch( /// /// Selected hunks are removed from source and added to target via a two-phase /// edit+continue rebase. Requires source to be newer than target. +#[allow(clippy::too_many_arguments)] fn run_patch_fold_commit_to_commit( repo: &Repository, workdir: &Path, @@ -968,6 +993,7 @@ fn run_patch_fold_commit_to_commit( target_hash: &str, target_arg: &str, picker: &Picker, + git_opts: &[&str], theme: &graph::Theme, ) -> Result<()> { let source_oid = git2::Oid::from_str(source_hash)?; @@ -990,6 +1016,7 @@ fn run_patch_fold_commit_to_commit( target_hash, target_arg, &selections, + git_opts, )?; msg::success(&format!( @@ -1017,6 +1044,7 @@ fn fold_selected_hunks_to_commit( target_hash: &str, target_arg: &str, selections: &[FileEntry], + git_opts: &[&str], ) -> Result<(String, String)> { let source_oid = git2::Oid::from_str(source_hash)?; @@ -1057,7 +1085,14 @@ fn fold_selected_hunks_to_commit( return Err(e); } - if let Err(e) = apply_and_amend(workdir, selections, &selected_patch, &whole_files, true) { + if let Err(e) = apply_and_amend( + workdir, + selections, + &selected_patch, + &whole_files, + true, + git_opts, + ) { // Outside the cleanup closure: a failed abort skips it, and there is no // `LoomState` yet for `loom abort` to find the patch in. let e = git::rebase_abort_then_cleanup(workdir, e, || { @@ -1129,7 +1164,14 @@ fn fold_selected_hunks_to_commit( return Err(e); } - if let Err(e) = apply_and_amend(workdir, selections, &selected_patch, &whole_files, false) { + if let Err(e) = apply_and_amend( + workdir, + selections, + &selected_patch, + &whole_files, + false, + git_opts, + ) { return Err(git::rebase_abort_then_cleanup(workdir, e, rollback)); } @@ -1168,6 +1210,7 @@ fn run_patch_fold_commit_to_unstaged( workdir: &Path, commit_hash: &str, picker: &Picker, + git_opts: &[&str], theme: &graph::Theme, ) -> Result<()> { let selections = staging::run_commit_hunk_picker(workdir, commit_hash, &[], picker, theme)? @@ -1200,7 +1243,14 @@ fn run_patch_fold_commit_to_unstaged( let pre_amend_hash = head_oid.to_string(); // Same rollback as below: a failure part-way through leaves the hunks // reverse-applied in the working tree, and `saved_staged` unstaged. - if let Err(e) = apply_and_amend(workdir, &selections, &selected_patch, &whole_files, true) { + if let Err(e) = apply_and_amend( + workdir, + &selections, + &selected_patch, + &whole_files, + true, + git_opts, + ) { rollback_fold(workdir, &pre_amend_hash, None, &saved_worktree); return Err(e).context("Failed to remove hunks from the commit, operation rolled back"); } @@ -1230,7 +1280,14 @@ fn run_patch_fold_commit_to_unstaged( return Err(e); } - if let Err(e) = apply_and_amend(workdir, &selections, &selected_patch, &whole_files, true) { + if let Err(e) = apply_and_amend( + workdir, + &selections, + &selected_patch, + &whole_files, + true, + git_opts, + ) { // Outside the cleanup closure: a failed abort skips it, and there is // no `LoomState` yet for `loom abort` to find the patch in. let e = git::rebase_abort_then_cleanup(workdir, e, || {}); @@ -1288,7 +1345,7 @@ fn run_patch_fold_commit_to_unstaged( /// /// Single-argument form: `loom fold `. The target must resolve to a /// commit. If nothing is staged, bails with the same message as `loom commit`. -fn run_staged(repo: &Repository, target_arg: &str) -> Result<()> { +fn run_staged(repo: &Repository, target_arg: &str, git_opts: &[&str]) -> Result<()> { let resolved = repo::resolve_arg(repo, target_arg, &[TargetKind::Commit])?; let commit_hash = match resolved { Target::Commit(hash) => hash, @@ -1299,7 +1356,7 @@ fn run_staged(repo: &Repository, target_arg: &str) -> Result<()> { if staged.is_empty() { bail!("Nothing to commit"); } - fold_files_into_commit(repo, &staged, &commit_hash, true) + fold_files_into_commit(repo, &staged, &commit_hash, true, git_opts) } #[derive(Debug)] @@ -1480,6 +1537,7 @@ fn fold_files_into_commit( files: &[String], commit_hash: &str, skip_staging: bool, + git_opts: &[&str], ) -> Result<()> { let workdir = repo::require_workdir(repo, COMMAND)?; @@ -1505,6 +1563,10 @@ fn fold_files_into_commit( let file_refs: Vec<&str> = files.iter().map(|s| s.as_str()).collect(); + // What a rollback has to take back out of the index, as opposed to what the + // user staged themselves. + let staged_by_loom: &[&str] = if skip_staging { &[] } else { &file_refs }; + // Unstage pre-existing staged files outside the target list, so they do // not end up in this commit/amend. let saved_staged = staging::save_and_unstage_other_staged(repo, workdir, &file_refs)?; @@ -1515,11 +1577,11 @@ fn fold_files_into_commit( if !skip_staging { git::stage_files(workdir, &file_refs)?; } - if let Err(e) = git::commit_amend_no_edit(workdir) { - if !skip_staging { - let _ = git::unstage_files(workdir, &file_refs); - } - git::restore_staged_patch(workdir, &saved_staged); + if let Err(e) = git::commit_amend_no_edit(workdir, git_opts) { + // An amend that got as far as replacing HEAD and then failed leaves + // it on a commit the user never asked for, so this takes HEAD back + // too. + undo_commit_attempt(workdir, head_oid, staged_by_loom, &saved_staged); return Err(e); } git::restore_staged_patch(workdir, &saved_staged); @@ -1536,14 +1598,36 @@ fn fold_files_into_commit( if !skip_staging { git::stage_files(workdir, &file_refs)?; } - if let Err(e) = git::commit(workdir, &message) { - if !skip_staging { - let _ = git::unstage_files(workdir, &file_refs); - } - git::restore_staged_patch(workdir, &saved_staged); + if let Err(e) = git::commit_captured(workdir, &message, git_opts) { + undo_commit_attempt(workdir, head_oid, staged_by_loom, &saved_staged); return Err(e); } + // Data safety: a forwarded argument git takes but loom does not know + // can leave no commit behind, or amend HEAD in place. The squash below + // would then feed the user's own HEAD commit into the target and lose + // it, so check what git actually did before anything is rewritten. + if !committed_onto(workdir, head_oid) { + undo_commit_attempt(workdir, head_oid, staged_by_loom, &saved_staged); + let blame = if git_opts.is_empty() { + "" + } else { + "\nAn argument after `--` stopped it from committing" + }; + bail!("`git commit` left no new commit on HEAD, so nothing was folded{blame}"); + } + // The parent check alone accepts a child that holds nothing: `--only` + // with no pathspec commits none of the index, and `--allow-empty` lets + // the result through. The squash would then rewrite the target with + // nothing in it and report the fold as done. + if !git_opts.is_empty() && committed_the_same_tree(workdir, head_oid) { + undo_commit_attempt(workdir, head_oid, staged_by_loom, &saved_staged); + bail!( + "`git commit` made an empty `fixup!` commit, so nothing was folded\n\ + An argument after `--` kept the staged changes out of it" + ); + } + // From here the repository carries a commit the user never asked for, // and their other staged files live only in `saved_staged`. let git_dir = repo.path().to_path_buf(); @@ -1595,6 +1679,62 @@ fn fold_files_into_commit( Ok(()) } +/// Whether HEAD is now a commit made on top of `parent`, which is what a `git +/// commit` that ran leaves behind. False for a root HEAD, and for a `git +/// commit` that committed nothing or amended `parent` away. +fn committed_onto(workdir: &Path, parent: git2::Oid) -> bool { + git::rev_parse(workdir, "HEAD^").is_ok_and(|first| first == parent.to_string()) +} + +/// Whether the commit git just made holds the same tree as `parent`, which +/// [`committed_onto`] accepts because it reads the parent alone. A git that +/// cannot answer says yes, so the caller takes the commit back rather than +/// rewriting history on top of it. +fn committed_the_same_tree(workdir: &Path, parent: git2::Oid) -> bool { + match ( + git::rev_parse(workdir, "HEAD^{tree}"), + git::rev_parse(workdir, &format!("{parent}^{{tree}}")), + ) { + (Ok(now), Ok(before)) => now == before, + _ => true, + } +} + +/// Take back a `git commit` that did not do what fold asked: whatever it did to +/// HEAD goes first, then loom's own staging, then the user's staged patch. +/// `staged_by_loom` is empty when the caller did not stage anything itself. +fn undo_commit_attempt( + workdir: &Path, + head_oid: git2::Oid, + staged_by_loom: &[&str], + saved_staged: &str, +) { + // A `--amend` moved HEAD instead of adding to it; the reset puts the + // commit back and leaves what it held staged, for the two steps below. A + // hook that simply refused leaves HEAD where it was, and there is nothing + // to take back — an unreadable HEAD counts as moved, so the reset runs. + let head_moved = git::rev_parse(workdir, "HEAD").ok() != Some(head_oid.to_string()); + if head_moved && let Err(e) = git::reset_soft(workdir, &head_oid.to_string()) { + // The commit git replaced is reachable by hash only, so name it. + msg::warn(&format!( + "could not put your commit back on HEAD: {e}\n\ + `git reset --soft {head_oid}` restores it" + )); + // Restoring the index over the wrong HEAD would only make it worse. + git::save_or_warn( + workdir, + "unrestored-staged", + saved_staged, + git::Replay::Cached, + ); + return; + } + if !staged_by_loom.is_empty() { + let _ = git::unstage_files(workdir, staged_by_loom); + } + git::restore_staged_patch(workdir, saved_staged); +} + /// How far [`squash_fixup_into_commit`] got. enum FixupOutcome { /// The rebase finished and the caller can finish off. @@ -2015,7 +2155,12 @@ fn keep_submodule_removals(workdir: &Path, commit: &str) -> Vec { /// Uncommit a single file from a commit: its changes leave the commit and land /// in the working tree as unstaged modifications. -fn fold_commit_file_to_unstaged(repo: &Repository, commit_hash: &str, path: &str) -> Result<()> { +fn fold_commit_file_to_unstaged( + repo: &Repository, + commit_hash: &str, + path: &str, + git_opts: &[&str], +) -> Result<()> { let workdir = repo::require_workdir(repo, COMMAND)?; let head_oid = repo::head_oid(repo)?; @@ -2045,7 +2190,7 @@ fn fold_commit_file_to_unstaged(repo: &Repository, commit_hash: &str, path: &str let saved_head = head_oid.to_string(); // A failure part-way through leaves the file reverse-applied in the // working tree, so this rolls back like the re-apply below does. - if let Err(e) = apply_and_amend_path(workdir, &file_diff, path, gitlink, true) { + if let Err(e) = apply_and_amend_path(workdir, &file_diff, path, gitlink, true, git_opts) { rollback_fold(workdir, &saved_head, None, &saved_worktree); return Err(e).context("Failed to uncommit file, operation rolled back"); } @@ -2072,7 +2217,7 @@ fn fold_commit_file_to_unstaged(repo: &Repository, commit_hash: &str, path: &str ) .inspect_err(|e| git::restore_or_park_after_abort(workdir, &saved_worktree.staged, e))?; - if let Err(e) = apply_and_amend_path(workdir, &file_diff, path, gitlink, true) { + if let Err(e) = apply_and_amend_path(workdir, &file_diff, path, gitlink, true, git_opts) { // Outside the cleanup closure: a failed abort skips it, and there // is no `LoomState` for `loom abort` to find the patch in. let e = git::rebase_abort_then_cleanup(workdir, e, || {}); @@ -2127,6 +2272,7 @@ fn fold_commit_file_to_commit( source_hash: &str, path: &str, target_hash: &str, + git_opts: &[&str], ) -> Result<()> { let workdir = repo::require_workdir(repo, COMMAND)?; @@ -2191,7 +2337,7 @@ fn fold_commit_file_to_commit( return Err(e); } - if let Err(e) = apply_and_amend_path(workdir, &file_diff, path, gitlink, true) { + if let Err(e) = apply_and_amend_path(workdir, &file_diff, path, gitlink, true, git_opts) { // Outside the cleanup closure: a failed abort skips it, and there // is no `LoomState` for `loom abort` to find the patch in. let e = git::rebase_abort_then_cleanup(workdir, e, || { @@ -2263,7 +2409,7 @@ fn fold_commit_file_to_commit( return Err(e); } - if let Err(e) = apply_and_amend_path(workdir, &file_diff, path, gitlink, false) { + if let Err(e) = apply_and_amend_path(workdir, &file_diff, path, gitlink, false, git_opts) { return Err(git::rebase_abort_then_cleanup(workdir, e, rollback)); } @@ -2310,7 +2456,7 @@ fn fold_commit_file_to_commit( ) .inspect_err(|e| git::restore_or_park_after_abort(workdir, &saved_worktree.staged, e))?; - if let Err(e) = apply_and_amend_path(workdir, &file_diff, path, gitlink, true) { + if let Err(e) = apply_and_amend_path(workdir, &file_diff, path, gitlink, true, git_opts) { // Outside the cleanup closure: a failed abort skips it, and there // is no `LoomState` for `loom abort` to find the patch in. let e = git::rebase_abort_then_cleanup(workdir, e, || {}); @@ -2337,7 +2483,7 @@ fn fold_commit_file_to_commit( })); } - if let Err(e) = apply_and_amend_path(workdir, &file_diff, path, gitlink, false) { + if let Err(e) = apply_and_amend_path(workdir, &file_diff, path, gitlink, false, git_opts) { return Err(git::rebase_abort_then_cleanup(workdir, e, || { rollback_fold(workdir, &saved_head, Some(&saved_refs), &saved_worktree); })); diff --git a/src/fold_test.rs b/src/fold_test.rs index 541325e..7b0c0f1 100644 --- a/src/fold_test.rs +++ b/src/fold_test.rs @@ -21,6 +21,7 @@ fn fold_file_into_head() { &["file1.txt".to_string()], &head_oid.to_string(), false, + &[], ); assert!( @@ -50,6 +51,7 @@ fn fold_multiple_files_into_head() { &["file1.txt".to_string(), "new_file.txt".to_string()], &head_oid.to_string(), false, + &[], ); assert!(result.is_ok(), "fold failed: {:?}", result); @@ -71,6 +73,7 @@ fn fold_file_into_non_head_commit() { &["file1.txt".to_string()], &c1_oid.to_string(), false, + &[], ); assert!( @@ -97,6 +100,7 @@ fn fold_file_no_changes_fails() { &["file1.txt".to_string()], &head_oid.to_string(), false, + &[], ); assert!(result.is_err()); @@ -117,6 +121,7 @@ fn fold_file_into_non_head_with_other_changes_autostashed() { &["file1.txt".to_string()], &c1_oid.to_string(), false, + &[], ); assert!( @@ -153,6 +158,7 @@ fn fold_file_into_woven_branch_commit() { &["feature1".to_string()], &feat1_oid.to_string(), false, + &[], ); assert!( @@ -221,7 +227,7 @@ fn fold_patch_only_staged_hunk_is_folded_into_head() { assert_eq!(staged, vec!["file.txt"]); let result = - super::fold_files_into_commit(&test_repo.repo, &staged, &head_oid.to_string(), true); + super::fold_files_into_commit(&test_repo.repo, &staged, &head_oid.to_string(), true, &[]); assert!( result.is_ok(), "fold_files_into_commit failed: {:?}", @@ -278,7 +284,7 @@ fn fold_patch_only_staged_hunk_is_folded_into_non_head() { let staged = crate::core::repo::get_staged_files(&test_repo.repo).unwrap(); let result = - super::fold_files_into_commit(&test_repo.repo, &staged, &target_oid.to_string(), true); + super::fold_files_into_commit(&test_repo.repo, &staged, &target_oid.to_string(), true, &[]); assert!( result.is_ok(), "fold_files_into_commit failed: {:?}", @@ -464,6 +470,7 @@ fn fold_commit_to_branch_via_short_ids() { None, HunkArgs::default(), vec![commit_sid.clone(), branch_sid.clone()], + vec![], &crate::core::graph::Theme::dark(), ) }); @@ -1130,6 +1137,7 @@ fn fold_unstaged_into_commit() { None, HunkArgs::default(), vec!["zz".into(), "HEAD".into()], + vec![], &crate::core::graph::Theme::dark(), ) }); @@ -1152,6 +1160,7 @@ fn fold_unstaged_clean_tree_fails() { None, HunkArgs::default(), vec!["zz".into(), "HEAD".into()], + vec![], &crate::core::graph::Theme::dark(), ) }); @@ -1179,8 +1188,12 @@ fn fold_commit_file_to_unstaged_head() { let head_oid = test_repo.head_oid(); - let result = - super::fold_commit_file_to_unstaged(&test_repo.repo, &head_oid.to_string(), "file1.txt"); + let result = super::fold_commit_file_to_unstaged( + &test_repo.repo, + &head_oid.to_string(), + "file1.txt", + &[], + ); assert!( result.is_ok(), @@ -1212,7 +1225,7 @@ fn fold_commit_file_to_unstaged_non_head() { test_repo.commit_staged("Second commit"); let result = - super::fold_commit_file_to_unstaged(&test_repo.repo, &c1_oid.to_string(), "file1.txt"); + super::fold_commit_file_to_unstaged(&test_repo.repo, &c1_oid.to_string(), "file1.txt", &[]); assert!( result.is_ok(), @@ -1243,7 +1256,8 @@ fn fold_commit_file_to_unstaged_submodule() { test_repo.commit_staged("Bump submodule"); let head_oid = test_repo.head_oid(); - super::fold_commit_file_to_unstaged(&test_repo.repo, &head_oid.to_string(), "Data").unwrap(); + super::fold_commit_file_to_unstaged(&test_repo.repo, &head_oid.to_string(), "Data", &[]) + .unwrap(); let new_head = test_repo.head_oid(); assert_eq!(test_repo.commit_file_paths(new_head), ["other.txt"]); @@ -1267,7 +1281,8 @@ fn fold_commit_file_to_unstaged_submodule_non_head() { test_repo.stage_files(&["later.txt"]); test_repo.commit_staged("Later"); - super::fold_commit_file_to_unstaged(&test_repo.repo, &bump_oid.to_string(), "Data").unwrap(); + super::fold_commit_file_to_unstaged(&test_repo.repo, &bump_oid.to_string(), "Data", &[]) + .unwrap(); assert_eq!( test_repo.commit_file_paths(test_repo.get_oid(1)), @@ -1289,7 +1304,8 @@ fn fold_commit_file_to_unstaged_submodule_add() { test_repo.commit_staged("Add submodule"); let head_oid = test_repo.head_oid(); - super::fold_commit_file_to_unstaged(&test_repo.repo, &head_oid.to_string(), "Data").unwrap(); + super::fold_commit_file_to_unstaged(&test_repo.repo, &head_oid.to_string(), "Data", &[]) + .unwrap(); let new_head = test_repo.head_oid(); assert!(!test_repo.commit_has_file(new_head, "Data")); @@ -1365,7 +1381,8 @@ fn fold_commit_file_to_unstaged_submodule_remove_keeps_the_checkout() { test_repo.commit_staged("Remove submodule"); let head_oid = test_repo.head_oid(); - super::fold_commit_file_to_unstaged(&test_repo.repo, &head_oid.to_string(), "Data").unwrap(); + super::fold_commit_file_to_unstaged(&test_repo.repo, &head_oid.to_string(), "Data", &[]) + .unwrap(); let new_head = test_repo.head_oid(); assert_eq!(test_repo.submodule_oid(new_head, "Data"), first); @@ -1388,7 +1405,8 @@ fn fold_commit_file_to_unstaged_submodule_remove_drops_the_checkout() { test_repo.commit_staged("Remove submodule"); let head_oid = test_repo.head_oid(); - super::fold_commit_file_to_unstaged(&test_repo.repo, &head_oid.to_string(), "Data").unwrap(); + super::fold_commit_file_to_unstaged(&test_repo.repo, &head_oid.to_string(), "Data", &[]) + .unwrap(); let new_head = test_repo.head_oid(); assert_eq!(test_repo.submodule_oid(new_head, "Data"), first); @@ -1417,7 +1435,8 @@ fn fold_commit_file_to_unstaged_submodule_remove_non_head() { test_repo.stage_files(&["later.txt"]); test_repo.commit_staged("Later"); - super::fold_commit_file_to_unstaged(&test_repo.repo, &remove_oid.to_string(), "Data").unwrap(); + super::fold_commit_file_to_unstaged(&test_repo.repo, &remove_oid.to_string(), "Data", &[]) + .unwrap(); assert_eq!(test_repo.submodule_oid(test_repo.head_oid(), "Data"), first); assert_eq!( @@ -1494,7 +1513,8 @@ fn fold_commit_file_to_unstaged_submodule_non_ascii_path() { test_repo.commit_staged("Bump submodule"); let head_oid = test_repo.head_oid(); - super::fold_commit_file_to_unstaged(&test_repo.repo, &head_oid.to_string(), "Dätä").unwrap(); + super::fold_commit_file_to_unstaged(&test_repo.repo, &head_oid.to_string(), "Dätä", &[]) + .unwrap(); let new_head = test_repo.head_oid(); assert_eq!(test_repo.commit_file_paths(new_head), ["other.txt"]); @@ -1621,7 +1641,7 @@ fn fold_commit_file_to_unstaged_rollback_keeps_uncommitted_changes() { test_repo.write_file("other.txt", "uncommitted work"); let err = - super::fold_commit_file_to_unstaged(&test_repo.repo, &c1_oid.to_string(), "file1.txt") + super::fold_commit_file_to_unstaged(&test_repo.repo, &c1_oid.to_string(), "file1.txt", &[]) .expect_err("the diff must not merge back"); assert!( err.to_string().contains("rolled back"), @@ -1648,6 +1668,7 @@ fn fold_commit_file_to_unstaged_no_changes_fails() { &test_repo.repo, &head_oid.to_string(), "nonexistent.txt", + &[], ); assert!(result.is_err()); @@ -1678,6 +1699,7 @@ fn fold_commit_file_to_commit() { &source_oid.to_string(), "file1.txt", &target_oid.to_string(), + &[], ); assert!( @@ -1715,6 +1737,7 @@ fn fold_commit_file_to_commit_submodule() { &source_oid.to_string(), "Data", &target_oid.to_string(), + &[], ) .unwrap(); @@ -1750,6 +1773,7 @@ fn fold_commit_file_to_commit_submodule_source_newer() { &source_oid.to_string(), "Data", &target_oid.to_string(), + &[], ) .unwrap(); @@ -1794,6 +1818,7 @@ fn fold_commit_file_to_commit_rollback_keeps_uncommitted_changes() { &source.to_string(), "f.txt", &target.to_string(), + &[], ) .expect_err("the file's diff must not apply onto the target"); @@ -1839,6 +1864,7 @@ fn fold_commit_file_to_commit_forward_rollback_keeps_uncommitted_changes() { &source.to_string(), "f.txt", &target.to_string(), + &[], ) .expect_err("the file's diff must not apply onto the target"); @@ -1861,6 +1887,7 @@ fn fold_commit_file_to_commit_same_commit_fails() { &c1_oid.to_string(), "file1.txt", &c1_oid.to_string(), + &[], ); assert!(result.is_err()); @@ -1893,6 +1920,7 @@ fn fold_commit_file_to_older_commit() { &c2_oid.to_string(), "file_a.txt", &c1_oid.to_string(), + &[], ); assert!( @@ -1957,7 +1985,7 @@ fn fold_commit_file_to_unstaged_stacked_branch() { let fa_tip = test_repo.get_branch_target("feature-a"); let result = - super::fold_commit_file_to_unstaged(&test_repo.repo, &fa_tip.to_string(), "fa1.txt"); + super::fold_commit_file_to_unstaged(&test_repo.repo, &fa_tip.to_string(), "fa1.txt", &[]); assert!( result.is_ok(), @@ -2015,6 +2043,7 @@ fn fold_commit_file_to_commit_stacked_branch() { &b1_oid.to_string(), "fb1.txt", &a1_oid.to_string(), + &[], ); assert!( @@ -2104,6 +2133,7 @@ fn fold_commit_file_to_commit_woven_branches() { &foo3_tip.to_string(), "feature7", &foo2_tip.to_string(), + &[], ); assert!( @@ -2478,6 +2508,7 @@ fn fold_rolls_back_when_the_rebase_refuses_to_start() { &["a1.txt".to_string()], &a_oid.to_string(), false, + &[], ); assert!( @@ -2548,6 +2579,7 @@ fn fold_into_an_out_of_scope_commit_leaves_the_repo_alone() { &["l1.txt".to_string()], &out_of_scope.to_string(), false, + &[], ); assert!(result.is_err(), "an out-of-scope target must be refused"); @@ -2810,6 +2842,7 @@ fn fold_moves_several_commits_to_a_branch() { None, HunkArgs::default(), vec![m2.to_string(), m1.to_string(), "feature-a".to_string()], + vec![], &crate::core::graph::Theme::dark(), ) }); @@ -2862,7 +2895,7 @@ fn fold_staged_into_head() { let head_oid = test_repo.head_oid(); - let result = super::run_staged(&test_repo.repo, &head_oid.to_string()); + let result = super::run_staged(&test_repo.repo, &head_oid.to_string(), &[]); assert!(result.is_ok(), "run_staged failed: {:?}", result); assert_eq!(test_repo.get_message(0), "Second commit"); @@ -2877,7 +2910,7 @@ fn fold_staged_nothing_staged_fails() { let head_oid = test_repo.head_oid(); - let result = super::run_staged(&test_repo.repo, &head_oid.to_string()); + let result = super::run_staged(&test_repo.repo, &head_oid.to_string(), &[]); assert!(result.is_err()); assert!( result @@ -2900,7 +2933,7 @@ fn fold_staged_only_uses_staged_not_unstaged() { let head_oid = test_repo.head_oid(); - let result = super::run_staged(&test_repo.repo, &head_oid.to_string()); + let result = super::run_staged(&test_repo.repo, &head_oid.to_string(), &[]); assert!(result.is_ok(), "run_staged failed: {:?}", result); // Only file1.txt should be in the commit; file2.txt should remain as unstaged @@ -2919,7 +2952,7 @@ fn fold_staged_non_commit_target_fails() { test_repo.stage_files(&["file1.txt"]); // Passing a branch name when only Commit is accepted should fail - let result = test_repo.in_dir(|| super::run_staged(&test_repo.repo, "feature-a")); + let result = test_repo.in_dir(|| super::run_staged(&test_repo.repo, "feature-a", &[])); assert!(result.is_err(), "should have failed"); let err_msg = result.unwrap_err().to_string(); assert!( @@ -2962,6 +2995,7 @@ fn fold_abort_preserves_working_state() { &["shared.txt".to_string()], &a_oid.to_string(), false, + &[], ); assert!( result.is_ok(), @@ -3017,6 +3051,7 @@ fn fold_unstaged_deletion_into_head() { &["file1.txt".to_string()], &head_oid.to_string(), false, + &[], ); assert!(result.is_ok(), "fold of a deletion failed: {:?}", result); @@ -3040,6 +3075,7 @@ fn fold_staged_deletion_into_head() { &["file1.txt".to_string()], &head_oid.to_string(), false, + &[], ); assert!( @@ -3067,6 +3103,7 @@ fn fold_staged_deletion_into_non_head_commit() { &["file1.txt".to_string()], &c2_oid.to_string(), false, + &[], ); assert!( @@ -3159,7 +3196,7 @@ fn apply_and_amend_uncommits_a_picked_submodule() { }]; let gitlinks = super::picked_whole_files(&workdir, &head, &selections).unwrap(); - super::apply_and_amend(&workdir, &selections, "", &gitlinks, true).unwrap(); + super::apply_and_amend(&workdir, &selections, "", &gitlinks, true, &[]).unwrap(); let new_head = test_repo.head_oid(); assert_eq!(test_repo.commit_file_paths(new_head), ["other.txt"]); @@ -3300,7 +3337,7 @@ fn fold_file_out_of_a_commit_refuses_when_the_replay_is_dropped() { let head_before = t.head_oid(); let alpha_before = t.get_branch_target("alpha"); - let err = super::fold_commit_file_to_unstaged(&t.repo, &target.to_string(), "one.txt") + let err = super::fold_commit_file_to_unstaged(&t.repo, &target.to_string(), "one.txt", &[]) .unwrap_err() .to_string(); @@ -3316,8 +3353,14 @@ fn fold_between_commits_walks_past_a_redundant_one() { // commit that replays empty sits between them, on the continue. let (t, older, newer) = crate::core::test_helpers::repo_with_a_redundant_commit_between(); - super::fold_commit_file_to_commit(&t.repo, &older.to_string(), "moved.txt", &newer.to_string()) - .unwrap(); + super::fold_commit_file_to_commit( + &t.repo, + &older.to_string(), + "moved.txt", + &newer.to_string(), + &[], + ) + .unwrap(); assert!(!crate::git::rebase_is_in_progress(t.repo.path())); assert!(!t.commit_messages().contains(&"branch change".to_string())); @@ -3401,6 +3444,7 @@ fn fold_files_into_commit_refuses_when_the_target_replays_empty() { &["three.txt".to_string()], &redundant.to_string(), false, + &[], ) .unwrap_err() .to_string(); @@ -3497,6 +3541,7 @@ fn fold_patch_between_commits_names_the_source_that_survives_phase_two() { &target.to_string(), "Target", &selections, + &[], ) .unwrap(); @@ -3682,7 +3727,7 @@ fn a_picked_deletion_leaves_the_source_commit() { let picked = picked_whole_files(&test_repo, head, &selections); let patch = super::build_selected_patch(&selections); - super::apply_and_amend(&workdir, &selections, &patch, &picked, true).unwrap(); + super::apply_and_amend(&workdir, &selections, &patch, &picked, true, &[]).unwrap(); let amended = crate::git::diff_commit_name_status(&workdir, "HEAD").unwrap(); assert!(amended.is_empty(), "{amended:?}"); @@ -3712,7 +3757,7 @@ fn a_picked_deletion_of_an_ignored_file_still_moves() { let picked = picked_whole_files(&test_repo, head, &selections); let patch = super::build_selected_patch(&selections); - super::apply_and_amend(&workdir, &selections, &patch, &picked, true).unwrap(); + super::apply_and_amend(&workdir, &selections, &patch, &picked, true, &[]).unwrap(); assert_eq!(test_repo.read_file("gone.txt"), "Add gone"); let amended = crate::git::diff_commit_name_status(&workdir, "HEAD").unwrap(); @@ -3742,7 +3787,7 @@ fn a_picked_deletion_of_a_binary_file_still_moves() { assert!(matches!(picked[0].kind, super::WholeFileKind::Deletion)); let patch = super::build_selected_patch(&selections); - super::apply_and_amend(&workdir, &selections, &patch, &picked, true).unwrap(); + super::apply_and_amend(&workdir, &selections, &patch, &picked, true, &[]).unwrap(); assert_eq!(test_repo.read_file("blob.bin"), "\u{0}\u{1}old\u{0}"); let amended = crate::git::diff_commit_name_status(&workdir, "HEAD").unwrap(); @@ -3770,7 +3815,7 @@ fn a_picked_deletion_of_an_empty_file_still_moves() { let picked = picked_whole_files(&test_repo, head, &selections); let patch = super::build_selected_patch(&selections); - super::apply_and_amend(&workdir, &selections, &patch, &picked, true).unwrap(); + super::apply_and_amend(&workdir, &selections, &patch, &picked, true, &[]).unwrap(); assert_eq!(test_repo.read_file("empty.txt"), ""); let amended = crate::git::diff_commit_name_status(&workdir, "HEAD").unwrap(); @@ -3792,7 +3837,7 @@ fn a_picked_deletion_enters_the_target_commit() { // Stand where the rebase pauses on the target: one commit below the source. test_repo.reset_hard(test_repo.get_oid(1)); - super::apply_and_amend(&workdir, &selections, &patch, &picked, false).unwrap(); + super::apply_and_amend(&workdir, &selections, &patch, &picked, false, &[]).unwrap(); let amended = crate::git::diff_commit_name_status(&workdir, "HEAD").unwrap(); assert!( @@ -3813,7 +3858,7 @@ fn a_picked_deletion_uncommits_as_an_unstaged_deletion() { let picked = picked_whole_files(&test_repo, head, &selections); let patch = super::build_selected_patch(&selections); - super::apply_and_amend(&workdir, &selections, &patch, &picked, true).unwrap(); + super::apply_and_amend(&workdir, &selections, &patch, &picked, true, &[]).unwrap(); super::restore_to_worktree(&workdir, &patch, &picked).unwrap(); assert!(!workdir.join("gone.txt").exists()); @@ -3909,7 +3954,7 @@ fn a_picked_deletion_with_a_glob_in_its_name_moves_alone() { let patch = super::build_selected_patch(&selections); assert!(patch.is_empty(), "only the deletion was picked: {patch}"); - super::apply_and_amend(&workdir, &selections, &patch, &picked, true).unwrap(); + super::apply_and_amend(&workdir, &selections, &patch, &picked, true, &[]).unwrap(); // The deletion left the commit; the change nobody picked stayed in it. let amended = crate::git::diff_commit_name_status(&workdir, "HEAD").unwrap(); @@ -3952,7 +3997,7 @@ fn a_picked_submodule_removal_stays_a_gitlink() { super::WholeFileKind::Gitlink { removed: true } )); - super::apply_and_amend(&workdir, &selections, "", &picked, true).unwrap(); + super::apply_and_amend(&workdir, &selections, "", &picked, true, &[]).unwrap(); assert_eq!(test_repo.submodule_oid(test_repo.head_oid(), "Data"), first); assert!(workdir.join("Data").exists(), "the checkout stays on disk"); @@ -4170,7 +4215,7 @@ fn fold_commit_file_to_unstaged_keeps_staging_on_success() { t.commit("Third", "third.txt"); let before = stage_a_mix(&t, "first.txt"); - super::fold_commit_file_to_unstaged(&t.repo, &c2.to_string(), "a.txt").unwrap(); + super::fold_commit_file_to_unstaged(&t.repo, &c2.to_string(), "a.txt", &[]).unwrap(); // `a.txt` leaves the commit and lands untracked; the staged set is what // has to be unchanged. @@ -4188,7 +4233,392 @@ fn fold_commit_file_to_commit_keeps_staging_on_success() { t.commit("Third", "third.txt"); let before = stage_a_mix(&t, "first.txt"); - super::fold_commit_file_to_commit(&t.repo, &c2.to_string(), "a.txt", &c1.to_string()).unwrap(); + super::fold_commit_file_to_commit(&t.repo, &c2.to_string(), "a.txt", &c1.to_string(), &[]) + .unwrap(); assert_eq!(t.status_porcelain(), before); } + +// ── Forwarded git arguments (Spec 021) ────────────────────────────────── + +/// The fixup path squashes whatever sits on HEAD, so it has to know that git +/// put a commit there; `--amend` would have replaced the user's own instead. +#[test] +fn committed_onto_sees_what_git_did() { + let t = TestRepo::new(); + let base = t.commit("Base", "base.txt"); + assert!(!super::committed_onto(&t.workdir(), base)); + + let child = t.commit("Child", "child.txt"); + assert!(super::committed_onto(&t.workdir(), base)); + assert!(!super::committed_onto(&t.workdir(), child)); +} + +/// The fixup path's last resort, reachable only through an argument loom does +/// not know: git amended HEAD instead of committing on top of it, and the +/// squash would have taken the user's own commit into the target. +#[test] +fn an_amend_that_replaced_head_is_taken_back() { + let t = TestRepo::new(); + t.commit("First", "file1.txt"); + t.commit("Second", "other.txt"); + t.write_file("other.txt", "staged by the user"); + t.stage_files(&["other.txt"]); + t.write_file("file1.txt", "folded"); + + let workdir = t.workdir(); + let saved = + crate::core::staging::save_and_unstage_other_staged(&t.repo, &workdir, &["file1.txt"]) + .unwrap(); + crate::git::stage_files(&workdir, &["file1.txt"]).unwrap(); + let head = t.head_oid(); + crate::git::run_git(&workdir, &["commit", "--amend", "--no-edit"]).unwrap(); + assert_ne!(t.head_oid(), head, "the amend should have moved HEAD"); + + super::undo_commit_attempt(&workdir, head, &["file1.txt"], &saved); + + assert_eq!(t.head_oid(), head); + assert_eq!(t.get_message(0), "Second"); + assert_eq!(t.read_file("file1.txt"), "folded"); + let status = t.status_porcelain(); + assert!(status.contains(" M file1.txt"), "{status}"); + assert!(status.contains("M other.txt"), "{status}"); +} + +/// The `-p` amend happens at a rebase pause, where the forwarded arguments have +/// to arrive too. +#[cfg(unix)] +#[test] +fn a_patch_fold_forwards_to_the_amend_at_the_rebase_pause() { + let body = "1\n2\n3\n4\n5\n6\n7\n8\n"; + let select_moved_hunks = |t: &TestRepo, source: git2::Oid| { + let mut selections = + crate::core::staging::collect_commit_hunks(&t.workdir(), &source.to_string(), &[]) + .unwrap(); + for file in &mut selections { + for hunk in &mut file.hunks { + hunk.selected = true; + } + } + selections + }; + let build = || { + let t = TestRepo::new_with_remote(); + t.commit_multi(&[("f.txt", body)], "Base"); + let target = t.commit_multi(&[("t.txt", "target\n")], "Target"); + let source = t.commit_multi(&[("f.txt", &body.replace("2\n", "TWO\n"))], "Source"); + t.install_hook("pre-commit", "exit 1\n"); + (t, target, source) + }; + + let (t, target, source) = build(); + let selections = select_moved_hunks(&t, source); + assert!( + super::fold_selected_hunks_to_commit( + &t.repo, + &t.workdir(), + &source.to_string(), + &target.to_string(), + "Target", + &selections, + &[], + ) + .is_err(), + "the hook should block the amend" + ); + + let (t, target, source) = build(); + let selections = select_moved_hunks(&t, source); + let (_, new_target) = super::fold_selected_hunks_to_commit( + &t.repo, + &t.workdir(), + &source.to_string(), + &target.to_string(), + "Target", + &selections, + &["--no-verify"], + ) + .unwrap(); + + assert!(t.commit_has_file(git2::Oid::from_str(&new_target).unwrap(), "f.txt")); + assert_eq!(t.commit_messages()[..3], ["Source", "Target", "Base"]); +} + +/// Moving a file between commits amends at a rebase pause, which is a third +/// place the hooks run and the forwarded arguments have to reach. +#[cfg(unix)] +#[test] +fn a_commit_file_fold_forwards_to_its_amend() { + let build = || { + let t = TestRepo::new_with_remote(); + t.write_file("file1.txt", "content1"); + t.write_file("file2.txt", "content2"); + t.stage_files(&["file1.txt", "file2.txt"]); + t.commit_staged("Source commit"); + let source = t.head_oid(); + t.write_file("file3.txt", "content3"); + t.stage_files(&["file3.txt"]); + t.commit_staged("Target commit"); + let target = t.head_oid(); + t.install_hook("pre-commit", "exit 1\n"); + (t, source, target) + }; + + let (t, source, target) = build(); + assert!( + super::fold_commit_file_to_commit( + &t.repo, + &source.to_string(), + "file1.txt", + &target.to_string(), + &[], + ) + .is_err(), + "the hook should block the amend" + ); + + let (t, source, target) = build(); + super::fold_commit_file_to_commit( + &t.repo, + &source.to_string(), + "file1.txt", + &target.to_string(), + &["--no-verify"], + ) + .unwrap(); + + assert_eq!(t.commit_messages()[..2], ["Target commit", "Source commit"]); + assert!(t.commit_has_file(t.head_oid(), "file1.txt")); + assert_eq!(t.read_file("file1.txt"), "content1"); +} + +/// A forwarded argument must not cost the commits above the target: the fixup +/// path squashes, and a squash that starts from the wrong commit eats one. +#[cfg(unix)] +#[test] +fn a_forwarded_fold_into_an_older_commit_keeps_the_commits_above_it() { + let t = TestRepo::new_with_remote(); + t.commit("Base", "base.txt"); + let first = t.commit("First", "f1.txt"); + t.commit("Second", "f2.txt"); + t.install_hook("pre-commit", "exit 1\n"); + t.write_file("f1.txt", "folded"); + + super::fold_files_into_commit( + &t.repo, + &["f1.txt".to_string()], + &first.to_string(), + false, + &["--no-verify"], + ) + .unwrap(); + + assert_eq!(t.commit_messages()[..3], ["Second", "First", "Base"]); + assert_eq!(t.read_file("f1.txt"), "folded"); +} + +/// `--amend` on the fixup path makes git rewrite the user's own HEAD instead +/// of committing the fixup; the squash would then have eaten that commit. +#[test] +fn a_forwarded_amend_on_the_fixup_path_rolls_back() { + let t = TestRepo::new_with_remote(); + t.commit("Base", "base.txt"); + let first = t.commit("First", "f1.txt"); + t.commit("Second", "f2.txt"); + let before = stage_a_mix(&t, "base.txt"); + t.write_file("f1.txt", "folded"); + + let err = super::fold_files_into_commit( + &t.repo, + &["f1.txt".to_string()], + &first.to_string(), + false, + &["--amend"], + ) + .unwrap_err(); + + assert!( + err.to_string().contains("left no new commit on HEAD"), + "{err}" + ); + assert_eq!(t.commit_messages()[..3], ["Second", "First", "Base"]); + assert_eq!(t.read_file("f1.txt"), "folded"); + let status = t.status_porcelain(); + assert!(status.contains(" M f1.txt"), "{status}"); + assert_eq!( + status.lines().filter(|l| !l.contains("f1.txt")).count(), + before.lines().count(), + "the user's own staged set is back: {status}" + ); +} + +/// A dry run prints and exits 0 without committing, so the amend has to catch +/// it before the fold reports success. +#[test] +fn a_forwarded_dry_run_on_the_amend_rolls_back() { + let t = TestRepo::new_with_remote(); + t.commit("Base", "base.txt"); + let head = t.commit("Only", "f1.txt"); + let before = stage_a_mix(&t, "base.txt"); + t.write_file("f1.txt", "folded"); + + let err = super::fold_files_into_commit( + &t.repo, + &["f1.txt".to_string()], + &head.to_string(), + false, + &["--dry-run"], + ) + .unwrap_err(); + + assert!(err.to_string().contains("nothing was amended"), "{err}"); + assert_eq!(t.head_oid(), head); + assert_eq!(t.read_file("f1.txt"), "folded"); + assert_eq!(t.status_porcelain(), format!("{before} M f1.txt\n")); +} + +/// The fixup path's `git commit` can fail outright — a refusing `pre-commit` +/// hook — and must hand the index back exactly as it found it. +#[cfg(unix)] +#[test] +fn a_refused_fixup_commit_gives_the_index_back() { + let t = TestRepo::new_with_remote(); + t.commit("Base", "base.txt"); + let first = t.commit("First", "f1.txt"); + t.commit("Second", "f2.txt"); + let before = stage_a_mix(&t, "base.txt"); + let head = t.head_oid(); + t.write_file("f1.txt", "folded"); + t.install_hook("pre-commit", "exit 1\n"); + + assert!( + super::fold_files_into_commit( + &t.repo, + &["f1.txt".to_string()], + &first.to_string(), + false, + &[], + ) + .is_err() + ); + + assert_eq!(t.head_oid(), head); + assert_eq!(t.commit_messages()[..3], ["Second", "First", "Base"]); + assert_eq!(t.status_porcelain(), format!("{before} M f1.txt\n")); +} + +/// Every whole-commit form rejects the separator, and each reaches the check +/// from a different arm of `run` (Spec 021). +#[test] +fn every_whole_commit_form_rejects_the_separator() { + let t = TestRepo::new_with_remote(); + t.commit("First", "f1.txt"); + let first = t.head_oid().to_string(); + t.commit("Second", "f2.txt"); + let second = t.head_oid().to_string(); + t.create_branch_at("other", &first); + + let cases: [(bool, Option, Vec, &str); 5] = [ + ( + false, + Some(super::Anchor::Above(first.clone())), + vec![second.clone()], + "moving commits next to another", + ), + ( + true, + None, + vec![second.clone(), "brand-new".into()], + "moving commits to a new branch", + ), + ( + false, + None, + vec![second.clone(), first.clone()], + "folding a commit into another", + ), + ( + false, + None, + vec![second.clone(), "other".into()], + "moving commits to a branch", + ), + ( + false, + None, + vec![second.clone(), "zz".into()], + "uncommitting a commit", + ), + ]; + + for (create, anchor, args, what) in cases { + let result = t.in_dir(|| { + super::run( + create, + false, + anchor.clone(), + HunkArgs::default(), + args.clone(), + vec!["--no-verify".into()], + &crate::core::graph::Theme::dark(), + ) + }); + let err = result.expect_err(what).to_string(); + assert!(err.contains(what), "{what}: {err}"); + assert!( + err.contains("takes no arguments after `--`"), + "{what}: {err}" + ); + } + + assert_eq!(t.commit_messages()[..2], ["Second", "First"]); +} + +/// Moving a file between commits amends both, so a forwarded message source +/// rewords both. Pinned because the docs promise exactly this (Spec 021). +#[test] +fn a_forwarded_message_rewords_both_commits_of_a_move() { + let t = TestRepo::new_with_remote(); + t.commit("Base", "base.txt"); + let target = t.commit("Target", "t.txt"); + let source = t.commit_multi(&[("moved.txt", "m"), ("stays.txt", "s")], "Source"); + + super::fold_commit_file_to_commit( + &t.repo, + &source.to_string(), + "moved.txt", + &target.to_string(), + &["-m", "hijacked"], + ) + .unwrap(); + + assert_eq!(t.commit_messages()[..3], ["hijacked", "hijacked", "Base"]); +} + +/// `--only` with no pathspec commits none of the index, and `--allow-empty` +/// lets the result through, so git makes a `fixup!` child holding nothing. +/// Squashing that rewrites the target with nothing in it and reports success. +#[test] +fn an_empty_fixup_commit_is_refused_before_the_squash() { + let t = TestRepo::new_with_remote(); + t.commit("Base", "base.txt"); + let first = t.commit("First", "f1.txt"); + t.commit("Second", "f2.txt"); + let before = stage_a_mix(&t, "base.txt"); + let head = t.head_oid(); + t.write_file("f1.txt", "folded"); + + let err = super::fold_files_into_commit( + &t.repo, + &["f1.txt".to_string()], + &first.to_string(), + false, + &["--only", "--allow-empty"], + ) + .unwrap_err(); + + assert!(err.to_string().contains("empty `fixup!` commit"), "{err}"); + assert_eq!(t.head_oid(), head, "nothing was rewritten"); + assert_eq!(t.commit_messages()[..3], ["Second", "First", "Base"]); + assert_eq!(t.status_porcelain(), format!("{before} M f1.txt\n")); +} diff --git a/src/git/git_commit.rs b/src/git/git_commit.rs index 77fa2fb..afdef35 100644 --- a/src/git/git_commit.rs +++ b/src/git/git_commit.rs @@ -1,6 +1,6 @@ use std::path::Path; -use anyhow::Result; +use anyhow::{Result, bail}; /// Amend the current commit, optionally replacing its message /// (`git commit --quiet --allow-empty --amend --only [-m msg]`). `--only` keeps @@ -54,11 +54,49 @@ pub fn commit_amend_message_unverified(workdir: &Path, message: &str) -> Result< /// Amend the current commit, keeping its message and including staged changes /// (`git commit --amend --no-edit --allow-empty` — no `--only`, unlike /// [`commit_amend`]). -pub fn commit_amend_no_edit(workdir: &Path) -> Result<()> { - super::run_git( - workdir, - &["commit", "--amend", "--no-edit", "--allow-empty"], - ) +/// +/// `opts` is what followed a `--` on a `fold` command line (Spec 021), +/// forwarded verbatim and placed first, so git's last-wins parse keeps the +/// `--amend` loom asked for. Captured whatever it holds, unlike +/// [`commit_opts`]: loom reads the result below and rewrites on it, which an +/// uncaptured run cannot report. Errs when git printed instead of committing, +/// which it can do while exiting 0. +pub fn commit_amend_no_edit(workdir: &Path, opts: &[&str]) -> Result<()> { + // Taken only with forwarded arguments: without one git cannot be told to + // do anything but amend. + let before = if opts.is_empty() { + None + } else { + Some(( + super::rev_parse(workdir, "HEAD")?, + super::rev_parse(workdir, "HEAD^{tree}")?, + )) + }; + + let mut args = vec!["commit"]; + args.extend(opts); + args.extend(["--amend", "--no-edit", "--allow-empty"]); + super::run_git(workdir, &args)?; + + if let Some((head, tree)) = before { + // Every fold amend has something to commit, so HEAD's tree has to come + // out different; git printing instead of committing leaves it alone. + // Read from HEAD on both sides, so a `pre-commit` or `post-commit` hook + // that stages something of its own cannot fail an amend that happened. + if super::rev_parse(workdir, "HEAD^{tree}")? == tree { + bail!( + "`git commit --amend` left the commit as it was, so nothing was amended\n\ + Either an argument after `--` kept git from committing, or what was staged \ + already matched the commit" + ); + } + // An amend replaces HEAD. Loom's own `--amend` comes last and wins, so + // this only catches a git that stops resolving the pair that way. + if super::rev_parse(workdir, "HEAD^").is_ok_and(|parent| parent == head) { + bail!("`git commit --amend` committed on top of the target instead of amending it"); + } + } + Ok(()) } /// True when `path` is gone from both the working tree and the index because @@ -122,7 +160,23 @@ pub fn stage_path(workdir: &Path, path: &str) -> Result<()> { /// Create a commit with a message (`git commit -m `). pub fn commit(workdir: &Path, message: &str) -> Result<()> { - commit_opts(workdir, Some(message), &[]) + commit_captured(workdir, message, &[]) +} + +/// Create a commit with forwarded options, always captured — unlike +/// [`commit_opts`], which steps back once the user forwards anything. +/// +/// For a commit loom makes as one step of a longer operation (`fold`): loom +/// reads the result itself and rewrites on it, and the rebase that follows +/// needs the terminal. +/// Unlike [`commit_amend_no_edit`] this does not check that git committed — +/// what proves it differs per caller — so a caller that then rewrites history +/// must check for itself (see `fold::committed_onto`). +pub fn commit_captured(workdir: &Path, message: &str, opts: &[&str]) -> Result<()> { + let mut args = vec!["commit"]; + args.extend(opts); + args.extend(["-m", message]); + super::run_git(workdir, &args) } /// Create a commit, with extra `git commit` options from the user. diff --git a/src/git/git_commit_test.rs b/src/git/git_commit_test.rs index 9bb952a..0b35b83 100644 --- a/src/git/git_commit_test.rs +++ b/src/git/git_commit_test.rs @@ -90,3 +90,82 @@ fn stage_path_accepts_already_staged_deletion() { assert!(result.is_ok(), "staging failed: {:?}", result); assert_eq!(test_repo.status_porcelain().trim(), "D file1.txt"); } + +/// `--porcelain` and its siblings make `git commit` print the status and exit +/// 0 without committing; the callers rewrite history on the amend's word. +#[test] +fn commit_amend_no_edit_refuses_an_amend_that_did_not_commit() { + let test_repo = TestRepo::new(); + test_repo.commit("First commit", "file1.txt"); + test_repo.write_file("file1.txt", "amended"); + let workdir = test_repo.workdir(); + git::stage_files(workdir.as_path(), &["file1.txt"]).unwrap(); + let head = test_repo.head_oid(); + + let result = git::commit_amend_no_edit(workdir.as_path(), &["--porcelain"]); + + assert!(result.is_err(), "the dry run should not pass for an amend"); + assert_eq!(test_repo.head_oid(), head); + assert_eq!(test_repo.status_porcelain().trim(), "M file1.txt"); +} + +/// Loom's own arguments come last, so git's last-wins parse keeps the amend +/// whatever the user forwards (Spec 021). +#[test] +fn commit_amend_no_edit_outranks_a_forwarded_no_amend() { + let test_repo = TestRepo::new(); + test_repo.commit("First commit", "file1.txt"); + test_repo.write_file("file1.txt", "amended"); + let workdir = test_repo.workdir(); + git::stage_files(workdir.as_path(), &["file1.txt"]).unwrap(); + let head = test_repo.head_oid(); + let parent = git::rev_parse(workdir.as_path(), "HEAD^").unwrap(); + + git::commit_amend_no_edit(workdir.as_path(), &["--no-amend"]).unwrap(); + + assert_ne!(test_repo.head_oid(), head, "the amend replaced HEAD"); + assert_eq!( + git::rev_parse(workdir.as_path(), "HEAD^").unwrap(), + parent, + "nothing was committed on top" + ); + assert_eq!(test_repo.get_message(0), "First commit"); +} + +/// A hook that stages something of its own must not make a successful amend +/// look like a dry run — on either side of the commit. A `pre-commit` hook +/// changes what is committed, and `post-commit` runs whatever `--no-verify` +/// says. +#[cfg(unix)] +#[test] +fn commit_amend_no_edit_survives_a_hook_that_stages() { + for (when, arg) in [("pre-commit", "-q"), ("post-commit", "--no-verify")] { + let test_repo = TestRepo::new(); + test_repo.commit("First commit", "file1.txt"); + let workdir = test_repo.workdir(); + test_repo.install_hook(when, "echo hooked > hooked.txt\ngit add hooked.txt\n"); + test_repo.write_file("file1.txt", "amended"); + git::stage_files(workdir.as_path(), &["file1.txt"]).unwrap(); + + git::commit_amend_no_edit(workdir.as_path(), &[arg]).unwrap(); + + assert_eq!(test_repo.get_message(0), "First commit", "{when}"); + assert_eq!(test_repo.read_file("file1.txt"), "amended", "{when}"); + let hooked_committed = test_repo.commit_has_file(test_repo.head_oid(), "hooked.txt"); + assert_eq!(hooked_committed, when == "pre-commit", "{when}"); + } +} + +#[test] +fn commit_amend_no_edit_takes_an_argument_that_does_commit() { + let test_repo = TestRepo::new(); + test_repo.commit("First commit", "file1.txt"); + test_repo.write_file("file1.txt", "amended"); + let workdir = test_repo.workdir(); + git::stage_files(workdir.as_path(), &["file1.txt"]).unwrap(); + + git::commit_amend_no_edit(workdir.as_path(), &["--no-verify"]).unwrap(); + + assert_eq!(test_repo.get_message(0), "First commit"); + assert_eq!(test_repo.status_porcelain().trim(), ""); +} diff --git a/src/git/mod.rs b/src/git/mod.rs index c2404bf..c4f17cb 100644 --- a/src/git/mod.rs +++ b/src/git/mod.rs @@ -17,9 +17,9 @@ pub use git_branch::{ branch_switch_create_tracking, branch_switch_detach, branch_validate_name, }; pub use git_commit::{ - commit, commit_amend, commit_amend_message_unverified, commit_amend_no_edit, commit_opts, - commit_with_editor, reset_hard, reset_mixed, reset_soft, stage_all, stage_all_opts, - stage_files, stage_files_opts, stage_path, + commit, commit_amend, commit_amend_message_unverified, commit_amend_no_edit, commit_captured, + commit_opts, commit_with_editor, reset_hard, reset_mixed, reset_soft, stage_all, + stage_all_opts, stage_files, stage_files_opts, stage_path, }; pub use git_diff::{ diff_cached, diff_cached_file, diff_cached_file_is_binary, diff_cached_files, diff_commit, diff --git a/src/main.rs b/src/main.rs index f0cba16..c8a629d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -247,6 +247,9 @@ enum Command { /// Source(s) and target: files, commits, or branches (last arg is the target, unless --above/--below names it) #[arg(required = true, num_args = 1..)] args: Vec, + /// Arguments forwarded verbatim to the `git commit` fold runs (everything after `--`) + #[arg(last = true, num_args = 0.., allow_hyphen_values = true, value_name = "GIT_ARG")] + git_args: Vec, }, /// Absorb working tree changes into the commits that introduced them Absorb { @@ -665,6 +668,7 @@ fn main() { hunks, hunks_from, args, + git_args, }) => { let anchor = above .map(fold::Anchor::Above) @@ -675,6 +679,7 @@ fn main() { anchor, HunkArgs::new(hunks, hunks_from), args, + git_args, &theme, ) } diff --git a/src/tui/app.rs b/src/tui/app.rs index 518f332..d45e6ef 100644 --- a/src/tui/app.rs +++ b/src/tui/app.rs @@ -334,7 +334,7 @@ fn execute_action( Action::Fold { sources, target } => { let mut args = sources; args.push(target); - fold::run(false, false, None, HunkArgs::default(), args, theme) + fold::run(false, false, None, HunkArgs::default(), args, vec![], theme) } Action::NewBranch { name, target } => branch::new::run(Some(name), target), Action::Drop { targets } => drop::run(targets, false), diff --git a/tests/integration/test_fold.sh b/tests/integration/test_fold.sh index 216400b..da7fed8 100644 --- a/tests/integration/test_fold.sh +++ b/tests/integration/test_fold.sh @@ -1039,6 +1039,101 @@ assert_eq "$(git -C "$WORK" rev-parse inner)" "$base_hash" "move_inner_parked" assert_eq "$(git -C "$WORK" log --format=%s -1 other)" "Inner I1" "move_inner_on_other" assert_eq "$(git -C "$WORK" log --format=%s -1 outer)" "Outer O1" "move_inner_outer_kept" +# ── GIT ARGUMENT FORWARDING ─────────────────────────────────────────────────── + +# The global config may point core.hooksPath elsewhere; aim it back at the repo. +install_failing_pre_commit() { + mkdir -p "$WORK/.git/hooks" + git -C "$WORK" config core.hooksPath "$WORK/.git/hooks" + printf '#!/bin/sh\nexit 1\n' > "$WORK/.git/hooks/pre-commit" + chmod +x "$WORK/.git/hooks/pre-commit" +} + +describe "amend: a pre-commit hook is skipped with -- --no-verify" +setup_repo_with_remote +commit_file "Hooked base" "hooked.txt" +install_failing_pre_commit +write_file "hooked.txt" "amended" +gl_capture fold hooked.txt HEAD +assert_exit_fail "$CODE" "fold_hook_blocks" +gl_capture fold hooked.txt HEAD -- --no-verify +assert_exit_ok "$CODE" "fold_no_verify_ok" +assert_eq "amended" "$(git -C "$WORK" show HEAD:hooked.txt)" "fold_no_verify_amended" + +describe "fixup: the hook on the non-HEAD path is skipped too" +setup_repo_with_remote +commit_file "H1" "h1.txt" +h1_hash=$(head_hash) +commit_file "H2" "h2.txt" +install_failing_pre_commit +write_file "h1.txt" "folded" +gl_capture fold h1.txt "$h1_hash" +assert_exit_fail "$CODE" "fold_fixup_hook_blocks" +gl_capture fold h1.txt "$h1_hash" -- --no-verify +assert_exit_ok "$CODE" "fold_fixup_no_verify_ok" +assert_eq "folded" "$(git -C "$WORK" show HEAD~1:h1.txt)" "fold_fixup_no_verify_content" +assert_log_contains "H2" "fold_fixup_no_verify_keeps_the_commit_above" + +describe "a fold that only rebases takes no arguments after --" +setup_repo_with_remote +commit_file "R1" "r1.txt" +r1_hash=$(head_hash) +commit_file "R2" "r2.txt" +r2_hash=$(head_hash) +gl_capture fold "$r2_hash" "$r1_hash" -- --no-verify +assert_exit_fail "$CODE" "fold_rebase_only_fail" +assert_contains "$OUT" 'runs no git commit' "fold_rebase_only_msg" + +describe "a forwarded --amend on the fixup path rolls back" +setup_repo_with_remote +commit_file "P1" "p1.txt" +p1_hash=$(head_hash) +commit_file "P2" "p2.txt" +write_file "p1.txt" "changed" +gl_capture fold p1.txt "$p1_hash" -- --amend +assert_exit_fail "$CODE" "fold_amend_fail" +assert_contains "$OUT" "left no new commit on HEAD" "fold_amend_msg" +assert_log_contains "P2" "fold_amend_history_intact" +assert_eq "changed" "$(cat "$WORK/p1.txt")" "fold_amend_change_kept" + +describe "a forwarded --dry-run leaves the fixup path nothing to squash" +setup_repo_with_remote +commit_file "D1" "d1.txt" +d1_hash=$(head_hash) +commit_file "D2" "d2.txt" +write_file "d1.txt" "changed" +gl_capture fold d1.txt "$d1_hash" -- --dry-run +assert_exit_fail "$CODE" "fold_fixup_dry_run_fail" +assert_contains "$OUT" "left no new commit on HEAD" "fold_fixup_dry_run_msg" +assert_log_contains "D2" "fold_fixup_dry_run_history_intact" +assert_eq "changed" "$(cat "$WORK/d1.txt")" "fold_fixup_dry_run_change_kept" + +describe "an empty fixup commit is refused before the squash" +setup_repo_with_remote +commit_file "E1" "e1.txt" +e1_hash=$(head_hash) +commit_file "E2" "e2.txt" +write_file "e1.txt" "changed" +gl_capture fold e1.txt "$e1_hash" -- --only --allow-empty +assert_exit_fail "$CODE" "fold_empty_fixup_fail" +assert_contains "$OUT" "empty" "fold_empty_fixup_msg" +assert_log_contains "E2" "fold_empty_fixup_history_intact" +assert_eq "changed" "$(cat "$WORK/e1.txt")" "fold_empty_fixup_change_kept" + +describe "a forwarded --dry-run leaves the amend to roll back" +setup_repo_with_remote +commit_file "Dry base" "dry.txt" +write_file "dry.txt" "changed" +gl_capture fold dry.txt HEAD -- --dry-run +assert_exit_fail "$CODE" "fold_dry_run_fail" +assert_contains "$OUT" "nothing was amended" "fold_dry_run_msg" + +describe "a status format is a dry run, and rolls back like one" +gl_capture fold dry.txt HEAD -- --porcelain +assert_exit_fail "$CODE" "fold_porcelain_fail" +assert_contains "$OUT" "nothing was amended" "fold_porcelain_msg" +assert_eq "changed" "$(cat "$WORK/dry.txt")" "fold_porcelain_change_kept" + # ── CONTINUE / ABORT ────────────────────────────────────────────────────────── # Shared conflict setup: C1 changes A→B, C2 changes B→C. # Uncommitting C1 (fold zz) drops it from history, forcing C2 to cherry-pick