Skip to content

[3/n] [dropshot-api-manager] handle unparseable git ref files gracefully #40

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

Open
sunshowers wants to merge 3 commits into
base: sunshowers/spr/main.3n-dropshot-api-manager-handle-unparseable-git-ref-files-gracefully
Choose a base branch
from
Open

[3/n] [dropshot-api-manager] handle unparseable git ref files gracefully #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a05a96a
[spr] initial version
sunshowers Jan 7, 2026
6b4a8ae
use atomicwrites
sunshowers Jan 7, 2026
4095148
clarify needs_rewrite
sunshowers Jan 7, 2026
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
51 changes: 51 additions & 0 deletions crates/dropshot-api-manager-types/src/validation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,57 @@ impl ApiSpecFileName {
_ => self.clone(),
}
}

/// Converts a `Versioned` to its `VersionedGitRef` equivalent.
///
/// For `VersionedGitRef` files, returns a clone of self.
/// For `Lockstep` files, returns a clone of self (lockstep files are not
/// converted to git refs).
pub fn to_git_ref_filename(&self) -> ApiSpecFileName {
match &self.kind {
ApiSpecFileNameKind::Versioned { version, hash } => {
ApiSpecFileName::new(
self.ident.clone(),
ApiSpecFileNameKind::VersionedGitRef {
version: version.clone(),
hash: hash.clone(),
},
)
}
_ => self.clone(),
}
}

/// Returns the basename for this file as a git ref.
///
/// - If this is already a `VersionedGitRef`, returns `basename()` directly.
/// - If this is a `Versioned`, returns `basename() + ".gitref"`.
/// - For `Lockstep`, returns `basename()` (lockstep files are not converted
/// to git refs).
pub fn git_ref_basename(&self) -> String {
match &self.kind {
ApiSpecFileNameKind::VersionedGitRef { .. } => self.basename(),
ApiSpecFileNameKind::Versioned { .. } => {
format!("{}.gitref", self.basename())
}
ApiSpecFileNameKind::Lockstep => self.basename(),
}
}

/// Returns the basename for this file as a JSON file.
///
/// - If this is a `VersionedGitRef`, returns the basename without the
/// `.gitref` suffix.
/// - Otherwise, returns `basename()` directly.
pub fn json_basename(&self) -> String {
match &self.kind {
ApiSpecFileNameKind::Versioned { .. } => self.basename(),
ApiSpecFileNameKind::VersionedGitRef { version, hash } => {
format!("{}-{}-{}.json", self.ident, version, hash)
}
ApiSpecFileNameKind::Lockstep => self.basename(),
}
}
}

/// Describes how a particular OpenAPI document is named.
Expand Down
164 changes: 161 additions & 3 deletions crates/dropshot-api-manager/src/git.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,11 +367,55 @@ impl GitRef {
) -> anyhow::Result<Vec<u8>> {
git_show_file(repo_root, &GitRevision::from(self.commit), &self.path)
}

/// Returns the canonical file contents for this git ref.
///
/// The canonical format is `commit:path\n` where:
/// - The path uses forward slashes (even on Windows).
/// - The file ends with a single newline.
pub fn to_file_contents(&self) -> String {
format!("{}\n", self)
}

/// Returns whether the given file contents representing a _valid_ Git ref
/// file need to be rewritten.
///
/// File contents need rewriting if they don't match the canonical format:
///
/// - Missing trailing newline.
/// - Contains backslashes in the path.
/// - Has extra whitespace.
///
/// This static method is used to detect files that are valid but need
/// normalization.
pub fn needs_rewrite(contents: &str) -> bool {
// Check for missing trailing newline.
if !contents.ends_with('\n') {
return true;
}

// Check for extra trailing newlines or other whitespace issues.
let trimmed = contents.trim();
if trimmed.len() + 1 != contents.len() {
return true;
}

// Check for backslashes in the path (after the colon).
if let Some((_commit, path)) = trimmed.split_once(':') {
if path.contains('\\') {
return true;
}
}

false
}
}

impl fmt::Display for GitRef {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}:{}", self.commit, self.path)
// Always use forward slashes for paths, even on Windows.
let path = self.path.as_str().replace('\\', "/");
write!(f, "{}:{}", self.commit, path)
}
}

