Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
144 changes: 144 additions & 0 deletions crates/flare-git-core/src/classify.rs
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,72 @@ pub fn is_destructive(subcommand: &str, args: &[String]) -> bool {
}
}

/// Commits by which `target` and `HEAD` have diverged, when a `reset
/// --soft`/`--mixed` onto `target` would stage more than the caller's
/// intended change — i.e. neither ref is an ancestor of the other.
/// `--soft`/`--mixed` leave the working tree untouched and just move HEAD,
/// so the diff that ends up staged is `old_HEAD_tree` vs `target_tree`: when
/// `target` is a strict ancestor or descendant of HEAD (a clean fast-forward
/// either direction), that diff is exactly the commit(s) between them, which
/// is what the command is for. Once they've diverged, that same diff also
/// carries every change unique to `target`'s side — unrelated drift the
/// caller likely never intended to stage (item #98's live incident: `reset
/// --soft origin/master` from a stale branch staged a phantom crate deletion
/// that was actually a refactor on master, not a removal).
///
/// `None` if there's no divergence to warn about, or if it can't be
/// determined (unresolvable target, ...) — fails open, matching this
/// policy's bias toward never warning on something it can't actually reason
/// about.
fn reset_soft_divergence(repo_root: &Path, target: &str) -> Option<u32> {
let head_is_ancestor =
crate::shell::run_in_ok(repo_root, &["merge-base", "--is-ancestor", "HEAD", target]);
let target_is_ancestor =
crate::shell::run_in_ok(repo_root, &["merge-base", "--is-ancestor", target, "HEAD"]);
if head_is_ancestor || target_is_ancestor {
return None; // clean fast-forward in one direction or the other
}
let counts = crate::shell::run_in(
repo_root,
&[
"rev-list",
"--left-right",
"--count",
&format!("HEAD...{target}"),
],
)
.ok()?;
let mut counts = counts.split_whitespace();
let unique_to_head: u32 = counts.next()?.parse().ok()?;
let unique_to_target: u32 = counts.next()?.parse().ok()?;
Some(unique_to_head + unique_to_target)
}

