fix: bookmark handling for PRs from other users' forks#69
Merged
Conversation
When a PR from another user's fork has a `headRefName` that collides with a local bookmark (e.g. someone pushes from their fork's "main" branch, matching the local "main" default branch), jj-pr incorrectly treated the local bookmark as the head branch for that foreign PR. The fix adds an ownership check in two places in `pr_dag::build()`: 1. The `cid_pr_tip` loop (PR tip detection) - skips PRs whose `head_repo_owner` doesn't match any configured remote owner 2. The `pr_needs_push` loop - same check to avoid flagging pushes for foreign PRs The check is: if `head_repo_owner` is set AND `remote_owners` is populated AND no remote owner matches the PR's owner, skip the PR. This preserves backward compatibility with legacy fixtures where `remote_owners` is empty. Also fixed test helpers to use `input.remote_owners` instead of hardcoded empty maps, converted the cargo-smart-release-73 fixture to `.json.gz` format with proper `remote_owners` data, and removed the old `.gz` file. Co-authored-by: Infinity 🤖 <infinity@hydro.run> PR: #69
…mote map Replaced the `foreign_prs` HashSet and the inline `find_map` reverse lookup with a single `owner_to_remote: HashMap<&Owner, &Remote>` built once at the top of `build()`. This map: 1. Detects foreign PRs via `contains_key` (both loops use this) 2. Resolves which remote corresponds to a PR's owner (replaces the `find_map` in the pr_needs_push loop) 3. Errors early if multiple remotes share the same owner (addresses #68) Added two focused unit tests: - `foreign_pr_bookmark_collision`: PR from another user's fork with headRefName "main" colliding with local "main" is completely ignored - `duplicate_remote_owner_errors`: two remotes with the same owner produces a clear error message Collapsed nested if statements per clippy. Co-authored-by: Infinity 🤖 <infinity@hydro.run> PR: #69
MingweiSamuel
force-pushed
the
not-our-branch
branch
from
d01c135 to
c5a1218
Compare
MingweiSamuel
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect handling of pull requests coming from other users’ forks when their headRefName collides with an existing local bookmark (e.g. a foreign PR from branch main colliding with the local main bookmark). It does so by using head_repo_owner plus configured remote_owners to identify and ignore “foreign” PRs during repo-state construction.
Changes:
- Add an owner-based skip for PRs whose
head_repo_ownerdoesn’t match any configured remote owner inpr_dag::build()(both PR tip detection andpr_needs_pushcomputation). - Update test helpers to pass
input.remote_ownersthrough the render/build paths and add regression tests for the collision scenario. - Update/introduce snapshot fixtures for
cargo-smart-release-73.json.gzand add new snapshots for the foreign-collision test.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pr_dag.rs | Adds owner→remote reverse lookup and skips foreign PRs during tip detection and push-need detection. |
| src/tests.rs | Passes remote_owners into pr_dag::build() for show/log/sync helpers; adds regression tests for foreign PR collisions and duplicate owners. |
| src/snapshots/jj_pr__tests__sync@cargo-smart-release-73.json.gz.snap | Adds snapshot output for sync using the updated fixture format. |
| src/snapshots/jj_pr__tests__show@cargo-smart-release-73.json.gz.snap | Adds snapshot output for show using the updated fixture format. |
| src/snapshots/jj_pr__tests__show-all@cargo-smart-release-73.json.gz.snap | Adds snapshot output for show --all using the updated fixture format. |
| src/snapshots/jj_pr__tests__log@cargo-smart-release-73.json.gz.snap | Adds snapshot output for log using the updated fixture format. |
| src/snapshots/jj_pr__tests__log-all@cargo-smart-release-73.json.gz.snap | Adds snapshot output for log --all using the updated fixture format. |
| src/snapshots/jj_pr__tests__foreign_pr_collision_show.snap | Adds snapshot verifying foreign PR is excluded from show. |
| src/snapshots/jj_pr__tests__foreign_pr_collision_sync.snap | Adds snapshot verifying foreign PR is excluded from sync planning. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The test now includes a PR with head_repo_owner matching the duplicated owner, which triggers the ambiguity warning in the pr_needs_push path. Removed the debugging logs_assert call and asserts the warning message via logs_contain. Co-authored-by: Infinity 🤖 <infinity@hydro.run> PR: #69
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a PR from another user's fork has a
headRefNamethat collides witha local bookmark (e.g. someone pushes from their fork's "main" branch,
matching the local "main" default branch), jj-pr incorrectly treated the
local bookmark as the head branch for that foreign PR.
The fix adds an ownership check in two places in
pr_dag::build():cid_pr_tiploop (PR tip detection) - skips PRs whosehead_repo_ownerdoesn't match any configured remote ownerpr_needs_pushloop - same check to avoid flagging pushes forforeign PRs
The check is: if
head_repo_owneris set ANDremote_ownersispopulated AND no remote owner matches the PR's owner, skip the PR.
This preserves backward compatibility with legacy fixtures where
remote_ownersis empty.Also fixed test helpers to use
input.remote_ownersinstead ofhardcoded empty maps, converted the cargo-smart-release-73 fixture to
.json.gzformat with properremote_ownersdata, and removed theold
.gzfile.Co-authored-by: Infinity 🤖 infinity@hydro.run