Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bootstrap: handle worktrees in warn_old_master_branch #130121

Merged
merged 1 commit into from
Sep 14, 2024
Merged

bootstrap: handle worktrees in warn_old_master_branch #130121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions src/bootstrap/src/core/sanity.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,13 +378,5 @@ $ pacman -R cmake && pacman -S mingw-w64-x86_64-cmake
cmd_finder.must_have(s);
}

// this warning is useless in CI,
// and CI probably won't have the right branches anyway.
if !build_helper::ci::CiEnv::is_ci() {
if let Err(e) = warn_old_master_branch(&build.config.git_config(), &build.config.src)
.map_err(|e| e.to_string())
{
eprintln!("unable to check if upstream branch is old: {e}");
}
}
warn_old_master_branch(&build.config.git_config(), &build.config.src);
}
62 changes: 46 additions & 16 deletions src/tools/build_helper/src/git.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,29 +167,59 @@ pub fn get_git_untracked_files(
///
/// This can result in formatting thousands of files instead of a dozen,
/// so we should warn the user something is wrong.
pub fn warn_old_master_branch(
config: &GitConfig<'_>,
git_dir: &Path,
) -> Result<(), Box<dyn std::error::Error>> {
use std::time::Duration;
const WARN_AFTER: Duration = Duration::from_secs(60 * 60 * 24 * 10);
let updated_master = updated_master_branch(config, Some(git_dir))?;
let branch_path = git_dir.join(".git/refs/remotes").join(&updated_master);
match std::fs::metadata(branch_path) {
Ok(meta) => {
if meta.modified()?.elapsed()? > WARN_AFTER {
eprintln!("warning: {updated_master} has not been updated in 10 days");
} else {
return Ok(());
pub fn warn_old_master_branch(config: &GitConfig<'_>, git_dir: &Path) {
if crate::ci::CiEnv::is_ci() {
// this warning is useless in CI,
// and CI probably won't have the right branches anyway.
return;
}
// this will be overwritten by the actual name, if possible
let mut updated_master = "the upstream master branch".to_string();
match warn_old_master_branch_(config, git_dir, &mut updated_master) {
Ok(branch_is_old) => {
if !branch_is_old {
return;
}
// otherwise fall through and print the rest of the warning
}
Err(err) => {
eprintln!("warning: unable to check if {updated_master} is old due to error: {err}")
}
}
eprintln!(
"warning: {updated_master} is used to determine if files have been modified\n\
warning: if it is not updated, this may cause files to be needlessly reformatted"
warning: if it is not updated, this may cause files to be needlessly reformatted"
);
Ok(())
}

pub fn warn_old_master_branch_(
config: &GitConfig<'_>,
git_dir: &Path,
updated_master: &mut String,
) -> Result<bool, Box<dyn std::error::Error>> {
use std::time::Duration;
*updated_master = updated_master_branch(config, Some(git_dir))?;
let branch_path = git_dir.join(".git/refs/remotes").join(&updated_master);
Copy link
Member

@RalfJung RalfJung Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there a way to avoid directly looking at git's data format? Breaking the abstraction like this should ideally be avoided. E.g. you could look at the date of the last commit in that branch.

If you must use the .git folder, please use git rev-parse --git-dir to determine its location, rather than assuming that it is .git. That will then also work with worktrees.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see you already found --git-common-dir... which is indeed the right location, --git-dir is wrong. Let me quickly fix some faulty logic I recently wrote elsewhere, then... ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i suppose i overestimated:

  1. how many people use worktrees
  2. how much people dislike incorrect warnings

waiting on a second opinion, i'll rewrite the code if that's the consensus preference.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The incorrect warning already led to incorrect diagnosis of an unrelated issue in #130144. So it's not just about dislike, it's about the real costs of an incorrect warning.

Copy link
Contributor Author

@lolbinarycat lolbinarycat Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i see that pretty clearly now, there's a reason i started writing this PR as soon as i was made aware of the issue.

i know the current PR works, and my main worry is that if i need to totally rewrite it, that might introduce even more bugs (this situation has happened to me many times), and this would be even worse with something that is so tedious for me to test (spinning up worktrees is not fast on a hdd system)

the worktree bug seems pretty severe, so i think we should prioritize getting a fix merged.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i started writing this PR as soon as i was made aware of the issue.

Thanks for spinning up a fix so quickly!

const WARN_AFTER: Duration = Duration::from_secs(60 * 60 * 24 * 10);
let meta = match std::fs::metadata(&branch_path) {
Ok(meta) => meta,
Err(err) => {
let gcd = git_common_dir(&git_dir)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use this only as a fallback instead of an error? There's a single unique way to find this path correctly for all situations, that should IMO be used consistently.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm trying not to have much of a performance impact over such a niche lint. i'm stuck on an hdd system for the time being, so git can take almost half a second to query basic information if it isn't in the I/O cache.. compared to a single stat syscall, which is always nearly instant.

yes, this does give slightly worse performance in the worktree case, but worktrees are already nearly unusable on a slow hdd system, so i'm not too concerned about that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned about maintainability of this codebase and how much we're willing to carry difficult-to-test fallback paths in code that is proven to be error-prone and not covered by CI. There's a global cost to every hack like this, and at some point making a slow system a bit slower is an okay way to keep the rest of the project productive -- the time spent figuring out why my worktrees now show this warning is time not spent working on rustc itself, after all.

In the end this will be up to the bootstrap team to decide though, these are just my thoughts.

if branch_path.starts_with(&gcd) {
return Err(Box::new(err));
}
std::fs::metadata(Path::new(&gcd).join("refs/remotes").join(&updated_master))?
}
};
if meta.modified()?.elapsed()? > WARN_AFTER {
eprintln!("warning: {updated_master} has not been updated in 10 days");
Ok(true)
} else {
Ok(false)
}
}

fn git_common_dir(dir: &Path) -> Result<String, String> {
output_result(Command::new("git").arg("-C").arg(dir).arg("rev-parse").arg("--git-common-dir"))
.map(|x| x.trim().to_string())
}
Loading