Skip to content

Commit

Permalink
git: Ensure no backend ever takes an index lock
Browse files Browse the repository at this point in the history
  • Loading branch information
@bgw
bgw committed Mar 6, 2025
1 parent f88d627 commit 0c96986
Show file tree
Hide file tree
Showing 4 changed files with 152 additions and 41 deletions.
27 changes: 26 additions & 1 deletion test_util/src/repo/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,18 @@ use gix as git;
use rand::{distributions::Alphanumeric, thread_rng, Rng};
use std::{
env,
fs::{self, OpenOptions},
fs::{self, FileTimes, OpenOptions},
io::{BufWriter, Write},
path::PathBuf,
time::{Duration, UNIX_EPOCH},
};
use std::{fs::File, num::NonZeroU32};

const BARE_REPO_PREFIX: &str = "vergen_tmp";
const BARE_REPO_SUFFIX: &str = ".git";
const CLONE_NAME_PREFIX: &str = "vergen_tmp";
const RUNNER_TEMP_ENV: &str = "RUNNER_TEMP";
const MAGIC_MTIME: u64 = 1234567890;

/// Utility to create a temporary bare repository and a repository cloned from the
/// bare repository.
Expand Down Expand Up @@ -319,6 +321,29 @@ impl TestRepos {
let rand_clone_path = format!("{CLONE_NAME_PREFIX}_{}", Self::rand_five());
temp_path.join(rand_clone_path)
}

pub fn set_index_magic_mtime(&self) {
let index_path = self.path().join(".git").join("index");
let magic_mtime = UNIX_EPOCH + Duration::from_secs(MAGIC_MTIME);
File::open(&index_path)
.unwrap()
.set_times(FileTimes::new().set_modified(magic_mtime))
.unwrap();
}

pub fn assert_is_index_magic_mtime(&self) {
let index_path = self.path().join(".git").join("index");
let magic_mtime = UNIX_EPOCH + Duration::from_secs(MAGIC_MTIME);
assert_eq!(
File::open(&index_path)
.unwrap()
.metadata()
.unwrap()
.modified()
.unwrap(),
magic_mtime
);
}
}

impl Drop for TestRepos {
Expand Down
22 changes: 21 additions & 1 deletion vergen-git2/src/git2/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -780,7 +780,7 @@ mod test {
use anyhow::Result;
use git2_rs::Repository;
use serial_test::serial;
use std::{collections::BTreeMap, env::current_dir, io::Write};
use std::{collections::BTreeMap, env::current_dir, io, io::Write};
use test_util::TestRepos;
use vergen::Emitter;
use vergen_lib::{count_idempotent, VergenKey};
Expand Down Expand Up @@ -1063,4 +1063,24 @@ mod test {
assert!(result.is_ok());
});
}

#[test]
#[serial]
fn git_no_index_update() -> Result<()> {
let repo = TestRepos::new(true, true, false)?;
repo.set_index_magic_mtime();

let mut git2 = Git2Builder::default()
.all()
.describe(true, true, None)
.build()?;
let _ = git2.at_path(repo.path());
let failed = Emitter::default()
.add_instructions(&git2)?
.emit_to(&mut io::stdout())?;
assert!(!failed);

repo.assert_is_index_magic_mtime();
Ok(())
}
}
122 changes: 84 additions & 38 deletions vergen-gitcl/src/gitcl/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,8 @@ impl Gitcl {
if let Some(path) = path_opt {
_ = cmd.current_dir(path);
}
// https://git-scm.com/docs/git-status#_background_refresh
_ = cmd.env("GIT_OPTIONAL_LOCKS", "0");
_ = cmd.arg("-c");
_ = cmd.arg(command);
_ = cmd.stdout(Stdio::piped());
Expand All @@ -526,13 +528,25 @@ impl Gitcl {
if let Some(path) = path_opt {
_ = cmd.current_dir(path);
}
// https://git-scm.com/docs/git-status#_background_refresh
_ = cmd.env("GIT_OPTIONAL_LOCKS", "0");
_ = cmd.arg("/c");
_ = cmd.arg(command);
_ = cmd.stdout(Stdio::piped());
_ = cmd.stderr(Stdio::piped());
Ok(cmd.output()?)
}

fn run_cmd_checked(command: &str, path_opt: Option<&PathBuf>) -> Result<Vec<u8>> {
let output = Self::run_cmd(command, path_opt)?;
if output.status.success() {
Ok(output.stdout)
} else {
let stderr = String::from_utf8_lossy(&output.stderr);
Err(anyhow!("Failed to run '{command}'! {stderr}"))
}
}

#[allow(clippy::too_many_lines)]
fn inner_add_git_map_entries(
&self,
Expand Down Expand Up @@ -626,14 +640,32 @@ impl Gitcl {
}
}

let mut dirty_cache = None; // attempt to re-use dirty status later if possible
if self.dirty {
if let Ok(_value) = env::var(GIT_DIRTY_NAME) {
add_default_map_entry(VergenKey::GitDirty, cargo_rustc_env, cargo_warning);
} else {
let dirty = self.compute_dirty(self.dirty_include_untracked)?;
if !self.dirty_include_untracked {
dirty_cache = Some(dirty);
}
add_map_entry(
VergenKey::GitDirty,
bool::to_string(&dirty),
cargo_rustc_env,
);
}
}

if self.describe {
// `git describe --dirty` does not support `GIT_OPTIONAL_LOCKS=0`
// (see https://github.com/gitgitgadget/git/pull/1872)
//
// Instead, always compute the dirty status with `git status`
if let Ok(_value) = env::var(GIT_DESCRIBE_NAME) {
add_default_map_entry(VergenKey::GitDescribe, cargo_rustc_env, cargo_warning);
} else {
let mut describe_cmd = String::from(DESCRIBE);
if self.describe_dirty {
describe_cmd.push_str(" --dirty");
}
if self.describe_tags {
describe_cmd.push_str(" --tags");
}
Expand All @@ -642,12 +674,14 @@ impl Gitcl {
describe_cmd.push_str(pattern);
describe_cmd.push('\"');
}
Self::add_git_cmd_entry(
&describe_cmd,
self.repo_path.as_ref(),
VergenKey::GitDescribe,
cargo_rustc_env,
)?;
let stdout = Self::run_cmd_checked(&describe_cmd, self.repo_path.as_ref())?;
let mut describe_value = String::from_utf8_lossy(&stdout).trim().to_string();
if self.describe_dirty
&& (dirty_cache.is_some_and(|dirty| dirty) || self.compute_dirty(false)?)
{
describe_value.push_str("-dirty");
}
add_map_entry(VergenKey::GitDescribe, describe_value, cargo_rustc_env);
}
}

Expand All @@ -669,23 +703,6 @@ impl Gitcl {
}
}

