Skip to content

Commit 594f0de

Browse files
committed
fix: make repo-root absolute path
Keeping the repo-root path relative breaks parsing of absolute paths in clang-tidy output. This didn't surface in the tests because they all use a compilation database generated for ninja (which uses relative paths). But in the test repo, we are using CMake which generates absolute paths in the compilation database. Remember, clang-tidy outputs filenames however they are stated in the compilation database. If not using a compilation database, then clang-tidy just outputs the filename as it was given in the CLI. --- I also removed an artifact debug log from developing solution in #386
1 parent 89f7b3d commit 594f0de

4 files changed

Lines changed: 35 additions & 14 deletions

File tree

cpp-linter/src/clang_tools/clang_tidy.rs

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -395,10 +395,6 @@ pub fn run_clang_tidy(
395395
&clang_params.database_json,
396396
&clang_params.repo_root,
397397
)?;
398-
logs.push((
399-
log::Level::Debug,
400-
format!("Parsed {} clang-tidy notifications", notes.len()),
401-
));
402398

403399
let tidy_advice = TidyAdvice { notes };
404400
file.patched_path = Some(cache_patch_path.to_path_buf());

cpp-linter/src/common_fs.rs

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -383,6 +383,20 @@ impl FileObj {
383383
}
384384
}
385385

386+
/// Make the given `path` absolute.
387+
///
388+
/// This function will canonicalize the path and remove any `\\?\` prefix
389+
/// returned by Windows API, which is really only designed for other Windows API.
390+
///
391+
/// This function can error if the given path fails to be canonicalized.
392+
/// This may happen if the path does not exist or a non-final path
393+
/// component is not a directory.
394+
pub(crate) fn mk_path_abs<P: AsRef<Path>>(path: P) -> Result<PathBuf, std::io::Error> {
395+
let abs_path = path.as_ref().canonicalize()?;
396+
let abs_path_str = abs_path.to_string_lossy();
397+
Ok(PathBuf::from(abs_path_str.trim_start_matches(r"\\?\")))
398+
}
399+
386400
#[cfg(test)]
387401
mod test {
388402
use std::path::PathBuf;
@@ -438,4 +452,13 @@ mod test {
438452
let file_obj = FileObj::new(PathBuf::from("tests/demo/demo.cpp"));
439453
assert!(!file_obj.is_line_in_diff(&42));
440454
}
455+
456+
#[test]
457+
fn canonical_path() {
458+
let file_obj = FileObj::new(PathBuf::from("tests/demo/demo.cpp"));
459+
let canonical_path = super::mk_path_abs(&file_obj.name).unwrap();
460+
assert!(canonical_path.is_file());
461+
println!("Canonical path: {}", canonical_path.display());
462+
assert!(!canonical_path.to_string_lossy().starts_with(r"\\?\"));
463+
}
441464
}

cpp-linter/src/git.rs

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ mod test {
1010

1111
use std::{
1212
env::{self, current_dir, set_current_dir},
13-
fs,
1413
process::Command,
1514
};
1615

@@ -41,12 +40,8 @@ mod test {
4140
}
4241
}
4342
if let Some(patch) = patch_path {
44-
let canonical_patch_path = fs::canonicalize(patch).unwrap();
45-
let patch_path = canonical_patch_path
46-
.to_str()
47-
.unwrap()
48-
// on Windows, canonical paths can have a prefix of "\\?\" which git does not recognize
49-
.trim_start_matches("\\\\?\\");
43+
let canonical_patch_path = crate::common_fs::mk_path_abs(patch).unwrap();
44+
let patch_path = canonical_patch_path.to_str().unwrap();
5045
let ok = Command::new("git")
5146
.args(["apply", "--index", patch_path])
5247
.current_dir(path)

cpp-linter/src/run.rs

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ use log::{LevelFilter, set_max_level};
2020
use crate::{
2121
clang_tools::capture_clang_tools_output,
2222
cli::{ClangParams, Cli, CliCommand, FeedbackInput, LinesChangedOnly},
23-
common_fs::FileObj,
23+
common_fs::{FileObj, mk_path_abs},
2424
rest_client::RestClient,
2525
};
2626
use git_bot_feedback::FileFilter;
@@ -47,7 +47,7 @@ const VERSION: &str = env!("CARGO_PKG_VERSION");
4747
/// alias ("path/to/cpp-linter.exe"). Thus, the parser in [`crate::cli`] will halt on an error
4848
/// because it is not configured to handle positional arguments.
4949
pub async fn run_main(args: Vec<String>) -> Result<()> {
50-
let cli = Cli::parse_from(args);
50+
let mut cli = Cli::parse_from(args);
5151

5252
if matches!(cli.commands, Some(CliCommand::Version))
5353
|| cli.general_options.version == RequestedVersion::NoValue
@@ -81,7 +81,14 @@ pub async fn run_main(args: Vec<String>) -> Result<()> {
8181
.collect::<Vec<&str>>(),
8282
None,
8383
);
84-
let gitmodules = cli.source_options.repo_root.join(".gitmodules");
84+
let repo_root_abs = mk_path_abs(&cli.source_options.repo_root).with_context(|| {
85+
format!(
86+
"Failed to canonicalize the repo root path: {}",
87+
cli.source_options.repo_root.to_string_lossy()
88+
)
89+
})?;
90+
let gitmodules = repo_root_abs.join(".gitmodules");
91+
cli.source_options.repo_root = repo_root_abs;
8592
file_filter.parse_submodules(Some(gitmodules.as_path()));
8693
if let Some(files) = &cli.not_ignored {
8794
file_filter.not_ignored.extend(files.clone());

0 commit comments

Comments
 (0)