From 01aeb2a3265bcafa162987c85dd281981770bba7 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Thu, 26 Feb 2026 16:35:59 +0000 Subject: fix: correct merge-base in PR events from git push of pr/ branch When pushing a pr/ branch, the ahead slice is reversed by callers before being passed to generate_patches_or_pr_event_or_pr_updates, making it oldest-first. The tip/first_commit assignments were backwards (using first()/last() as if the slice were youngest-first), so the merge-base was computed as the parent of the PR tip rather than the parent of the oldest commit. Multi-commit PRs therefore showed only 1 commit when applied via ngit apply. Adds an integration test that pushes a two-commit large-file PR branch and asserts the merge-base tag equals the main branch tip. --- CHANGELOG.md | 1 + src/bin/git_remote_nostr/push.rs | 4 +- tests/git_remote_nostr/push.rs | 99 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 102 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 31716ff..d4b8083 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `merge-base` tag in PR events generated by `git push` of a `pr/` branch was set to the parent of the PR tip instead of the actual base commit; multi-commit PRs showed only 1 commit when applied via `ngit apply` - `git-remote-nostr` list now advertises the newest state event whose OIDs are all confirmed present on a git server or locally, rather than unconditionally using the latest nostr state event; this prevents catastrophic fetch/clone failures when a state event was published before the corresponding git push completed - Tag tracking refs written with wrong path (`refs/remotes/origin/refs/tags/v1.0.0` instead of `refs/remotes/origin/v1.0.0`) after a push via `git-remote-nostr`, causing `ngit sync` to fail with "src refspec does not match any existing object" when syncing tags - Annotated tag tracking refs stored with the peeled commit OID instead of the tag object OID after a push via `git-remote-nostr`; this caused `ngit sync` to push the wrong object to grasp servers, which rejected it because the nostr state event referenced the tag object OID diff --git a/src/bin/git_remote_nostr/push.rs b/src/bin/git_remote_nostr/push.rs index bd1188b..870f22d 100644 --- a/src/bin/git_remote_nostr/push.rs +++ b/src/bin/git_remote_nostr/push.rs @@ -623,8 +623,8 @@ async fn generate_patches_or_pr_event_or_pr_updates( let use_pr = parent_is_pr || git_repo.are_commits_too_big_for_patches(ahead); if use_pr { - let tip = ahead.first().context("no commits")?; // ahead is youngest first - let first_commit = ahead.last().context("no commits")?; + let tip = ahead.last().context("no commits")?; // ahead is oldest first (callers reverse it) + let first_commit = ahead.first().context("no commits")?; let push_options_refs: Vec<&str> = git_server_push_options.iter().map(String::as_str).collect(); select_servers_push_refs_and_generate_pr_or_pr_update_event( diff --git a/tests/git_remote_nostr/push.rs b/tests/git_remote_nostr/push.rs index 6467dec..254208f 100644 --- a/tests/git_remote_nostr/push.rs +++ b/tests/git_remote_nostr/push.rs @@ -2122,6 +2122,105 @@ async fn force_push_to_existing_patch_series_with_title_description_options_crea Ok(()) } +#[tokio::test] +#[serial] +async fn push_new_pr_branch_with_multiple_commits_sets_merge_base_to_main_tip() -> Result<()> { + let (events, source_git_repo) = prep_source_repo_and_events_including_proposals().await?; + let _source_path = source_git_repo.dir.to_str().unwrap().to_string(); + + let (mut r51, mut r52, mut r53, mut r55, mut r56, mut r57) = ( + Relay::new(8051, None, None), + Relay::new(8052, None, None), + Relay::new(8053, None, None), + Relay::new(8055, None, None), + Relay::new(8056, None, None), + Relay::new(8057, None, None), + ); + r51.events = events.clone(); + r55.events = events.clone(); + + #[allow(clippy::mutable_key_type)] + let before = r55.events.iter().cloned().collect::>(); + let branch_name = "pr/multi-commit-pr"; + + let cli_tester_handle = std::thread::spawn(move || -> Result { + let mut git_repo = clone_git_repo_with_nostr_url()?; + git_repo.delete_dir_on_drop = false; + + // Record the main tip — this should become the merge-base in the PR event + let main_tip = git_repo.get_tip_of_local_branch("main")?.to_string(); + + git_repo.create_branch(branch_name)?; + git_repo.checkout(branch_name)?; + + // Add two large commits so the push is forced into PR (not patch) mode + let large_content = "x".repeat(70 * 1024); + std::fs::write(git_repo.dir.join("large1.txt"), &large_content)?; + git_repo.stage_and_commit("add large1")?; + + std::fs::write(git_repo.dir.join("large2.txt"), &large_content)?; + git_repo.stage_and_commit("add large2")?; + + let mut p = CliTester::new_git_with_remote_helper_from_dir( + &git_repo.dir, + ["push", "-u", "origin", branch_name], + ); + cli_expect_nostr_fetch(&mut p)?; + p.expect("git servers: listing refs...\r\n")?; + p.expect_eventually_and_print(format!("To {}\r\n", get_nostr_remote_url()?).as_str())?; + let output = p.expect_end_eventually()?; + + for p in [51, 52, 53, 55, 56, 57] { + relay::shutdown_relay(8000 + p)?; + } + + Ok(format!("{main_tip}\n{output}")) + }); + let _ = join!( + r51.listen_until_close(), + r52.listen_until_close(), + r53.listen_until_close(), + r55.listen_until_close(), + r56.listen_until_close(), + r57.listen_until_close(), + ); + + let result = cli_tester_handle.join().unwrap()?; + let (main_tip, _output) = result.split_once('\n').unwrap(); + + let new_events = r55 + .events + .iter() + .cloned() + .collect::>() + .difference(&before) + .cloned() + .collect::>(); + assert_eq!(new_events.len(), 1, "should create exactly 1 PR event"); + + let pr_event = new_events.first().unwrap(); + assert!( + pr_event.kind.eq(&KIND_PULL_REQUEST), + "event should be a PR event" + ); + + let merge_base_tag = pr_event + .tags + .iter() + .find(|t| t.as_slice()[0].eq("merge-base")); + assert!( + merge_base_tag.is_some(), + "PR event should have a merge-base tag" + ); + assert_eq!( + merge_base_tag.unwrap().as_slice()[1], + main_tip, + "merge-base should be the main branch tip at the time of branching, not the parent of the PR tip" + ); + + Ok(()) +} + mod push_from_another_maintainer { // TODO that has issued announcement -- cgit v1.2.3