/// `Some(warning)` if `subcommand`/`args` is a `reset --soft`/`--mixed`
/// targeting a ref that's diverged from HEAD (see `reset_soft_divergence`).
/// `--hard` is deliberately excluded — that's `is_destructive`'s job (a
/// working-tree-loss concern, already snapshotted before it runs), and
/// orthogonal to this one (a staged-diff surprise concern, which `--hard`
/// can't cause since it discards the index along with everything else).
#[must_use]
pub fn reset_soft_divergence_warning(
repo_root: &Path,
subcommand: &str,
args: &[String],
) -> Option<String> {
if subcommand != "reset" || !args.iter().any(|a| a == "--soft" || a == "--mixed") {
return None;
}
let target = args
.iter()
.take_while(|a| a.as_str() != "--")
.find(|a| !a.starts_with('-'))?;
let commits = reset_soft_divergence(repo_root, target)?;
Comment on lines +313 to +320

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- candidate file outline ---'
ast-grep outline crates/flare-git-core/src/classify.rs 2>/dev/null | head -200 || true
printf '%s\n' '--- relevant implementation ---'
sed -n '260,345p' crates/flare-git-core/src/classify.rs
printf '%s\n' '--- referenced tests ---'
sed -n '1370,1485p' crates/flare-git-core/src/classify.rs
printf '%s\n' '--- related symbols and call sites ---'
rg -n -C 3 'reset_soft_divergence|--soft|--mixed|subcommand.*reset|reset.*subcommand' crates/flare-git-core

Repository: getappz/agentflare

Length of output: 25163


🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- repository files related to classify tests ---'
git ls-files | rg '(^|/)(classify|git-core|Cargo\.toml|Cargo\.lock)' | head -200
printf '%s\n' '--- exact test names and reset test bodies ---'
rg -n -C 8 'reset|divergen|soft|mixed' crates/flare-git-core/src/classify.rs crates/flare-git-core/tests 2>/dev/null | head -500

Repository: getappz/agentflare

Length of output: 20883


🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- Git reset synopsis ---'
git reset -h 2>&1 | sed -n '1,120p'
printf '%s\n' '--- warning callers ---'
rg -n -C 8 'reset_soft_divergence_warning' crates src
printf '%s\n' '--- classify orchestration around warning handling ---'
sed -n '680,805p' crates/flare-git-core/src/classify.rs
printf '%s\n' '--- current argument behavior for representative inputs ---'
python3 - <<'PY'
def current(args):
    if not any(a in ('--soft', '--mixed') for a in args):
        return None
    before = []
    for a in args:
        if a == '--':
            break
        before.append(a)
    return next((a for a in before if not a.startswith('-')), None)

cases = [
    ['origin/master'],
    ['origin/master', '--', 'path'],
    ['origin/master', '--', '--soft'],
    ['--soft', 'origin/master', '--', 'path'],
    ['--', '--soft'],
    ['--patch', 'origin/master'],
]
for args in cases:
    print(args, '=>', current(args))
PY

Repository: getappz/agentflare

Length of output: 17311


🌐 Web query:

git reset documentation default mixed mode pathspec -- boundary patch form

💡 Result:

The git reset command documentation defines multiple invocation forms, each handling different scopes such as entire branches, specific paths, or interactive changes [1][2]. Default Behavior and Mixed Mode When git reset is used without path specifications—specifically in the form git reset [] []—it acts on the current branch head [1][3]. In this form, if the argument is omitted, it defaults to --mixed [1][4]. The --mixed mode updates the index to match the specified commit (defaulting to HEAD) while leaving the working directory unchanged [1][5]. Pathspec Usage When you provide path specifications (pathspec) to git reset, the command operates differently; it does not move the branch head [2][6]. Instead, it updates the staged version of the specified files or directories to match the version from the given commit or tree (which also defaults to HEAD) [7][2]. This essentially unstages changes for the specified paths and is considered the opposite of git add [7][6]. Patch Mode The git reset --patch (or -p) option allows for interactive selection of changes [7][2]. In this mode, Git presents differences between the index and the specified commit or tree (defaulting to HEAD) [6][3]. You can interactively choose hunks to "reset," which reverses the chosen changes in the index [2][3]. Like the pathspec form, this is the opposite of git add --patch and does not affect the working tree or the current branch head [7][2]. Summary of Forms 1. Branch-level reset: git reset [--soft | --mixed | --hard |...] [] (defaults to --mixed HEAD) [1][2]. 2. Path-level reset: git reset [] [--] ... (updates index for specific paths) [2][6]. 3. Interactive reset: git reset --patch [] [--] [...] (selectively unstage changes) [2][3].

Citations:


Handle default mixed resets before the -- boundary.

git reset <target> defaults to --mixed and moves HEAD, but lines 313-320 require an explicit mode. The mode check also scans arguments after --, so git reset <target> -- --soft can produce a warning even though this is a pathspec form. Parse only options before --, treat no explicit mode as --mixed, and exclude pathspec and --patch forms. Update tests at lines 1420-1451 for the implicit mixed case and the -- boundary.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/flare-git-core/src/classify.rs` around lines 313 - 320, Update the
reset classification logic around the subcommand mode check to parse only
arguments before the `--` boundary, treat an absent mode as the default
`--mixed`, and reject pathspec and `--patch` forms. Ensure target selection and
divergence handling use the pre-boundary arguments, and extend the related tests
covering implicit mixed resets and arguments after `--`.

Some(format!(
"'{target}' and HEAD have diverged by {commits} commit(s) — `reset --soft`/`--mixed` will stage the full content diff between them, not just your intended change. Consider `git cherry-pick` onto a fresh branch instead."
))
}

/// Pure classification core — no I/O, so it's unit-testable with fixed
/// inputs. `default_branch` is the repo's resolved default branch.
/// `trust_root_touch` and `push_targets_default_branch` are pre-resolved by
Expand Down Expand Up @@ -1351,6 +1417,84 @@ mod tests {
assert!(!is_destructive("clean", &args(&["--dry-run"])));
}

#[test]
fn reset_soft_divergence_warning_only_applies_to_soft_or_mixed_reset() {
// No I/O needed for these -- both bail out before touching the repo.
assert_eq!(
reset_soft_divergence_warning(
std::path::Path::new("."),
"reset",
&args(&["origin/master"])
),
None
);
assert_eq!(
reset_soft_divergence_warning(
std::path::Path::new("."),
"reset",
&args(&["--hard", "origin/master"])
),
None
);
assert_eq!(
reset_soft_divergence_warning(std::path::Path::new("."), "commit", &args(&["-m", "x"])),
None
);
}

#[test]
fn reset_soft_divergence_warning_none_without_explicit_target() {
assert_eq!(
reset_soft_divergence_warning(std::path::Path::new("."), "reset", &args(&["--soft"])),
None
);
}

#[test]
fn reset_soft_divergence_warning_fires_when_head_and_target_have_diverged() {
let repo = crate::shell::test_support::init_repo_with_branch("master");
crate::shell::run_in(&repo.path, &["checkout", "-b", "feature"]).unwrap();
std::fs::write(repo.path.join("feature.txt"), "feature").unwrap();
crate::shell::run_in(&repo.path, &["add", "feature.txt"]).unwrap();
crate::shell::run_in(&repo.path, &["commit", "-m", "feature commit"]).unwrap();
crate::shell::run_in(&repo.path, &["checkout", "master"]).unwrap();
std::fs::write(repo.path.join("master.txt"), "master").unwrap();
crate::shell::run_in(&repo.path, &["add", "master.txt"]).unwrap();
crate::shell::run_in(&repo.path, &["commit", "-m", "master commit"]).unwrap();

let msg = reset_soft_divergence_warning(&repo.path, "reset", &args(&["--soft", "feature"]));
let msg = msg.expect("HEAD and feature have diverged -- expected a warning");
assert!(msg.contains("diverged"), "{msg}");
assert!(msg.contains("feature"), "{msg}");

let msg =
reset_soft_divergence_warning(&repo.path, "reset", &args(&["--mixed", "feature"]));
assert!(msg.is_some());
}

#[test]
fn reset_soft_divergence_warning_silent_on_clean_fast_forward() {
let repo = crate::shell::test_support::init_repo_with_branch("master");
let base_sha = crate::shell::run_in(&repo.path, &["rev-parse", "HEAD"]).unwrap();
std::fs::write(repo.path.join("a.txt"), "a").unwrap();
crate::shell::run_in(&repo.path, &["add", "a.txt"]).unwrap();
crate::shell::run_in(&repo.path, &["commit", "-m", "second"]).unwrap();

// `base_sha` is a strict ancestor of HEAD -- a clean fast-forward
// reset, not a divergence.
assert_eq!(
reset_soft_divergence_warning(&repo.path, "reset", &args(&["--soft", &base_sha])),
None
);
// And the reverse direction: HEAD is a strict ancestor of `feature`.
crate::shell::run_in(&repo.path, &["branch", "feature"]).unwrap();
crate::shell::run_in(&repo.path, &["reset", "--hard", &base_sha]).unwrap();
assert_eq!(
reset_soft_divergence_warning(&repo.path, "reset", &args(&["--soft", "feature"])),
None
);
}

fn single_ref(refs: Option<Vec<PushRef>>) -> PushRef {
let mut refs = refs.expect("push targets must resolve");
assert_eq!(refs.len(), 1, "{refs:?}");
Expand Down
5 changes: 5 additions & 0 deletions crates/flare-git-shim/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,11 @@ fn main() {
),
}
}
if let Some(msg) =
classify::reset_soft_divergence_warning(&repo_root, &subcommand, &rest)
{
eprintln!("agentflare git shim: warning -- {msg}");
}
exec_real(&tool, filtered_path.as_ref(), &args);
}
}
Expand Down
Loading