Expand All @@ -380,10 +424,19 @@ impl FromStr for GitRef {

fn from_str(s: &str) -> Result<Self, Self::Err> {
let s = s.trim();
if s.is_empty() {
return Err(GitRefParseError::EmptyInput);
}
let (commit, path) = s
.split_once(':')
.ok_or_else(|| GitRefParseError::InvalidFormat(s.to_owned()))?;
let commit: GitCommitHash = commit.parse()?;
// Normalize backslashes to forward slashes for cross-platform
// compatibility.
let path = path.replace('\\', "/");
if path.is_empty() {
return Err(GitRefParseError::EmptyPath);
}
Ok(GitRef { commit, path: Utf8PathBuf::from(path) })
}
}
Expand All @@ -392,13 +445,24 @@ impl FromStr for GitRef {
#[derive(Debug, Error)]
#[non_exhaustive]
pub enum GitRefParseError {
/// The input was empty or contained only whitespace.
#[error("git ref file is empty")]
EmptyInput,

/// The git ref string did not contain the expected 'commit:path' format.
#[error("invalid git ref format: expected 'commit:path', got {0}")]
#[error(
"invalid git ref format: expected 'commit:path', got {0:?} \
(missing ':' separator)"
)]
InvalidFormat(String),

/// The commit hash in the git ref was invalid.
#[error("invalid commit hash")]
#[error("invalid commit hash in git ref")]
InvalidCommitHash(#[from] CommitHashParseError),

/// The path component was empty.
#[error("git ref has empty path (nothing after ':')")]
EmptyPath,
}

#[cfg(test)]
Expand Down Expand Up @@ -515,4 +579,98 @@ mod tests {
let parsed = s.parse::<GitRef>().unwrap();
assert_eq!(git_ref, parsed);
}

#[test]
fn test_git_ref_to_file_contents() {
let git_ref = GitRef {
commit: VALID_SHA1.parse().unwrap(),
path: Utf8PathBuf::from("path/to/file.json"),
};
let contents = git_ref.to_file_contents();
let expected = format!("{}:path/to/file.json\n", VALID_SHA1);
assert_eq!(contents, expected, "should have trailing newline");
}

#[test]
fn test_git_ref_display_forward_slashes() {
// Even if the path internally has backslashes, display should use
// forward slashes.
let git_ref = GitRef {
commit: VALID_SHA1.parse().unwrap(),
path: Utf8PathBuf::from("path\\to\\file.json"),
};
let s = git_ref.to_string();
assert!(
!s.contains('\\'),
"display should convert backslashes to forward slashes"
);
assert!(s.contains("path/to/file.json"));
}

#[test]
fn test_git_ref_parse_normalizes_backslashes() {
// Parsing should normalize backslashes to forward slashes.
let input = format!("{}:path\\to\\file.json", VALID_SHA1);
let git_ref = input.parse::<GitRef>().unwrap();
assert_eq!(
git_ref.path.as_str(),
"path/to/file.json",
"backslashes should be normalized to forward slashes"
);
}

#[test]
fn test_git_ref_parse_error_variants() {
// Empty input.
let result = "".parse::<GitRef>();
assert!(matches!(result, Err(GitRefParseError::EmptyInput)));

// Whitespace-only input.
let result = " \n ".parse::<GitRef>();
assert!(matches!(result, Err(GitRefParseError::EmptyInput)));

// Empty path (valid commit hash but nothing after colon).
let input = format!("{}:", VALID_SHA1);
let result = input.parse::<GitRef>();
assert!(matches!(result, Err(GitRefParseError::EmptyPath)));
}

