diff --git a/scripts/release.sh b/scripts/release.sh index d21c3d35e..1a7204c84 100755 --- a/scripts/release.sh +++ b/scripts/release.sh @@ -30,6 +30,10 @@ # instead; a v tag anywhere else on origin is refused. A bump PR # closed without merging is refused while release- is on origin; # once that branch is deleted, the next run starts over. +# A resumed PR must belong to origin and match its branch tip while open, or +# it is refused, even once tagged. Its tree (or a pushed branch with no PR) +# must also be exactly the version bump in Cargo.toml and Cargo.lock from +# its merge base, or it is refused before merging or tagging. # # Prints a progress line on stderr for each step, with the release workflow # run's URL once it waits for that run, and the GitHub Release's URL last. @@ -67,6 +71,9 @@ main() { fetch_main find_earlier_run + case $pr_state in + OPEN | MERGED) validate_bump_pr ;; + esac finish_if_tagged case $pr_state in @@ -77,6 +84,7 @@ main() { git push --quiet origin "$head:refs/heads/$branch" else progress "resuming: $branch is on origin at ${head:0:7} with no PR" + validate_bump_pr write_summary "no PR was opened" fi open_pr @@ -104,25 +112,68 @@ main() { # CLOSED, or empty with no PR), url, head and merge from the newest PR from # $branch, or with no PR, head from $branch on origin if it was pushed. A PR # closed without merging whose branch is gone from origin counts as no PR. +# Refuses a PR from another repository, or an open PR whose head is not +# $branch on origin. find_earlier_run() { - local pr - pr=$(gh pr list --head "$branch" --base main --state all --limit 1 \ - --json state,url,headRefOid,mergeCommit \ - --jq '.[0] // empty | "\(.state) \(.url) \(.headRefOid) \(.mergeCommit.oid // "")"') - read -r pr_state url head merge <<<"$pr" + local pr cross_repository origin_head + pr=$(gh pr list --repo "$(git remote get-url origin)" --head "$branch" --base main --state all --limit 1 \ + --json state,url,headRefOid,mergeCommit,isCrossRepository \ + --jq '.[0] // empty | "\(.isCrossRepository) \(.state) \(.url) \(.headRefOid) \(.mergeCommit.oid // "")"') + read -r cross_repository pr_state url head merge <<<"$pr" + origin_head=$(git ls-remote origin "refs/heads/$branch" | cut -f 1) # A PR closed without merging whose branch was then deleted leaves - # nothing to resume: start over. - if [ "$pr_state" = CLOSED ] && [ -z "$(git ls-remote origin "refs/heads/$branch")" ]; then + # nothing to resume, whichever repository it came from: start over. + if [ "$pr_state" = CLOSED ] && [ -z "$origin_head" ]; then pr_state='' url='' head='' merge='' fi + if [ -n "$pr_state" ] && [ "$cross_repository" != false ]; then + refuse "$url has its head in another repository, not origin" + fi + if [ "$pr_state" = OPEN ] && { [ -z "$origin_head" ] || [ "$head" != "$origin_head" ]; }; then + refuse "$url head does not match refs/heads/$branch on origin" + fi if [ -z "$pr_state" ]; then - head=$(git ls-remote origin "refs/heads/$branch" | cut -f 1) + head=$origin_head if [ -n "$head" ]; then git fetch --quiet origin "refs/heads/$branch" fi fi } +# validate_bump_pr +# Refuses unless the selected PR's (or pushed branch's) entire tree is the +# bump_version result from its merge base. A merged PR uses main just before +# its merge, since its head is already an ancestor of main now. +validate_bump_pr() { + local base target expected candidate=${url:-$branch on origin} + if ! git fetch --quiet origin "$head"; then + refuse "$candidate head could not be fetched from origin" + fi + target=origin/main + if [ "$pr_state" = MERGED ]; then + target=$merge^1 + fi + base=$(git merge-base "$head" "$target") + expected=$(bump_tree "$base") + if [ "$expected" != "$(git rev-parse "$head^{tree}")" ]; then + refuse "$candidate changes more than the version bump to $version in Cargo.toml and Cargo.lock" + fi +} + +# bump_tree +# The tree commit_bump would make, without committing or touching the caller's +# checkout. The subshell owns the temporary worktree and its cleanup trap. +bump_tree() ( + local repo=$PWD work + work=$(mktemp -d) + trap 'git -C "$repo" worktree remove --force "$work" 2>/dev/null || rm -rf "$work"' EXIT + git worktree add --quiet --detach "$work" "$1" + cd "$work" + bump_version "$version" + git add Cargo.toml Cargo.lock + git write-tree +) + # finish_if_tagged # After an earlier run, if $tag is on origin at the merge of the bump PR, # waits for its release workflow run and exits as wait_for_release does, and @@ -202,7 +253,7 @@ wait_and_merge() { exit 1 fi - gh pr merge "$branch" --merge --match-head-commit "$head" >/dev/null + gh pr merge "$url" --merge --match-head-commit "$head" >/dev/null fetch_main merge=$(merge_commit_of "$head") progress "merged $url as ${merge:0:7}" diff --git a/tests/fakes/gh.rs b/tests/fakes/gh.rs index 4d61179bf..83f1014e1 100644 --- a/tests/fakes/gh.rs +++ b/tests/fakes/gh.rs @@ -102,7 +102,7 @@ //! //! `gh api --method PATCH repos//pulls/ -f body=` sets the //! PR's body. `gh pr view`, `gh pr ready` and `gh pr merge` name the PR by -//! its head branch or its number; numeric merge reads that PR's actual head. +//! its head branch, number or URL; a number or URL selects that PR's actual head. //! //! `gh issue close` closes the issue, recording its `--comment`; on an issue //! already closed it only warns, as gh does. @@ -266,9 +266,11 @@ fn check_repo(state: &Json, flags: &Flags) { /// Fail unless `repo`, if any, is the fake's. `{owner}/{repo}` is gh's /// placeholder for the current directory's repo, which is always the fake's. +/// The local origin path stands in for that repository's remote URL. fn check_repo_is(state: &Json, repo: Option<&str>) { if let Some(repo) = repo && repo != "{owner}/{repo}" + && Path::new(repo) != origin_repo() && repo.to_lowercase() != state.at("repo").str().to_lowercase() { die(&format!("fake gh: unknown repo {repo}"), 1); @@ -287,9 +289,14 @@ fn newest_pr_from(state: &Json, head: &str) -> usize { .unwrap_or_else(|| no_pr_for(head)) } -/// Resolve a numeric identity or, for fake agents, a head branch. +/// Resolve a number or URL, or, for fake agents, a head branch. fn selected_pr(state: &Json, name: &str) -> usize { - if !name.is_empty() && name.bytes().all(|b| b.is_ascii_digit()) { + if name.starts_with("https://") { + prs(state) + .iter() + .rposition(|pr| pr.at("url").str() == name) + .unwrap_or_else(|| no_pr_for(name)) + } else if !name.is_empty() && name.bytes().all(|b| b.is_ascii_digit()) { prs(state) .iter() .rposition(|pr| pr.at("number").as_i64() == name.parse().ok()) @@ -448,6 +455,8 @@ fn pr_fields(pr: &Json, wanted: &[String]) -> Json { const PR_LINE_OF_NEWEST: &str = r#".[0] // empty | "\(.state) \(.url) \(.headRefOid) \(.mergeCommit.oid // "")""#; +const PR_LINE_OF_NEWEST_WITH_OWNERSHIP: &str = r#".[0] // empty | "\(.isCrossRepository) \(.state) \(.url) \(.headRefOid) \(.mergeCommit.oid // "")""#; + const WORKFLOW_RUN_LINES: &str = r#".workflow_runs[] | "\(.status) \(.conclusion) \(.name)""#; const COMMIT_PR_LINES: &str = r#".[] | "\(.merge_commit_sha) \(.number)""#; @@ -460,7 +469,7 @@ const FAILED_JOB_NAMES: &str = r#"[.jobs[] | select(.conclusion != "success" and /// Newest first. `--search head:` matches head branches starting with /// , as GitHub's search does; `--head ` matches that branch -/// exactly. `--jq` is supported only as `PR_LINE_OF_NEWEST`. +/// exactly. `--jq` supports the newest-PR line with or without its ownership. fn pr_list(state: &Json, flags: &Flags) { let wanted_state = flag(flags, "state").unwrap_or("open").to_uppercase(); let search = flag(flags, "search").unwrap_or(""); @@ -484,7 +493,7 @@ fn pr_list(state: &Json, flags: &Flags) { .collect(); match flag(flags, "jq") { None => println!("{}", Array(listed)), - Some(PR_LINE_OF_NEWEST) => { + Some(jq @ (PR_LINE_OF_NEWEST | PR_LINE_OF_NEWEST_WITH_OWNERSHIP)) => { if let Some(pr) = listed.first() { let merge = match pr.at("mergeCommit") { commit if commit.truthy() => { @@ -492,8 +501,12 @@ fn pr_list(state: &Json, flags: &Flags) { } _ => String::new(), }; + let ownership = match jq { + PR_LINE_OF_NEWEST_WITH_OWNERSHIP => format!("{} ", pr.at("isCrossRepository")), + _ => String::new(), + }; let line = format!( - "{} {} {} {merge}", + "{ownership}{} {} {} {merge}", pr.at("state").python(), pr.at("url").python(), pr.at("headRefOid").python() diff --git a/tests/release.rs b/tests/release.rs index 3b09e2866..3aee490c7 100644 --- a/tests/release.rs +++ b/tests/release.rs @@ -62,6 +62,14 @@ fn with_version(file: &str, version: &str) -> String { format!("{}{version}{}", &text[..start], &text[end..]) } +#[derive(Clone, Copy, Debug, PartialEq)] +enum BumpChange { + AppendComment(&'static str), + AddedFile, + FileMode, + WrongVersion, +} + /// A bare origin whose main is at version 0.1.0, tagged `v0.1.0`, with one PR /// merged before the tag and two after, and a green CI run on main's tip; a /// contributor's clone that makes them; the maintainer's clone the script @@ -277,6 +285,20 @@ impl Release { .clone() } + /// Calls that attempted to merge a PR, oldest first. + fn merge_calls(&self) -> Vec> { + let path = self.root().join("gh-calls.json"); + if !path.exists() { + return Vec::new(); + } + let calls: Vec> = + serde_json::from_str(&fs::read_to_string(path).unwrap()).unwrap(); + calls + .into_iter() + .filter(|call| call.starts_with(&["pr".into(), "merge".into()])) + .collect() + } + fn command(&self, program: &Path, dir: &Path) -> Command { let path = format!( "{}:{}", @@ -325,6 +347,19 @@ impl Release { ) } + /// Sources the script in the maintainer's clone, with what `run_script` + /// runs it with, then runs the bash `code`, which sets what the earlier + /// steps would have and calls its functions. + fn with_functions(&self, code: &str) -> Output { + self.command(Path::new("bash"), &self.maintainer()) + .arg("-c") + .arg(format!("source \"$1\"\n{code}")) + .arg("bash") + .arg(manifest_dir().join("scripts/release.sh")) + .output() + .expect("could not run bash") + } + /// Runs the Release notes script for `tag` in the maintainer's clone, /// once it has fetched the tags, with `INSTALL_NOTES` as dist's install /// instructions. @@ -408,6 +443,40 @@ impl Release { head } + /// Pushes one more commit to `version`'s pushed bump branch, making + /// `change`, and returns that commit. + fn push_change_to_bump(&self, version: &str, change: BumpChange) -> String { + let contributor = self.contributor(); + let branch = format!("release-{version}"); + git(&contributor, &["checkout", "-q", &branch]); + match change { + BumpChange::AddedFile => fs::write(contributor.join("extra.txt"), "extra\n").unwrap(), + BumpChange::FileMode => { + fs::set_permissions( + contributor.join("Cargo.toml"), + fs::Permissions::from_mode(0o755), + ) + .unwrap(); + } + BumpChange::WrongVersion => { + for file in ["Cargo.toml", "Cargo.lock"] { + fs::write(contributor.join(file), with_version(file, "0.3.0")).unwrap(); + } + } + BumpChange::AppendComment(file) => { + let path = contributor.join(file); + let text = fs::read_to_string(&path).unwrap(); + fs::write(&path, format!("{text}\n# Unrelated change\n")).unwrap(); + } + } + git(&contributor, &["add", "-A"]); + git(&contributor, &["commit", "-q", "-m", "Unrelated change"]); + git(&contributor, &["push", "-q", "origin", &branch]); + let head = git(&contributor, &["rev-parse", "HEAD"]); + git(&contributor, &["checkout", "-q", "--detach", "origin/main"]); + head + } + /// Opens the bump PR for `version`'s pushed branch. fn open_bump_pr(&self, version: &str) { self.gh(&[ @@ -527,6 +596,11 @@ fn release_url() -> String { format!("https://github.com/{REPO}/releases/tag/v0.2.0") } +/// Sets what `main` would for 0.2.0 and finds the earlier run, as `main` +/// does before anything else. +const FIND_EARLIER_RUN: &str = + "version=0.2.0 tag=v0.2.0 branch=release-0.2.0\nfetch_main\nfind_earlier_run"; + const GREEN: &str = r#"[{"name": "test", "conclusion": "success", "pending_polls": 2}]"#; const RED: &str = r#"[{"name": "test", "conclusion": "failure"}]"#; const AGENT_SUMMARY: &str = "The headline is the frobnicator (#2)."; @@ -1386,19 +1460,31 @@ fn when_the_bump_pr_cannot_be_read_the_release_body_is_the_generated_notes_with_ /// Runs the script with `version` and asserts it refused before pushing /// anything, with a message on stderr containing each of `reasons`. fn assert_refused(release: &Release, version: &str, reasons: &[&str]) { + assert_refused_by(release, version, reasons, || release.run_script(version)); +} + +/// Runs `run`, the script or some of its functions, and asserts it refused +/// the `case` before pushing anything, with a message on stderr containing +/// each of `reasons`. +fn assert_refused_by( + release: &Release, + case: &str, + reasons: &[&str], + run: impl FnOnce() -> Output, +) { let refs_before = release.origin_refs(); let prs_before = release.gh_state()["prs"].clone(); - let output = release.run_script(version); + let output = run(); - assert!(!output.status.success(), "{version} was not refused"); + assert_eq!(output.status.code(), Some(1), "{case} was not refused"); let err = stderr(&output); - assert!(err.contains("refusing to release"), "{version}: {err}"); + assert!(err.contains("refusing to release"), "{case}: {err}"); for reason in reasons { - assert!(err.contains(reason), "{version}: no {reason:?} in {err}"); + assert!(err.contains(reason), "{case}: no {reason:?} in {err}"); } - assert_eq!(release.origin_refs(), refs_before, "{version}"); - assert_eq!(release.gh_state()["prs"], prs_before, "{version}"); + assert_eq!(release.origin_refs(), refs_before, "{case}"); + assert_eq!(release.gh_state()["prs"], prs_before, "{case}"); } #[test] @@ -1560,6 +1646,104 @@ fn a_rerun_after_the_branch_was_pushed_opens_its_pr_then_merges_and_tags() { assert_eq!(release.origin_tag("v0.2.0"), Some(merge)); } +#[test] +#[cfg(target_os = "linux")] +fn a_resumed_bump_pr_from_another_repository_is_refused() { + // The open PR end to end; finding the earlier run takes the rest. + for (merged, tagged) in [(false, false), (true, false), (true, true)] { + let release = Release::new(); + let head = release.push_bump("0.2.0"); + release.open_bump_pr("0.2.0"); + if merged { + let merge = release.merge_bump_pr("0.2.0", &head); + if tagged { + release.origin(&["tag", "v0.2.0", &merge]); + } + } + release.gh(&["fake", "pr", "release-0.2.0", "isCrossRepository", "true"]); + release.ci_reports(GREEN); + let url = release.pr("release-0.2.0")["url"] + .as_str() + .unwrap() + .to_owned(); + let reasons = [url.as_str(), "another repository"]; + + if merged { + assert_refused_by( + &release, + &format!("merged {merged}, tagged {tagged}"), + &reasons, + || release.with_functions(FIND_EARLIER_RUN), + ); + } else { + assert_refused(&release, "0.2.0", &reasons); + } + } +} + +#[test] +#[cfg(target_os = "linux")] +fn a_resumed_bump_pr_with_changes_beyond_the_version_bump_is_refused() { + // One open PR end to end; checking the bump PR takes the rest. + for merged in [false, true] { + for change in [ + BumpChange::AppendComment("README.md"), + BumpChange::AppendComment("Cargo.toml"), + BumpChange::AppendComment("Cargo.lock"), + BumpChange::AddedFile, + BumpChange::FileMode, + BumpChange::WrongVersion, + ] { + let release = Release::new(); + release.push_bump("0.2.0"); + let head = release.push_change_to_bump("0.2.0", change); + release.open_bump_pr("0.2.0"); + if merged { + release.merge_bump_pr("0.2.0", &head); + } + release.ci_reports(GREEN); + let url = release.pr("release-0.2.0")["url"] + .as_str() + .unwrap() + .to_owned(); + let reasons = [url.as_str(), "version bump"]; + + if !merged && change == BumpChange::AppendComment("README.md") { + assert_refused(&release, "0.2.0", &reasons); + } else { + assert_refused_by( + &release, + &format!("merged {merged}, {change:?}"), + &reasons, + || release.with_functions(&format!("{FIND_EARLIER_RUN}\nvalidate_bump_pr")), + ); + } + } + } +} + +#[test] +#[cfg(target_os = "linux")] +fn a_merged_bump_pr_whose_head_is_not_on_origin_is_refused() { + let release = Release::new(); + let head = release.push_bump("0.2.0"); + release.open_bump_pr("0.2.0"); + release.merge_bump_pr("0.2.0", &head); + let missing = json!("0".repeat(40)).to_string(); + release.gh(&["fake", "pr", "release-0.2.0", "headRefOid", &missing]); + let url = release.pr("release-0.2.0")["url"] + .as_str() + .unwrap() + .to_owned(); + + assert_refused_by( + &release, + "missing head", + &[&url, "could not be fetched"], + || release.with_functions(&format!("{FIND_EARLIER_RUN}\nvalidate_bump_pr")), + ); +} + #[test] #[cfg(target_os = "linux")] fn a_rerun_after_the_pr_was_opened_merges_and_tags_without_a_second_pr_or_bump() { @@ -1567,6 +1751,8 @@ fn a_rerun_after_the_pr_was_opened_merges_and_tags_without_a_second_pr_or_bump() let head = release.push_bump("0.2.0"); release.open_bump_pr("0.2.0"); release.ci_reports(GREEN); + // Branch lookup is ambiguous; only the selected PR's URL may be merged. + release.gh(&["fake", "fails", "pr merge release-0.2.0"]); let output = release.run_script("0.2.0"); @@ -1584,6 +1770,8 @@ fn a_rerun_after_the_pr_was_opened_merges_and_tags_without_a_second_pr_or_bump() err.contains("release: waiting for CI on https://github.com/"), "{err}" ); + let merges = release.merge_calls(); + assert_eq!(merges.last().unwrap()[2], pr["url"].as_str().unwrap()); } #[test] @@ -1772,6 +1960,32 @@ fn a_tag_with_no_bump_pr_is_refused() { assert_eq!(release.prs_from("release-0.2.0").len(), 0); } +#[test] +#[cfg(target_os = "linux")] +fn an_open_bump_pr_with_a_different_head_from_origin_is_refused() { + let release = Release::new(); + release.push_bump("0.2.0"); + release.open_bump_pr("0.2.0"); + // A stale PR head is still a real, fetchable commit, but not the branch tip. + let stale = release.origin(&["rev-parse", "main"]); + release.gh(&[ + "fake", + "pr", + "release-0.2.0", + "headRefOid", + &json!(stale).to_string(), + ]); + release.ci_reports(GREEN); + let url = release.pr("release-0.2.0")["url"] + .as_str() + .unwrap() + .to_owned(); + let merges_before = release.merge_calls(); + + assert_refused(&release, "0.2.0", &[&url, "head does not match", "origin"]); + assert_eq!(release.merge_calls(), merges_before); +} + #[test] #[cfg(target_os = "linux")] fn a_bump_pr_closed_without_merging_is_refused() { @@ -1793,25 +2007,35 @@ fn a_bump_pr_closed_without_merging_is_refused() { #[test] #[cfg(target_os = "linux")] fn a_bump_pr_closed_without_merging_and_its_branch_deleted_starts_over() { - let release = Release::new(); - let stale = release.push_bump("0.2.0"); - release.open_bump_pr("0.2.0"); - release.gh(&["fake", "pr", "release-0.2.0", "state", "\"CLOSED\""]); - // GitHub keeps a closed PR's head once its branch is deleted. - let head = format!("\"{stale}\""); - release.gh(&["fake", "pr", "release-0.2.0", "headRefOid", &head]); - release.origin(&["branch", "-D", "release-0.2.0"]); - release.ci_reports(GREEN); + // Even a closed PR from another repository leaves nothing to resume. + for cross_repository in ["false", "true"] { + let release = Release::new(); + let stale = release.push_bump("0.2.0"); + release.open_bump_pr("0.2.0"); + release.gh(&["fake", "pr", "release-0.2.0", "state", "\"CLOSED\""]); + release.gh(&[ + "fake", + "pr", + "release-0.2.0", + "isCrossRepository", + cross_repository, + ]); + // GitHub keeps a closed PR's head once its branch is deleted. + let head = format!("\"{stale}\""); + release.gh(&["fake", "pr", "release-0.2.0", "headRefOid", &head]); + release.origin(&["branch", "-D", "release-0.2.0"]); + release.ci_reports(GREEN); - let output = release.run_script("0.2.0"); + let output = release.run_script("0.2.0"); - assert!(output.status.success(), "{}", stderr(&output)); - let prs = release.prs_from("release-0.2.0"); - assert_eq!(prs.len(), 2); - assert_eq!(prs[0]["state"], "CLOSED"); - assert_eq!(prs[1]["state"], "MERGED"); - let merge = release.origin(&["rev-parse", "main"]); - assert_eq!(release.origin_tag("v0.2.0"), Some(merge)); + assert!(output.status.success(), "{}", stderr(&output)); + let prs = release.prs_from("release-0.2.0"); + assert_eq!(prs.len(), 2); + assert_eq!(prs[0]["state"], "CLOSED"); + assert_eq!(prs[1]["state"], "MERGED"); + let merge = release.origin(&["rev-parse", "main"]); + assert_eq!(release.origin_tag("v0.2.0"), Some(merge)); + } } #[test] @@ -1833,3 +2057,38 @@ fn review_no_on_a_rerun_after_the_branch_was_pushed_stops_with_no_pr_opened() { assert!(!release.pr_opened("release-0.2.0")); assert_eq!(release.origin_refs(), refs_before); } + +#[test] +#[cfg(target_os = "linux")] +fn review_resumed_branch_without_pr_with_extra_changes_is_refused() { + let release = Release::new(); + release.push_bump("0.2.0"); + release.push_change_to_bump("0.2.0", BumpChange::AddedFile); + release.ci_reports(GREEN); + let refs_before = release.origin_refs(); + let merges_before = release.merge_calls(); + + let output = release.run_script("0.2.0"); + + assert_eq!(output.status.code(), Some(1), "{}", stderr(&output)); + assert!(stderr(&output).contains("refusing to release")); + assert_eq!(release.origin_refs(), refs_before); + assert_eq!(release.merge_calls(), merges_before); +} + +#[test] +#[cfg(target_os = "linux")] +fn review_tagged_resumed_pr_with_extra_changes_is_refused() { + let release = Release::new(); + release.push_bump("0.2.0"); + let head = release.push_change_to_bump("0.2.0", BumpChange::AddedFile); + release.open_bump_pr("0.2.0"); + let merge = release.merge_bump_pr("0.2.0", &head); + release.origin(&["tag", "v0.2.0", &merge]); + let url = release.pr("release-0.2.0")["url"] + .as_str() + .unwrap() + .to_owned(); + + assert_refused(&release, "0.2.0", &[&url, "version bump"]); +}