diff --git a/ghostflow-gitlab/src/lib.rs b/ghostflow-gitlab/src/lib.rs index 8a0b9c61bc383eec338d86b53590fa24580b03b8..bd911d79ce75d9523cbbd8a7fa83b4bac5a58c3c 100644 --- a/ghostflow-gitlab/src/lib.rs +++ b/ghostflow-gitlab/src/lib.rs @@ -581,6 +581,11 @@ impl From for HostingServiceError { } impl HostingService for GitlabService { + fn suppress_ci_push_option(&self, branch: &str) -> Option { + // TODO: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/170960 + Some(format!("branch.{}.ci.skip", branch)) + } + fn as_pipeline_service(self: Arc) -> Option> { Some(self as Arc) } diff --git a/ghostflow/src/actions/merge/backport.rs b/ghostflow/src/actions/merge/backport.rs index b1e2b5034738a33400f15ee4a9a7eae56214b7ec..feddcdf939d7f6de6c52a0e87d27280e7f2adf98 100644 --- a/ghostflow/src/actions/merge/backport.rs +++ b/ghostflow/src/actions/merge/backport.rs @@ -170,8 +170,8 @@ impl MergeMany { let refs_status = refs.iter().map(|(&branch, &(ref commit, settings))| { (branch.into(), commit.clone(), settings.into_branches()) }); - let push_refs = merger.perform_update_merges(sorter, refs_status, &info, renamer)?; + let push_info = merger.perform_update_merges(sorter, refs_status, &info, renamer)?; - merger.push_refs(quiet, push_refs) + merger.push_refs(quiet, push_info.options, push_info.ref_updates) } } diff --git a/ghostflow/src/actions/merge/settings.rs b/ghostflow/src/actions/merge/settings.rs index 94c871c7657a710f8db825faf0eb7be2920c5e7e..3bd3d962080c433a213bb477e5f3307e6bb80457 100644 --- a/ghostflow/src/actions/merge/settings.rs +++ b/ghostflow/src/actions/merge/settings.rs @@ -4,7 +4,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -use std::collections::HashMap; +use std::collections::{BTreeSet, HashMap}; use std::iter; use chrono::{DateTime, Utc}; @@ -274,6 +274,14 @@ pub struct Merger<'a> { /// Alias used for nested results involved in merging a merge request. type StepResult = MergeResult>; +/// Information required to push updates for a merge. +pub struct PushInformation { + /// Push options to pass along. + pub options: Vec, + /// A set of commits to push to named refs. + pub ref_updates: Vec<(CommitId, String)>, +} + impl<'a> Merger<'a> { /// Create a new merger object. pub fn new( @@ -314,8 +322,8 @@ impl<'a> Merger<'a> { renamer.insert(settings.branch.clone(), settings.merge_name()); settings.add_topo_links(&mut sorter); let refs = iter::once((settings.branch.clone(), commit_id, settings.into_branches())); - let push_refs = self.perform_update_merges(sorter, refs, &info, renamer)?; - self.push_refs(settings.quiet, push_refs) + let push_info = self.perform_update_merges(sorter, refs, &info, renamer)?; + self.push_refs(settings.quiet, push_info.options, push_info.ref_updates) } /// Prepare to merge a merge request. @@ -350,21 +358,34 @@ impl<'a> Merger<'a> { /// Returns a vector of commits which need to be pushed to the given branches on the remote. pub fn perform_update_merges<'b, I>( &self, - mut sorter: TopologicalSort, + sorter: TopologicalSort, refs: I, info: &MergeInformation<'b>, renamer: HashMap, - ) -> MergeResult> + ) -> MergeResult where I: IntoIterator, { - // A map of branch -> Vec for knowing which branches to merge into this branch. - let mut from_branches: HashMap<_, Vec<_>> = HashMap::new(); // A map of branch -> (commit, [IntoBranch]) to know where branches should go. - let mut push_refs = refs + let refs = refs .into_iter() .map(|(branch, commit, into_branches)| (branch, (commit, into_branches))) - .collect::>(); + .collect(); + self.perform_update_merges_impl(sorter, refs, info, renamer) + } + + /// Monomorphized variant of `perform_update_merges`. + fn perform_update_merges_impl<'b>( + &self, + mut sorter: TopologicalSort, + mut push_refs: HashMap, + info: &MergeInformation<'b>, + renamer: HashMap, + ) -> MergeResult { + // A map of branch -> Vec for knowing which branches to merge into this branch. + let mut from_branches: HashMap<_, Vec<_>> = HashMap::new(); + // A set of branches which have only received update merges. + let mut only_update_merges = BTreeSet::new(); // Look at a free branch. while let Some(target_branch) = sorter.pop() { @@ -478,10 +499,18 @@ impl<'a> Merger<'a> { .entry(target_into.name()) .or_insert_with(Vec::new) .push(target_branch.clone()); - push_refs.entry(target_into.name().into()).or_insert(( - CommitId::new(target_into.name()), - target_into.chain_branches(), - )); + push_refs + .entry(target_into.name().into()) + .or_insert_with(|| { + // We did not have a reason to update this branch prior to this, so it is + // only updated via update merges, so track it. + only_update_merges.insert(target_into.name()); + + ( + CommitId::new(target_into.name()), + target_into.chain_branches(), + ) + }); }); } @@ -498,10 +527,18 @@ impl<'a> Merger<'a> { ); } - Ok(push_refs + let options = only_update_merges .into_iter() - .map(|(branch, (commit, _))| (commit, branch)) - .collect()) + .filter_map(|branch| self.project.service.suppress_ci_push_option(branch)) + .collect(); + + Ok(PushInformation { + options, + ref_updates: push_refs + .into_iter() + .map(|(branch, (commit, _))| (commit, branch)) + .collect(), + }) } /// Create a merge commit for the merge request into the branch. @@ -612,24 +649,47 @@ impl<'a> Merger<'a> { Ok(Left(merge_command.commit(commit_message)?)) } - /// Push the results of a merge action to the remote repository. - pub fn push_refs(&self, quiet: bool, refs: R) -> MergeResult + /// Push the results of a merge action to the remote repository including options. + pub fn push_refs( + &self, + quiet: bool, + options: O, + refs: R, + ) -> MergeResult where + O: IntoIterator, + OV: AsRef, R: IntoIterator, B: AsRef, { + self.push_refs_impl( + quiet, + options + .into_iter() + .map(|option| format!("--push-option={}", option.as_ref())) + .collect(), + refs.into_iter() + .map(|(commit_id, branch)| format!("{}:{}", commit_id, branch.as_ref())) + .collect(), + ) + } + + /// Monomorphized variant of `push_refs`. + fn push_refs_impl( + &self, + quiet: bool, + option_args: Vec, + refs: Vec, + ) -> MergeResult { let push = self .ctx .git() .arg("push") .arg("--atomic") .arg("--porcelain") + .args(option_args) .arg("origin") - .args( - refs.into_iter() - .map(|(commit_id, branch)| format!("{}:{}", commit_id, branch.as_ref())) - .collect::>(), - ) + .args(refs) .output() .map_err(|err| GitError::subcommand("push", err))?; if !push.status.success() { diff --git a/ghostflow/src/host/traits.rs b/ghostflow/src/host/traits.rs index 24f7b71dc92dfeea26ed159ff86220ffea9ef75a..a08c90ddc2d9078bd777a420b2057088f23cd5a9 100644 --- a/ghostflow/src/host/traits.rs +++ b/ghostflow/src/host/traits.rs @@ -146,6 +146,13 @@ pub trait HostingService: Send + Sync { .map_err(HostingServiceError::fetch) } + /// Create a push option to suppress the CI for a branch when pushing. + fn suppress_ci_push_option(&self, branch: &str) -> Option { + // https://github.com/rust-lang/rust/issues/91074 + let _unused_args_for_rustdoc = branch; + None + } + /// Get more specific hosting service access. fn as_pipeline_service(self: Arc) -> Option> { None diff --git a/ghostflow/src/tests/merge/behavior.rs b/ghostflow/src/tests/merge/behavior.rs index 287753536418a639e17ad1fa61d055ca0e8961a0..94340f119fca9535d6dd97baf568c3ea9b719e7a 100644 --- a/ghostflow/src/tests/merge/behavior.rs +++ b/ghostflow/src/tests/merge/behavior.rs @@ -13,7 +13,7 @@ use crate::actions::merge::*; use crate::host::HostingService; use crate::tests::merge::utils; use crate::tests::mock::{MockData, MockMergeRequest, MockService}; -use crate::tests::utils::test_workspace_dir; +use crate::tests::utils::{install_hook, test_workspace_dir}; const REPO_NAME: &str = "base"; const MR_ID: u64 = 1; @@ -1089,3 +1089,67 @@ fn test_merge_ff_skip_sync_already_merged() { utils::check_mr_commit_ff(&origin_ctx, BRANCH_NAME, &mr.commit.id); utils::check_mr_commit_ff(&origin_ctx, INTO_BRANCH_NAME, &CommitId::new(BASE_WITH_FF)); } + +// base -> into, but with push options +#[test] +fn test_merge_simple_into_push_options() { + let tempdir = test_workspace_dir(); + let (origin_ctx, ctx) = utils::git_context(tempdir.path(), BASE); + + install_hook("pre-receive", "log-options", &origin_ctx); + + let mut service = merge_service(); + Arc::get_mut(&mut service) + .map(|mock| mock.support_ci_skip_option()) + .unwrap(); + let mut settings = MergeSettings::new(BRANCH_NAME, utils::TestMergePolicy::default()); + + settings.add_into_branches([IntoBranch::new(INTO_BRANCH_NAME)]); + + let merge = utils::create_merge_settings(&ctx, &service, settings); + + let mr = service.merge_request(REPO_NAME, MR_ID).unwrap(); + let res = merge + .merge_mr(&mr, utils::merger_ident(), utils::author_date()) + .unwrap(); + assert_eq!(res, MergeActionResult::Success); + + let mr_comments = service.mr_comments(mr.id); + + assert_eq!(service.remaining_data(), 0); + + assert_eq!(mr_comments.len(), 1); + assert_eq!(mr_comments[0], "Topic successfully merged and pushed."); + + utils::check_mr_commit( + &origin_ctx, + BRANCH_NAME, + "Merge topic 'topic-1' into test-merge\n\ + \n\ + 7189cf5 topic-1: make a change\n\ + \n\ + Acked-by: Ghostflow \n\ + Merge-request: !1\n", + ); + utils::check_mr_commit( + &origin_ctx, + INTO_BRANCH_NAME, + "Merge branch 'test-merge' into test-merge-into", + ); + utils::check_noop_merge(&origin_ctx, INTO_BRANCH_NAME); + + let options = origin_ctx + .git() + .arg("config") + // XXX(git-2.46.0): use `git config get --all` + // .arg("get") + .arg("--get-all") + .arg("-f") + .arg(origin_ctx.gitdir().join("push-option-log")) + // .arg("--all") + .arg("ghostflow.push") + .output() + .unwrap(); + let values = String::from_utf8_lossy(&options.stdout); + itertools::assert_equal(values.trim().lines(), ["branch.test-merge-into.ci.skip"]); +} diff --git a/ghostflow/src/tests/mock/service.rs b/ghostflow/src/tests/mock/service.rs index d150b725996af02af4a266290ce27fd25831fced..a97f3fe836319a38d1cb2e3133c204f07280122e 100644 --- a/ghostflow/src/tests/mock/service.rs +++ b/ghostflow/src/tests/mock/service.rs @@ -45,6 +45,7 @@ pub struct MockService { data: Mutex, user: User, state: Mutex, + support_ci_skip_option: bool, } impl MockService { @@ -53,9 +54,14 @@ impl MockService { data: Mutex::new(data), user: admin, state: Mutex::new(State::default()), + support_ci_skip_option: false, } } + pub fn support_ci_skip_option(&mut self) { + self.support_ci_skip_option = true; + } + pub fn modify(&self) -> MockDataModifier { let data = self.data.lock().expect(LOCK_POISONED); MockDataModifier { @@ -248,6 +254,14 @@ fn commit_status_from_pending(status: &PendingCommitStatus, author: User) -> Com } impl HostingService for MockService { + fn suppress_ci_push_option(&self, branch: &str) -> Option { + if self.support_ci_skip_option { + Some(format!("branch.{}.ci.skip", branch)) + } else { + None + } + } + fn service_user(&self) -> &User { &self.user } diff --git a/ghostflow/src/tests/utils.rs b/ghostflow/src/tests/utils.rs index cf9488b02285d9978858084299145e842f3ea5e0..d1e4a841888d838a8a07f629469ce06c56d34864 100644 --- a/ghostflow/src/tests/utils.rs +++ b/ghostflow/src/tests/utils.rs @@ -5,6 +5,8 @@ // except according to those terms. use std::env; +use std::fs; +use std::os; use git_workarea::GitContext; use tempfile::TempDir; @@ -46,3 +48,32 @@ fn get_git_config(ctx: &GitContext, name: &str) -> String { String::from_utf8_lossy(&config.stdout).into_owned() } + +pub fn install_hook(hook: &str, name: &str, ctx: &GitContext) { + let config = ctx + .git() + .arg("config") + // XXX(git-2.46.0): use `git config set` + // .arg("set") + // .arg("--type=bool") + .arg("receive.advertisePushOptions") + .arg("true") + .output() + .unwrap(); + if !config.status.success() { + panic!( + "failed to set `receive.advertisePushOptions`: {}", + String::from_utf8_lossy(&config.stderr), + ); + } + + let origin = format!("{}/test/{}-{}", env!("CARGO_MANIFEST_DIR"), hook, name); + let link = ctx.gitdir().join("hooks").join(hook); + + #[cfg(unix)] + if os::unix::fs::symlink(&origin, &link).is_ok() { + return; + } + + fs::copy(origin, link).unwrap(); +} diff --git a/ghostflow/test/pre-receive-log-options b/ghostflow/test/pre-receive-log-options new file mode 100755 index 0000000000000000000000000000000000000000..66b6536926d8541dbffef3d1ecb2154c9d33142c --- /dev/null +++ b/ghostflow/test/pre-receive-log-options @@ -0,0 +1,36 @@ +#!/usr/bin/env python3 + +import os +import subprocess +import sys + +count = os.environ.get('GIT_PUSH_OPTION_COUNT') +if count is None: + sys.exit(0) + +try: + count = int(count) +except ValueError: + sys.stderr.write(f'`GIT_PUSH_OPTION_COUNT` was not an integer: {count}\n') + sys.exit(1) + +def git(*args): + subprocess.check_call(['git'] + args) + +for i in range(count): + push_option = os.environ.get(f'GIT_PUSH_OPTION_{i}') + if push_option is None: + sys.stderr.write(f'`GIT_PUSH_OPTION_{i}` is not set?\n') + sys.exit(1) + + subprocess.check_call([ + 'git', + 'config', + # XXX(git-2.46.0): use `git config set` + # 'set', + '-f', f'{os.environ["GIT_DIR"]}/push-option-log', + # '--append', + '--add', + 'ghostflow.push', + push_option, + ])