Repository: rust-lang/rust
File: src/tools/tidy/src/diagnostics.rs (Rust)

Code before:
        ci_flag: Option<bool>,
        tidy_flags: TidyFlags,
    ) -> Self {
        let ci_env = match ci_flag {
            Some(true) => CiEnv::GitHubActions,
            Some(false) => CiEnv::None,
            None => CiEnv::None,
        };

        let mut tidy_ctx = Self {
            diag_ctx: Arc::new(Mutex::new(DiagCtxInner {
                running_checks: Default::default(),
                finished_checks: Default::default(),
                root_path: root_path.to_path_buf(),
                verbose,
            })),
            tidy_flags,
            ci_env,
            base_commit: None,
        };
        tidy_ctx.base_commit();

        tidy_ctx
    }

    fn base_commit(&mut self) {
        let mut check = self.start_check("CI history");

        let stage0 = parse_stage0_file();
        let Stage0Config { nightly_branch, git_merge_commit_email, .. } = stage0.config;

        let base_commit = match get_closest_upstream_commit(
            None,
            &GitConfig {
                nightly_branch: &nightly_branch,
                git_merge_commit_email: &git_merge_commit_email,
            },
            self.ci_env,
        ) {
            Ok(Some(commit)) => Some(commit),
            Ok(None) => {
                self.error_if_in_ci("no base commit found", &mut check);
                None
            }
            Err(error) => {
                self.error_if_in_ci(
                    &format!("failed to retrieve base commit: {error}"),
                    &mut check,
                );
                None
            }

Review comment (refactor):
This method does not return anything and modifies its inner state, while it is expected that it is called on the context before it is properly initialized. That's not great. This is the fallout from removing `CiInfo`, and thus I wonder if removing it is worth it, if we then have to write a function like this, which initializes the state of the context while already using the partially initialized context to report errors.

I also don't think it's worth doing a larger refactoring to untangle this mess at the moment though, the functionality is quite entangled, so it's not easy.

At least please rename the method to something like `find_base_commit`, and make it a normal function in this module that will receive `TidyCtx` as an argument, and return the base commit (`Option<String>`).

Code after:
        ci_flag: Option<bool>,
        tidy_flags: TidyFlags,
    ) -> Self {
        let ci_env = match ci_flag {
            Some(true) => CiEnv::GitHubActions,
            Some(false) => CiEnv::None,
            None => CiEnv::current(),
        };

        let mut tidy_ctx = Self {
            diag_ctx: Arc::new(Mutex::new(DiagCtxInner {
                running_checks: Default::default(),
                finished_checks: Default::default(),
                root_path: root_path.to_path_buf(),
                verbose,
            })),
            tidy_flags,
            ci_env,
            base_commit: None,
        };
        tidy_ctx.base_commit = find_base_commit(&tidy_ctx);

        tidy_ctx
    }

    pub fn is_bless_enabled(&self) -> bool {
        self.tidy_flags.bless
    }

    pub fn is_running_on_ci(&self) -> bool {
        self.ci_env.is_running_in_ci()
    }

    pub fn start_check<Id: Into<CheckId>>(&self, id: Id) -> RunningCheck {
        let mut id = id.into();

        let mut ctx = self.diag_ctx.lock().unwrap();

        // Shorten path for shorter diagnostics
        id.path = match id.path {
            Some(path) => Some(path.strip_prefix(&ctx.root_path).unwrap_or(&path).to_path_buf()),
            None => None,
        };

        ctx.start_check(id.clone());
        RunningCheck {
            id,
            bad: false,
            ctx: self.diag_ctx.clone(),
            #[cfg(test)]
            errors: vec![],