diff options
| author | DanConwayDev <DanConwayDev@protonmail.com> | 2026-03-05 21:25:50 +0000 |
|---|---|---|
| committer | DanConwayDev <DanConwayDev@protonmail.com> | 2026-03-05 22:12:07 +0000 |
| commit | f6d1e03dc99b3ea48cb6e4bd1d3371dd924a567a (patch) | |
| tree | 19fb789e83bd8d4ccc25f735a80cf41c0eccad17 /src/bin/ngit/sub_commands | |
| parent | 83b0886b97e2e90e328f91fcfaeb59726c93308f (diff) | |
fix(pr-checkout): require --force on diverged proposal branch
checkout_patch() previously re-applied the patch chain whenever the local
branch tip didn't match the published tip, silently overwriting local
amendments and rebased revisions without warning.
Now detects the relationship between local and published tips:
- up to date: check out as-is
- behind (local is ancestor of published): fast-forward, no flag needed
- local commits on top (published is ancestor of local): check out as-is
- diverged (neither ancestor): bail with guidance, --force to overwrite
- published tip not found locally and branch exists: same as diverged
Also adds --force flag to `ngit pr checkout` to explicitly opt in to
overwriting a diverged branch, covering both local amendments and
author force-pushes.
Bug discovered during test implementation in tests/ngit_pr_checkout.rs.
Diffstat (limited to 'src/bin/ngit/sub_commands')
| -rw-r--r-- | src/bin/ngit/sub_commands/checkout.rs | 294 |
1 files changed, 213 insertions, 81 deletions
diff --git a/src/bin/ngit/sub_commands/checkout.rs b/src/bin/ngit/sub_commands/checkout.rs index 67447ae..ca9005f 100644 --- a/src/bin/ngit/sub_commands/checkout.rs +++ b/src/bin/ngit/sub_commands/checkout.rs | |||
| @@ -28,7 +28,7 @@ use crate::{ | |||
| 28 | repo_ref::get_repo_coordinates_when_remote_unknown, | 28 | repo_ref::get_repo_coordinates_when_remote_unknown, |
| 29 | }; | 29 | }; |
| 30 | 30 | ||
| 31 | pub async fn launch(id: &str, offline: bool) -> Result<()> { | 31 | pub async fn launch(id: &str, force: bool, offline: bool) -> Result<()> { |
| 32 | let event_id = parse_event_id(id)?; | 32 | let event_id = parse_event_id(id)?; |
| 33 | 33 | ||
| 34 | let git_repo = Repo::discover().context("failed to find a git repository")?; | 34 | let git_repo = Repo::discover().context("failed to find a git repository")?; |
| @@ -89,6 +89,7 @@ pub async fn launch(id: &str, offline: bool) -> Result<()> { | |||
| 89 | &cover_letter, | 89 | &cover_letter, |
| 90 | &most_recent_proposal_patch_chain_or_pr_or_pr_update, | 90 | &most_recent_proposal_patch_chain_or_pr_or_pr_update, |
| 91 | nostr_remote.as_ref().map(|(name, _)| name.as_str()), | 91 | nostr_remote.as_ref().map(|(name, _)| name.as_str()), |
| 92 | force, | ||
| 92 | ) | 93 | ) |
| 93 | } else { | 94 | } else { |
| 94 | checkout_patch( | 95 | checkout_patch( |
| @@ -96,6 +97,7 @@ pub async fn launch(id: &str, offline: bool) -> Result<()> { | |||
| 96 | &cover_letter, | 97 | &cover_letter, |
| 97 | &most_recent_proposal_patch_chain_or_pr_or_pr_update, | 98 | &most_recent_proposal_patch_chain_or_pr_or_pr_update, |
| 98 | nostr_remote.as_ref().map(|(name, _)| name.as_str()), | 99 | nostr_remote.as_ref().map(|(name, _)| name.as_str()), |
| 100 | force, | ||
| 99 | ) | 101 | ) |
| 100 | } | 102 | } |
| 101 | } | 103 | } |
| @@ -168,106 +170,155 @@ fn parse_event_id(id: &str) -> Result<EventId> { | |||
| 168 | bail!("invalid event-id or nevent: {id}") | 170 | bail!("invalid event-id or nevent: {id}") |
| 169 | } | 171 | } |
| 170 | 172 | ||
| 173 | fn print_diverged_branch_help(branch_name: &str) { | ||
| 174 | eprintln!( | ||
| 175 | "{}", | ||
| 176 | console::style(format!( | ||
| 177 | "Branch '{branch_name}' has diverged from the published proposal." | ||
| 178 | )) | ||
| 179 | .yellow() | ||
| 180 | ); | ||
| 181 | eprintln!( | ||
| 182 | "{}", | ||
| 183 | console::style( | ||
| 184 | "This may be because you have local amendments, or the author force-pushed a new revision." | ||
| 185 | ) | ||
| 186 | .yellow() | ||
| 187 | ); | ||
| 188 | eprintln!( | ||
| 189 | "{}", | ||
| 190 | console::style("To overwrite local branch with the published version:").yellow() | ||
| 191 | ); | ||
| 192 | eprintln!( | ||
| 193 | "{}", | ||
| 194 | console::style(" ngit pr checkout --force <id>").yellow() | ||
| 195 | ); | ||
| 196 | eprintln!( | ||
| 197 | "{}", | ||
| 198 | console::style("To publish your local amendments as a new revision:").yellow() | ||
| 199 | ); | ||
| 200 | eprintln!("{}", console::style(" ngit push --force").yellow()); | ||
| 201 | } | ||
| 202 | |||
| 171 | fn checkout_pr( | 203 | fn checkout_pr( |
| 172 | git_repo: &Repo, | 204 | git_repo: &Repo, |
| 173 | repo_ref: &RepoRef, | 205 | repo_ref: &RepoRef, |
| 174 | cover_letter: &crate::git_events::CoverLetter, | 206 | cover_letter: &crate::git_events::CoverLetter, |
| 175 | most_recent_proposal_patch_chain_or_pr_or_pr_update: &[nostr::Event], | 207 | most_recent_proposal_patch_chain_or_pr_or_pr_update: &[nostr::Event], |
| 176 | nostr_remote_name: Option<&str>, | 208 | nostr_remote_name: Option<&str>, |
| 209 | force: bool, | ||
| 177 | ) -> Result<()> { | 210 | ) -> Result<()> { |
| 178 | let branch_name = cover_letter.get_branch_name_with_pr_prefix_and_shorthand_id()?; | 211 | let branch_name = cover_letter.get_branch_name_with_pr_prefix_and_shorthand_id()?; |
| 179 | let proposal_tip_event = most_recent_proposal_patch_chain_or_pr_or_pr_update | 212 | let proposal_tip_event = most_recent_proposal_patch_chain_or_pr_or_pr_update |
| 180 | .first() | 213 | .first() |
| 181 | .context("most_recent_proposal_patch_chain_or_pr_or_pr_update will always contain an event with c tag")?; | 214 | .context("most_recent_proposal_patch_chain_or_pr_or_pr_update will always contain an event with c tag")?; |
| 182 | let proposal_tip = tag_value(proposal_tip_event, "c")?; | 215 | let proposal_tip = tag_value(proposal_tip_event, "c")?; |
| 216 | let proposal_tip_sha1 = str_to_sha1(&proposal_tip)?; | ||
| 217 | |||
| 218 | // Case 1: branch doesn't exist yet — create it. | ||
| 219 | let Ok(local_branch_tip) = git_repo.get_tip_of_branch(&branch_name) else { | ||
| 220 | if let Some(remote_name) = nostr_remote_name { | ||
| 221 | let remote_branch = format!("{remote_name}/{branch_name}"); | ||
| 222 | if git_repo.get_tip_of_branch(&remote_branch).is_ok() { | ||
| 223 | checkout_remote_branch_with_tracking(git_repo, remote_name, &branch_name)?; | ||
| 224 | println!( | ||
| 225 | "checked out proposal branch '{branch_name}' with tracking to {remote_name}" | ||
| 226 | ); | ||
| 227 | return Ok(()); | ||
| 228 | } | ||
| 229 | } | ||
| 230 | fetch_oid_for_from_servers_for_pr(&proposal_tip, git_repo, repo_ref, proposal_tip_event)?; | ||
| 231 | git_repo.create_branch_at_commit(&branch_name, &proposal_tip)?; | ||
| 232 | git_repo.checkout(&branch_name)?; | ||
| 233 | println!("created and checked out proposal branch '{branch_name}'"); | ||
| 234 | return Ok(()); | ||
| 235 | }; | ||
| 183 | 236 | ||
| 184 | if let Ok(local_branch_tip) = git_repo.get_tip_of_branch(&branch_name) { | 237 | // Case 2: up to date. |
| 238 | if local_branch_tip.to_string() == proposal_tip { | ||
| 185 | git_repo | 239 | git_repo |
| 186 | .checkout(&branch_name) | 240 | .checkout(&branch_name) |
| 187 | .context("cannot checkout existing proposal branch")?; | 241 | .context("cannot checkout existing proposal branch")?; |
| 188 | if local_branch_tip.to_string() == proposal_tip { | 242 | println!("checked out up-to-date proposal branch '{branch_name}'"); |
| 189 | println!("checked out up-to-date proposal branch '{branch_name}'"); | 243 | return Ok(()); |
| 190 | return Ok(()); | 244 | } |
| 191 | } | ||
| 192 | 245 | ||
| 193 | let has_tracking = git_repo.get_upstream_for_branch(&branch_name)?.is_some(); | 246 | // Branch has a tracking remote — defer to git pull for updates. |
| 247 | if git_repo.get_upstream_for_branch(&branch_name)?.is_some() { | ||
| 248 | git_repo | ||
| 249 | .checkout(&branch_name) | ||
| 250 | .context("cannot checkout existing proposal branch")?; | ||
| 251 | println!( | ||
| 252 | "{}", | ||
| 253 | console::style(format!( | ||
| 254 | "Local branch '{branch_name}' is behind. Run git pull to update." | ||
| 255 | )) | ||
| 256 | .yellow() | ||
| 257 | ); | ||
| 258 | return Ok(()); | ||
| 259 | } | ||
| 194 | 260 | ||
| 195 | if has_tracking { | 261 | if git_repo.does_commit_exist(&proposal_tip)? { |
| 196 | println!( | 262 | let local_is_ancestor_of_published = |
| 197 | "{}", | 263 | git_repo.ancestor_of(&proposal_tip_sha1, &local_branch_tip)?; |
| 198 | console::style(format!( | 264 | let published_is_ancestor_of_local = |
| 199 | "Local branch '{branch_name}' is behind. Run git pull to update." | 265 | git_repo.ancestor_of(&local_branch_tip, &proposal_tip_sha1)?; |
| 200 | )) | 266 | |
| 201 | .yellow() | 267 | // Case 3: branch is behind — fast-forward. |
| 202 | ); | 268 | if local_is_ancestor_of_published { |
| 269 | git_repo.create_branch_at_commit(&branch_name, &proposal_tip)?; | ||
| 270 | git_repo.checkout(&branch_name)?; | ||
| 271 | println!("checked out proposal branch and updated tip '{branch_name}'"); | ||
| 203 | return Ok(()); | 272 | return Ok(()); |
| 204 | } | 273 | } |
| 205 | 274 | ||
| 206 | if git_repo.does_commit_exist(&proposal_tip)? { | 275 | // Case 4: local commits on top — check out without touching them. |
| 207 | if git_repo.ancestor_of(&str_to_sha1(&proposal_tip)?, &local_branch_tip)? { | 276 | if published_is_ancestor_of_local { |
| 208 | git_repo.create_branch_at_commit(&branch_name, &proposal_tip)?; | 277 | git_repo |
| 209 | git_repo.checkout(&branch_name)?; | 278 | .checkout(&branch_name) |
| 210 | println!("checked out proposal branch and updated tip '{branch_name}'"); | 279 | .context("cannot checkout existing proposal branch")?; |
| 211 | return Ok(()); | ||
| 212 | } | ||
| 213 | println!( | ||
| 214 | "{}", | ||
| 215 | console::style(format!( | ||
| 216 | "Branch '{branch_name}' has diverged from proposal tip." | ||
| 217 | )) | ||
| 218 | .yellow() | ||
| 219 | ); | ||
| 220 | println!("{}", console::style("To reset to proposal tip:").yellow()); | ||
| 221 | println!( | ||
| 222 | "{}", | ||
| 223 | console::style(format!(" git reset --hard {proposal_tip}")).yellow() | ||
| 224 | ); | ||
| 225 | println!( | ||
| 226 | "{}", | ||
| 227 | console::style("To rebase local commits onto proposal tip:").yellow() | ||
| 228 | ); | ||
| 229 | println!( | 280 | println!( |
| 230 | "{}", | 281 | "checked out proposal branch '{branch_name}' (local branch has unpublished commits on top)" |
| 231 | console::style(format!(" git rebase {proposal_tip}")).yellow() | ||
| 232 | ); | 282 | ); |
| 233 | bail!("branch diverged from proposal"); | 283 | return Ok(()); |
| 234 | } | 284 | } |
| 235 | |||
| 236 | bail!( | ||
| 237 | "proposal tip {proposal_tip} not found locally and branch has no tracking remote. \n\ | ||
| 238 | Try fetching from git servers first." | ||
| 239 | ); | ||
| 240 | } | 285 | } |
| 241 | 286 | ||
| 242 | if let Some(remote_name) = nostr_remote_name { | 287 | // Case 5 (and tip-not-found): diverged — require --force. |
| 243 | let remote_branch = format!("{remote_name}/{branch_name}"); | 288 | if force { |
| 244 | if git_repo.get_tip_of_branch(&remote_branch).is_ok() { | 289 | fetch_oid_for_from_servers_for_pr(&proposal_tip, git_repo, repo_ref, proposal_tip_event)?; |
| 245 | checkout_remote_branch_with_tracking(git_repo, remote_name, &branch_name)?; | 290 | git_repo.create_branch_at_commit(&branch_name, &proposal_tip)?; |
| 246 | println!("checked out proposal branch '{branch_name}' with tracking to {remote_name}"); | 291 | git_repo.checkout(&branch_name)?; |
| 247 | return Ok(()); | 292 | println!( |
| 248 | } | 293 | "checked out proposal branch '{branch_name}' updated to published tip (overwrote diverged local branch)" |
| 294 | ); | ||
| 295 | return Ok(()); | ||
| 249 | } | 296 | } |
| 250 | 297 | ||
| 251 | fetch_oid_for_from_servers_for_pr(&proposal_tip, git_repo, repo_ref, proposal_tip_event)?; | 298 | git_repo |
| 252 | git_repo.create_branch_at_commit(&branch_name, &proposal_tip)?; | 299 | .checkout(&branch_name) |
| 253 | git_repo.checkout(&branch_name)?; | 300 | .context("cannot checkout existing proposal branch")?; |
| 254 | println!("created and checked out proposal branch '{branch_name}'"); | 301 | print_diverged_branch_help(&branch_name); |
| 255 | Ok(()) | 302 | bail!( |
| 303 | "branch '{branch_name}' has diverged from the published proposal; use --force to overwrite" | ||
| 304 | ) | ||
| 256 | } | 305 | } |
| 257 | 306 | ||
| 307 | #[allow(clippy::too_many_lines)] | ||
| 258 | fn checkout_patch( | 308 | fn checkout_patch( |
| 259 | git_repo: &Repo, | 309 | git_repo: &Repo, |
| 260 | cover_letter: &crate::git_events::CoverLetter, | 310 | cover_letter: &crate::git_events::CoverLetter, |
| 261 | most_recent_proposal_patch_chain_or_pr_or_pr_update: &[nostr::Event], | 311 | most_recent_proposal_patch_chain_or_pr_or_pr_update: &[nostr::Event], |
| 262 | nostr_remote_name: Option<&str>, | 312 | nostr_remote_name: Option<&str>, |
| 313 | force: bool, | ||
| 263 | ) -> Result<()> { | 314 | ) -> Result<()> { |
| 264 | let (_, _master_tip) = git_repo.get_main_or_master_branch()?; | ||
| 265 | |||
| 266 | if git_repo.has_outstanding_changes()? { | 315 | if git_repo.has_outstanding_changes()? { |
| 267 | bail!("working directory is not clean. Discard or stash (un)staged changes and try again."); | 316 | bail!("working directory is not clean. Discard or stash (un)staged changes and try again."); |
| 268 | } | 317 | } |
| 269 | 318 | ||
| 270 | let branch_name = cover_letter.get_branch_name_with_pr_prefix_and_shorthand_id()?; | 319 | let branch_name = cover_letter.get_branch_name_with_pr_prefix_and_shorthand_id()?; |
| 320 | |||
| 321 | // Case 1: branch doesn't exist yet — create and apply. | ||
| 271 | let branch_exists = git_repo | 322 | let branch_exists = git_repo |
| 272 | .get_local_branch_names() | 323 | .get_local_branch_names() |
| 273 | .context("failed to get local branch names")? | 324 | .context("failed to get local branch names")? |
| @@ -297,34 +348,115 @@ fn checkout_patch( | |||
| 297 | 348 | ||
| 298 | let local_branch_tip = git_repo.get_tip_of_branch(&branch_name)?; | 349 | let local_branch_tip = git_repo.get_tip_of_branch(&branch_name)?; |
| 299 | 350 | ||
| 300 | // If we can reliably determine the proposal tip commit, use it to skip | 351 | // Resolve the published tip commit id. If we can't (no commit tag), fall |
| 301 | // re-applying when already up-to-date. If the commit tag is absent or | 352 | // through to apply_patch_chain which handles idempotency itself. |
| 302 | // unreliable, skip this check and let apply_patch_chain handle idempotency. | 353 | let Ok(proposal_tip_str) = get_commit_id_from_patch( |
| 303 | if let Ok(proposal_tip_str) = get_commit_id_from_patch( | ||
| 304 | most_recent_proposal_patch_chain_or_pr_or_pr_update | 354 | most_recent_proposal_patch_chain_or_pr_or_pr_update |
| 305 | .first() | 355 | .first() |
| 306 | .context("there should be at least one patch")?, | 356 | .context("there should be at least one patch")?, |
| 307 | ) { | 357 | ) else { |
| 308 | if let Ok(proposal_tip) = str_to_sha1(&proposal_tip_str) { | 358 | git_repo.checkout(&branch_name)?; |
| 309 | if proposal_tip.eq(&local_branch_tip) { | 359 | let _ = git_repo |
| 310 | git_repo.checkout(&branch_name)?; | 360 | .apply_patch_chain( |
| 311 | println!("branch '{branch_name}' checked out and up-to-date"); | 361 | &branch_name, |
| 312 | return Ok(()); | 362 | most_recent_proposal_patch_chain_or_pr_or_pr_update.to_vec(), |
| 313 | } | 363 | ) |
| 364 | .context("failed to apply patch chain")?; | ||
| 365 | println!("checked out updated proposal as '{branch_name}' branch"); | ||
| 366 | return Ok(()); | ||
| 367 | }; | ||
| 368 | |||
| 369 | let Ok(proposal_tip) = str_to_sha1(&proposal_tip_str) else { | ||
| 370 | git_repo.checkout(&branch_name)?; | ||
| 371 | println!("checked out proposal as '{branch_name}' branch"); | ||
| 372 | return Ok(()); | ||
| 373 | }; | ||
| 374 | |||
| 375 | // Case 2: already up to date. | ||
| 376 | if proposal_tip.eq(&local_branch_tip) { | ||
| 377 | git_repo.checkout(&branch_name)?; | ||
| 378 | println!("branch '{branch_name}' checked out and up-to-date"); | ||
| 379 | return Ok(()); | ||
| 380 | } | ||
| 381 | |||
| 382 | // For cases 3-5 we need to know the ancestry relationship. | ||
| 383 | if git_repo.does_commit_exist(&proposal_tip_str)? { | ||
| 384 | let published_is_ancestor_of_local = | ||
| 385 | git_repo.ancestor_of(&local_branch_tip, &proposal_tip)?; | ||
| 386 | let local_is_ancestor_of_published = | ||
| 387 | git_repo.ancestor_of(&proposal_tip, &local_branch_tip)?; | ||
| 388 | |||
| 389 | // Case 3: branch is behind — local tip is an ancestor of the published | ||
| 390 | // tip, meaning the author appended new patches. Fast-forward. | ||
| 391 | if local_is_ancestor_of_published { | ||
| 392 | git_repo.checkout(&branch_name)?; | ||
| 393 | let _ = git_repo | ||
| 394 | .apply_patch_chain( | ||
| 395 | &branch_name, | ||
| 396 | most_recent_proposal_patch_chain_or_pr_or_pr_update.to_vec(), | ||
| 397 | ) | ||
| 398 | .context("failed to apply patch chain")?; | ||
| 399 | println!("checked out updated proposal as '{branch_name}' branch"); | ||
| 400 | return Ok(()); | ||
| 314 | } | 401 | } |
| 402 | |||
| 403 | // Case 4: local has commits stacked on top of the published tip — | ||
| 404 | // published tip is an ancestor of local tip. Check out without touching | ||
| 405 | // commits. | ||
| 406 | if published_is_ancestor_of_local { | ||
| 407 | git_repo.checkout(&branch_name)?; | ||
| 408 | println!( | ||
| 409 | "checked out proposal branch '{branch_name}' (local branch has unpublished commits on top)" | ||
| 410 | ); | ||
| 411 | return Ok(()); | ||
| 412 | } | ||
| 413 | |||
| 414 | // Case 5: diverged — neither is an ancestor of the other. | ||
| 415 | // This covers both local amendments and author force-pushes. | ||
| 416 | // Require --force to overwrite. | ||
| 417 | if force { | ||
| 418 | git_repo.checkout(&branch_name)?; | ||
| 419 | let _ = git_repo | ||
| 420 | .apply_patch_chain( | ||
| 421 | &branch_name, | ||
| 422 | most_recent_proposal_patch_chain_or_pr_or_pr_update.to_vec(), | ||
| 423 | ) | ||
| 424 | .context("failed to apply patch chain")?; | ||
| 425 | println!( | ||
| 426 | "checked out updated proposal as '{branch_name}' branch (overwrote diverged local branch)" | ||
| 427 | ); | ||
| 428 | return Ok(()); | ||
| 429 | } | ||
| 430 | |||
| 431 | git_repo.checkout(&branch_name)?; | ||
| 432 | print_diverged_branch_help(&branch_name); | ||
| 433 | bail!( | ||
| 434 | "branch '{branch_name}' has diverged from the published proposal; use --force to overwrite" | ||
| 435 | ); | ||
| 436 | } | ||
| 437 | |||
| 438 | // Published tip not found locally and branch already exists — the author | ||
| 439 | // has published a new revision whose commits we don't have yet. Treat as | ||
| 440 | // diverged: require --force to overwrite. | ||
| 441 | if force { | ||
| 442 | git_repo.checkout(&branch_name)?; | ||
| 443 | let _ = git_repo | ||
| 444 | .apply_patch_chain( | ||
| 445 | &branch_name, | ||
| 446 | most_recent_proposal_patch_chain_or_pr_or_pr_update.to_vec(), | ||
| 447 | ) | ||
| 448 | .context("failed to apply patch chain")?; | ||
| 449 | println!( | ||
| 450 | "checked out updated proposal as '{branch_name}' branch (overwrote diverged local branch)" | ||
| 451 | ); | ||
| 452 | return Ok(()); | ||
| 315 | } | 453 | } |
| 316 | 454 | ||
| 317 | // Branch exists but may need updating — re-apply the chain. | ||
| 318 | // apply_patch_chain handles already-applied commits idempotently. | ||
| 319 | git_repo.checkout(&branch_name)?; | 455 | git_repo.checkout(&branch_name)?; |
| 320 | let _ = git_repo | 456 | print_diverged_branch_help(&branch_name); |
| 321 | .apply_patch_chain( | 457 | bail!( |
| 322 | &branch_name, | 458 | "branch '{branch_name}' has diverged from the published proposal; use --force to overwrite" |
| 323 | most_recent_proposal_patch_chain_or_pr_or_pr_update.to_vec(), | 459 | ) |
| 324 | ) | ||
| 325 | .context("failed to apply patch chain")?; | ||
| 326 | println!("checked out updated proposal as '{branch_name}' branch"); | ||
| 327 | Ok(()) | ||
| 328 | } | 460 | } |
| 329 | 461 | ||
| 330 | fn fetch_oid_for_from_servers_for_pr( | 462 | fn fetch_oid_for_from_servers_for_pr( |