first pass of merging in warp (doesn't build)
This commit is contained in:
+66
-66
@@ -1,76 +1,76 @@
|
||||
# APP-4218: Git operations dialogs compare against the branch's actual parent — Tech Spec
|
||||
Product spec: `specs/APP-4218/PRODUCT.md`
|
||||
## Context
|
||||
Today the Push / Publish dialog, the Create PR dialog, and their AI helpers hard-code the repo's main branch as the comparison base whenever the current branch has no upstream. On a branch-off-a-branch, every commit inherited from the parent branch shows up as "included" in the push / PR.
|
||||
All of the offending code paths already route their base through one function call: `detect_main_branch`. The fix is to introduce a `detect_parent_branch` helper that returns the closest-ancestor branch (falling back to main), and swap the four callers that currently say `detect_main_branch` to say `detect_parent_branch`. No function signatures change.
|
||||
APP-4218 is about avoiding misleading Git Operations previews when a user creates a branch from another feature branch. The original implementation fell back to the detected default branch when a branch had no upstream, so the Push / Publish dialog could show inherited parent-branch commits as if they belonged to the current branch.
|
||||
This PR implements the first, low-risk part of that behavior: the no-upstream Push / Publish commit list now falls back to a fork-point SHA instead of the default branch. The PR dialog, PR AI inputs, and `gh pr create --base` path still use the detected default branch in this checkout; they are listed as follow-ups below so the checked-in spec does not overstate what shipped.
|
||||
Relevant code:
|
||||
- `app/src/util/git.rs:316-359` — `get_unpushed_commits`: `git log @{u}..HEAD` with `main..HEAD` fallback.
|
||||
- `app/src/util/git.rs:602-634` — `get_branch_diff_entries`: `main..<end>`.
|
||||
- `app/src/util/git.rs:743-766` — `get_diff_for_pr`: `main..<end>`, feeds AI.
|
||||
- `app/src/util/git.rs:775-784` — `get_branch_commit_messages`: `main..HEAD`, feeds AI.
|
||||
- `app/src/util/git.rs:803-826` — `create_pr`: invokes `gh pr create` without `--base`.
|
||||
- `app/src/code_review/git_dialog/{push,pr}.rs` — per-dialog state, unchanged in shape. The detected parent is used transparently through the four util helpers.
|
||||
- `app/src/util/git.rs (199-249)` — `detect_fork_point`, which computes a SHA for the point where `HEAD` forked from other refs.
|
||||
- `app/src/util/git.rs (391-428)` — `get_unpushed_commits`, which uses `<upstream>..HEAD` when an upstream exists and `<fork>..HEAD` only in the no-upstream fallback.
|
||||
- `app/src/code_review/diff_state.rs (1360-1399)` — `load_metadata_for_repo`, which detects the current branch and upstream, then stores `unpushed_commits` in `DiffMetadata`.
|
||||
- `app/src/code_review/code_review_view.rs (6770-6796)` — `primary_git_action_mode`, which turns `unpushed_commits` into Publish / Push button state.
|
||||
- `app/src/util/git.rs (677-904)` — PR diff / AI / create helpers, which still compare against and target the detected default branch.
|
||||
## Proposed changes
|
||||
### 1. `detect_parent_branch`
|
||||
```rust path=null start=null
|
||||
// app/src/util/git.rs
|
||||
/// Returns the closest-ancestor branch of `HEAD`, or the main branch when
|
||||
/// no candidate qualifies. Ties prefer the detected main branch, then local
|
||||
/// over `origin/*`, then alphabetical for determinism.
|
||||
pub async fn detect_parent_branch(repo_path: &Path) -> Result<String>;
|
||||
```
|
||||
Implementation (all upfront queries run in parallel via `futures::join!`):
|
||||
1. `git for-each-ref --merged HEAD --format='%(objectname) %(refname:short)' refs/heads refs/remotes` to list ancestor refs with their commit SHAs. `--merged HEAD` filters out non-ancestors at the git level, avoiding per-candidate subprocess spawns.
|
||||
2. `git log HEAD --format=%H` to walk HEAD's history once. A `HashMap<&str, usize>` of `sha → position` gives each candidate's distance from HEAD in O(1) lookups.
|
||||
3. Resolve the actual upstream via `git rev-parse --abbrev-ref --symbolic-full-name @{u}` and exclude it (plus the current branch name) from candidates. Handles non-`origin` upstream configurations.
|
||||
4. Rank candidates by `(distance, !is_main, !is_local, name)`. Log the winner at debug.
|
||||
5. If no candidate qualified, return `detect_main_branch(repo_path)`.
|
||||
Return type is a plain `String` — either a local branch (`feature-a`) or a remote-tracking ref (`origin/feature-a`).
|
||||
### 2. Swap `detect_main_branch` for `detect_parent_branch` inside the four helpers
|
||||
No caller / signature changes. Each helper keeps its current shape; only the internal base-branch lookup changes:
|
||||
- `get_unpushed_commits`: the no-upstream fallback branch becomes `detect_parent_branch` instead of `detect_main_branch`. The primary `@{u}..HEAD` path is unchanged (when an upstream exists, it's still the most accurate "what will be pushed"). When upstream is unset, the fallback now uses the closest ancestor.
|
||||
- `get_branch_diff_entries`: `let base = detect_parent_branch(repo_path).await?;` in place of the current `detect_main_branch` call. The `{base}..{end_ref}` range logic is unchanged.
|
||||
- `get_diff_for_pr`: same one-line swap.
|
||||
- `get_branch_commit_messages`: same one-line swap.
|
||||
### 3. `create_pr` passes `--base`
|
||||
`create_pr` internally calls `detect_parent_branch`, strips any `origin/` prefix, and passes `--base <parent>` to `gh pr create`. Signature unchanged:
|
||||
```rust path=null start=null
|
||||
pub async fn create_pr(
|
||||
### 1. Add `detect_fork_point`
|
||||
`detect_fork_point(repo_path, current_branch_name)` returns the SHA where `HEAD` forked from other local or remote refs. It accepts the current branch name so the current branch and `origin/<current>` can be excluded from the comparison set; otherwise the branch would subtract itself and report no unique commits.
|
||||
The current implementation uses one reachability query plus a `rev-parse`:
|
||||
```rust
|
||||
pub async fn detect_fork_point(
|
||||
repo_path: &Path,
|
||||
title: Option<&str>,
|
||||
body: Option<&str>,
|
||||
) -> Result<PrInfo> {
|
||||
let base = detect_parent_branch(repo_path).await?;
|
||||
let base = base.strip_prefix("origin/").unwrap_or(&base).to_string();
|
||||
// ...existing gh pr create invocation, plus --base <base>...
|
||||
}
|
||||
current_branch_name: Option<&str>,
|
||||
) -> Result<Option<String>>;
|
||||
```
|
||||
If detection errors, propagate the error — the caller's existing `user_facing_git_error` path shows the generic failure toast.
|
||||
### 4. Dialogs
|
||||
No dialog-level plumbing. The four util helpers (`get_unpushed_commits`, `get_branch_diff_entries`, `get_diff_for_pr`, `get_branch_commit_messages`) already feed the Push and Create-PR dialogs; swapping them to `detect_parent_branch` internally is enough. The Commit dialog's `CommitAndCreatePr` chain inherits the fix via `create_pr` (§3).
|
||||
Surfacing the detected parent in the dialog chrome (e.g. a "Based on" row) was evaluated and dropped for now — it added visible latency waiting on detection to resolve, with limited user value. Tracked as a follow-up.
|
||||
### 5. Feature flag gating
|
||||
All changes live under `FeatureFlag::GitOperationsInCodeReview` (already gating the dialogs); no new flag.
|
||||
Algorithm:
|
||||
1. Normalize `current_branch_name`; ignore empty names and detached `HEAD`.
|
||||
2. Build `git rev-list HEAD --not --exclude=<current> --branches --exclude=origin/<current> --remotes`.
|
||||
3. Treat the last non-empty line as the oldest commit unique to `HEAD`.
|
||||
4. Return that commit's parent via `git rev-parse <oldest-unique>^`.
|
||||
5. If there are no unique commits, resolve `HEAD` itself; if the git commands fail, return `Ok(None)`.
|
||||
This differs from the earlier plan that introduced a separate `for-each-ref` step and a branch-name detector. The branch-name detector is not present in this implementation.
|
||||
### 2. Use the fork point only for no-upstream unpushed commits
|
||||
`get_unpushed_commits(repo_path, current_branch_name, upstream_ref)` keeps the upstream path unchanged:
|
||||
- If `upstream_ref` exists, run `git log <upstream>..HEAD --format=COMMIT:%H\t%s --numstat`.
|
||||
- If `upstream_ref` is missing, call `detect_fork_point(repo_path, current_branch_name)` and run `git log <fork>..HEAD ...`.
|
||||
- If no fork point can be resolved, fall back to `git log HEAD ...`.
|
||||
This is the behavior that fixes the Publish dialog for stacked no-upstream branches while preserving existing behavior for branches that already have an upstream.
|
||||
### 3. Keep dialog and metadata wiring unchanged
|
||||
`DiffStateModel::load_metadata_for_repo` already computes the current branch, upstream ref, and `unpushed_commits` during metadata refresh. The Git Operations button already derives its mode from `unpushed_commits`, `upstream_ref`, uncommitted stats, and PR info.
|
||||
No new UI state is required. The Push / Publish dialog still receives a `Vec<Commit>` from `DiffStateModel` when opened, so switching the no-upstream fallback is enough to change the included commit list.
|
||||
### 4. Leave PR helpers on the default branch for this PR
|
||||
The current code still uses `detect_main_branch` for:
|
||||
- `get_branch_diff_entries` — Create PR dialog file stats.
|
||||
- `get_diff_for_pr` — AI PR title / body diff input.
|
||||
- `get_branch_commit_messages` — AI PR title / body commit-message input.
|
||||
- `create_pr` — `gh pr create --base <default-branch>`.
|
||||
This means `PRODUCT.md` behavior around Create PR targeting the detected parent is not fully implemented by this PR. The tech spec should not claim `detect_pr_base_branch` exists or that these helpers use the fork-point SHA.
|
||||
## End-to-end flow
|
||||
1. Code review metadata refresh runs in `DiffStateModel::load_metadata_for_repo`.
|
||||
2. The model resolves `current_branch_name` and optional `upstream_ref`.
|
||||
3. `get_unpushed_commits` computes either `<upstream>..HEAD` or `<fork>..HEAD`.
|
||||
4. `CodeReviewView::primary_git_action_mode` uses non-empty `unpushed_commits` plus upstream state to choose Publish or Push.
|
||||
5. Opening the Push / Publish dialog passes those commits into `GitDialog::new_for_push`.
|
||||
## Risks and mitigations
|
||||
### Heuristic picks the wrong branch
|
||||
Two branches pointing at the same commit, deleted historical parents, etc. The parent isn't visible in the UI right now, so bad detections only manifest as a wrong commit list / wrong PR base. A follow-up can surface the parent or add a per-branch override.
|
||||
### Cost of repeated detection
|
||||
`detect_parent_branch` runs inside each of the four helpers on every dialog open (and once more in `create_pr`). Each detection is 2–4 parallel subprocess calls (`for-each-ref --merged HEAD`, `log HEAD --format=%H`, `rev-parse @{u}`, `detect_main_branch`) regardless of branch count — typically sub-100ms even in repos with thousands of remote-tracking refs. For the common PR-create flow, that's ~4× the cost on top of the AI call, which is already the dominant latency. Acceptable. If large-repo latency becomes measurable, cache per-repo inside `detect_parent_branch` itself (follow-up).
|
||||
### PR targets an unpushed base
|
||||
If the parent is a local-only branch, `gh pr create --base <b>` fails. Surfaces via the generic "Git operation failed." toast; we do not silently retry without `--base`.
|
||||
### Backwards compatibility
|
||||
Fresh-feature-off-main shapes resolve to `main`, so today's behavior is preserved on the common path.
|
||||
### Fork-point SHA is not a PR base branch name
|
||||
The fork point is enough for a commit range, but `gh pr create --base` needs a branch name. This PR intentionally does not infer a parent branch name from the fork point. Create PR behavior remains default-branch based until that follow-up lands.
|
||||
### Stale refs can affect fork-point detection
|
||||
Because `rev-list` subtracts all branches and remotes except the current branch and `origin/<current>`, stale local or remote-tracking refs can make the fork point earlier than a user expects. This is conservative for Publish commit lists but can still be surprising. Pruning stale refs remains the mitigation.
|
||||
### Remote name assumption
|
||||
The current self-exclusion only excludes `origin/<current>`. Repos whose current branch is pushed to a differently named remote could still include that remote-tracking ref in the subtraction set. That can hide unique commits after a manual push to a non-origin remote. If that matters, resolve the actual remote-tracking branch before building excludes.
|
||||
### Root commit / no other refs
|
||||
If the oldest unique commit has no parent, `rev-parse <sha>^` fails and `detect_fork_point` returns `None`; `get_unpushed_commits` then falls back to logging `HEAD`. This is acceptable for orphan or brand-new repositories.
|
||||
## Testing and validation
|
||||
References below are to `specs/APP-4218/PRODUCT.md` success criteria.
|
||||
### Manual validation
|
||||
- `feature-a` (pushed), `git checkout -b feature-b feature-a`, 1 new commit, no push: Publish dialog shows 1 commit (SC 1). Create PR shows only those files; confirming runs `gh pr create --base feature-a` and the PR targets `feature-a` (SC 2).
|
||||
- Fresh branch off main, no upstream: dialog shows `main..HEAD` (SC 3).
|
||||
- Rebase `feature-b` onto `main`, reopen dialog: commits list reflects the rebased range (SC 4).
|
||||
- Commit-and-create-PR on `feature-b`: PR targets `feature-a` (SC 6).
|
||||
- Change the pane's diff-mode dropdown; reopen dialogs: previews are unchanged (SC 7).
|
||||
### Integration / screenshot coverage
|
||||
None added. The `git_dialog` module ships without an integration harness today (see `specs/APP-4125/TECH.md`).
|
||||
Manual validation for the current implementation:
|
||||
- Create `feature-a` from main, add commits, then create `feature-b` from `feature-a` without setting an upstream. The Publish dialog on `feature-b` should show only commits unique to `feature-b`, not all of `feature-a`.
|
||||
- Create a fresh no-upstream branch from main and add commits. The Publish dialog should still show commits since the branch forked from main.
|
||||
- On a branch with an upstream, the Push dialog should still use `<upstream>..HEAD`.
|
||||
- Reopen the dialog after rebasing the current branch; metadata refresh should recompute the fallback range from the new graph.
|
||||
- Create PR dialog validation should expect current behavior for this PR: file stats, AI inputs, and `gh pr create --base` still use the detected default branch.
|
||||
Recommended unit coverage in `app/src/util/git_tests.rs`:
|
||||
- `detect_fork_point` returns the original fork commit after main advances beyond the branch point.
|
||||
- No-upstream `get_unpushed_commits` excludes commits inherited from the parent feature branch.
|
||||
- Upstream-backed `get_unpushed_commits` remains unchanged and uses `<upstream>..HEAD`.
|
||||
- Detached `HEAD` does not try to exclude a branch named `HEAD`.
|
||||
## Follow-ups
|
||||
- **Surface the detected parent in the dialog chrome** (a "Based on" row) once we have a way to populate it without visible latency — e.g. caching it on `DiffMetadata` so it's ready by the time the dialog opens.
|
||||
- **Per-branch override** for mis-detected parents (stored in `.git/config` as `branch.<name>.warpParent`).
|
||||
- **Per-repo caching** inside `detect_parent_branch` if repeated detection shows up in profiles.
|
||||
- Add parent branch-name detection for PR creation. A likely implementation is to find refs that contain or are closest to the fork point, then choose the best branch name with deterministic tiebreakers.
|
||||
- Switch `get_branch_diff_entries`, `get_diff_for_pr`, and `get_branch_commit_messages` from default-branch ranges to the detected parent range once branch-name detection exists.
|
||||
- Switch `create_pr` to pass `--base <detected-parent>` and strip `origin/` when the selected parent is a remote-tracking ref.
|
||||
- Consider resolving the actual remote-tracking branch for current-branch self-exclusion instead of assuming `origin/<current>`.
|
||||
|
||||
Reference in New Issue
Block a user