#[test]
fn test_git_ref_needs_rewrite() {
// Canonical format: forward slashes, single trailing newline.
let canonical = format!("{}:path/to/file.json\n", VALID_SHA1);
assert!(
!GitRef::needs_rewrite(&canonical),
"canonical format should not need rewrite"
);

// Missing trailing newline.
let missing_newline = format!("{}:path/to/file.json", VALID_SHA1);
assert!(
GitRef::needs_rewrite(&missing_newline),
"missing trailing newline should need rewrite"
);

// Extra trailing newlines.
let extra_newlines = format!("{}:path/to/file.json\n\n", VALID_SHA1);
assert!(
GitRef::needs_rewrite(&extra_newlines),
"extra trailing newlines should need rewrite"
);

// Leading whitespace.
let leading_whitespace =
format!(" {}:path/to/file.json\n", VALID_SHA1);
assert!(
GitRef::needs_rewrite(&leading_whitespace),
"leading whitespace should need rewrite"
);

// Backslashes in path.
let backslashes = format!("{}:path\\to\\file.json\n", VALID_SHA1);
assert!(
GitRef::needs_rewrite(&backslashes),
"backslashes in path should need rewrite"
);
}
}
65 changes: 24 additions & 41 deletions crates/dropshot-api-manager/src/resolved.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,27 +563,25 @@ impl Fix<'_> {
Fix::UpdateSymlink { .. } => {}
Fix::ConvertToGitRef { local_file, .. } => {
// Writes to the .gitref path, not the JSON path.
let json_path = local_file.spec_file_name().path();
paths
.insert(Utf8PathBuf::from(format!("{}.gitref", json_path)));
paths.insert(
local_file.spec_file_name().to_git_ref_filename().path(),
);
}
Fix::ConvertToJson { local_file, .. } => {
// Writes to the JSON path (removing .gitref suffix).
let git_ref_path = local_file.spec_file_name().path();
if let Some(json_path) =
git_ref_path.as_str().strip_suffix(".gitref")
{
paths.insert(Utf8PathBuf::from(json_path));
}
// Writes to the JSON path.
paths.insert(
local_file.spec_file_name().to_json_filename().path(),
);
}
Fix::RegenerateFromBlessed { local_file, git_ref, .. } => {
if git_ref.is_some() {
// When regenerating as git ref, writes to a .gitref file.
let json_path = local_file.spec_file_name().path();
paths.insert(Utf8PathBuf::from(format!(
"{}.gitref",
json_path
)));
// Writes to a .gitref file.
paths.insert(
local_file
.spec_file_name()
.to_git_ref_filename()
.path(),
);
} else {
// Overwrites the corrupted local file.
paths.insert(local_file.spec_file_name().path().to_owned());
Expand Down Expand Up @@ -667,21 +665,18 @@ impl Fix<'_> {
Fix::ConvertToGitRef { local_file, git_ref } => {
let json_path = root.join(local_file.spec_file_name().path());

let git_ref_basename = format!(
"{}.gitref",
local_file.spec_file_name().basename()
);
let git_ref_basename =
local_file.spec_file_name().git_ref_basename();
let git_ref_path = json_path
.parent()
.ok_or_else(|| anyhow!("cannot get parent directory"))?
.join(&git_ref_basename);

// Write the git ref file. Add a trailing newline so diffs don't
// have the "\ No newline at end of file" message. Otherwise,
// the extra newline has no impact on usability or correctness.
// Write the git ref file in canonical format (forward slashes,
// trailing newline).
let overwrite_status = overwrite_file(
&git_ref_path,
format!("{}\n", git_ref).as_bytes(),
git_ref.to_file_contents().as_bytes(),
)?;

// Remove the original JSON file.
Expand All @@ -700,17 +695,7 @@ impl Fix<'_> {
// valid.
let contents = blessed.contents();

// Compute the JSON file path by removing the .gitref suffix.
let git_ref_basename = local_file.spec_file_name().basename();
let json_basename = git_ref_basename
.strip_suffix(".gitref")
.ok_or_else(|| {
anyhow!(
"expected git ref file to end with .gitref: {}",
git_ref_basename
)
})?;

let json_basename = local_file.spec_file_name().json_basename();
let json_path = git_ref_path
.parent()
.ok_or_else(|| anyhow!("cannot get parent directory"))?
Expand All @@ -737,19 +722,17 @@ impl Fix<'_> {

if let Some(git_ref) = git_ref {
// Write as a git ref file.
let git_ref_basename = format!(
"{}.gitref",
local_file.spec_file_name().basename()
);
let git_ref_basename =
local_file.spec_file_name().git_ref_basename();
let git_ref_path = local_path
.parent()
.ok_or_else(|| anyhow!("cannot get parent directory"))?
.join(&git_ref_basename);

// Add a trailing newline for clean diffs.
// Write in canonical format (forward slashes, trailing newline).
let overwrite_status = overwrite_file(
&git_ref_path,
format!("{}\n", git_ref).as_bytes(),
git_ref.to_file_contents().as_bytes(),
)?;

Ok(vec![
Expand Down
Loading