if self.dirty {
if let Ok(_value) = env::var(GIT_DIRTY_NAME) {
add_default_map_entry(VergenKey::GitDirty, cargo_rustc_env, cargo_warning);
} else {
let mut dirty_cmd = String::from(DIRTY);
if !self.dirty_include_untracked {
dirty_cmd.push_str(" --untracked-files=no");
}
let output = Self::run_cmd(&dirty_cmd, self.repo_path.as_ref())?;
if output.stdout.is_empty() {
add_map_entry(VergenKey::GitDirty, "false", cargo_rustc_env);
} else {
add_map_entry(VergenKey::GitDirty, "true", cargo_rustc_env);
}
}
}

Ok(())
}

Expand Down Expand Up @@ -741,17 +758,12 @@ impl Gitcl {
key: VergenKey,
cargo_rustc_env: &mut CargoRustcEnvMap,
) -> Result<()> {
let output = Self::run_cmd(cmd, path)?;
if output.status.success() {
let stdout = String::from_utf8_lossy(&output.stdout)
.trim()
.trim_matches('\'')
.to_string();
add_map_entry(key, stdout, cargo_rustc_env);
} else {
let stderr = String::from_utf8_lossy(&output.stderr);
return Err(anyhow!("Failed to run '{cmd}'! {stderr}"));
}
let stdout = Self::run_cmd_checked(cmd, path)?;
let stdout = String::from_utf8_lossy(&stdout)
.trim()
.trim_matches('\'')
.to_string();
add_map_entry(key, stdout, cargo_rustc_env);
Ok(())
}

Expand Down Expand Up @@ -857,6 +869,15 @@ impl Gitcl {
Ok((false, no_offset))
}
}

fn compute_dirty(&self, include_untracked: bool) -> Result<bool> {
let mut dirty_cmd = String::from(DIRTY);
if !include_untracked {
dirty_cmd.push_str(" --untracked-files=no");
}
let stdout = Self::run_cmd_checked(&dirty_cmd, self.repo_path.as_ref())?;
Ok(!stdout.is_empty())
}
}

impl AddEntries for Gitcl {
Expand Down Expand Up @@ -960,7 +981,11 @@ mod test {
use crate::Emitter;
use anyhow::Result;
use serial_test::serial;
use std::{collections::BTreeMap, env::temp_dir, io::Write};
use std::{
collections::BTreeMap,
env::temp_dir,
io::{self, Write},
};
use test_util::TestRepos;
use vergen_lib::{count_idempotent, VergenKey};

Expand Down Expand Up @@ -1287,4 +1312,25 @@ mod test {
assert!(result.is_ok());
});
}

#[test]
#[serial]
fn git_no_index_update() -> Result<()> {
let repo = TestRepos::new(true, true, false)?;
repo.set_index_magic_mtime();

// The GIT_OPTIONAL_LOCKS=0 environment variable should prevent modifications to the index
let mut gitcl = GitclBuilder::default()
.all()
.describe(true, true, None)
.build()?;
let _ = gitcl.at_path(repo.path());
let failed = Emitter::default()
.add_instructions(&gitcl)?
.emit_to(&mut io::stdout())?;
assert!(!failed);

repo.assert_is_index_magic_mtime();
Ok(())
}
}
22 changes: 21 additions & 1 deletion vergen-gix/src/gix/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -663,7 +663,7 @@ impl AddEntries for Gix {

#[cfg(test)]
mod test {
use std::env::temp_dir;
use std::{env::temp_dir, io};

use super::GixBuilder;
use anyhow::Result;
Expand Down Expand Up @@ -1032,4 +1032,24 @@ mod test {
assert!(result.is_ok());
});
}

#[test]
#[serial]
fn git_no_index_update() -> Result<()> {
let repo = TestRepos::new(true, true, false)?;
repo.set_index_magic_mtime();

let mut gix = GixBuilder::default()
.all()
.describe(true, true, None)
.build()?;
let _ = gix.at_path(repo.path());
let failed = Emitter::default()
.add_instructions(&gix)?
.emit_to(&mut io::stdout())?;
assert!(!failed);

repo.assert_is_index_magic_mtime();
Ok(())
}
}

0 comments on commit 0c96986

Please sign in to comment.