-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat: ignore files listed in .mdbookignore during build
#3040
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
9452beb
6bbcd17
1b0a23a
9620641
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| //! Filesystem utilities and helpers. | ||
|
|
||
| use anyhow::{Context, Result}; | ||
| use ignore::gitignore::Gitignore; | ||
| use std::fs; | ||
| use std::path::{Component, Path, PathBuf}; | ||
| use tracing::debug; | ||
|
|
@@ -96,12 +97,30 @@ pub fn copy_files_except_ext( | |
| recursive: bool, | ||
| avoid_dir: Option<&PathBuf>, | ||
| ext_blacklist: &[&str], | ||
| ) -> Result<()> { | ||
| let mut builder = ignore::gitignore::GitignoreBuilder::new(from); | ||
| for ext in ext_blacklist { | ||
| builder.add_line(None, &format!("*.{ext}"))?; | ||
| } | ||
| let ignore = builder.build()?; | ||
|
|
||
| copy_files_except_ignored(from, to, recursive, avoid_dir, Some(&ignore)) | ||
| } | ||
|
|
||
| /// Copies all files of a directory to another one except the files that are | ||
| /// ignored by the passed [`Gitignore`] | ||
| pub fn copy_files_except_ignored( | ||
| from: &Path, | ||
| to: &Path, | ||
| recursive: bool, | ||
| avoid_dir: Option<&PathBuf>, | ||
| ignore: Option<&Gitignore>, | ||
| ) -> Result<()> { | ||
| debug!( | ||
| "Copying all files from {} to {} (blacklist: {:?}), avoiding {:?}", | ||
| "Copying all files from {} to {} (ignoring: {:?}), avoiding {:?}", | ||
| from.display(), | ||
| to.display(), | ||
| ext_blacklist, | ||
| ignore, | ||
| avoid_dir | ||
| ); | ||
|
|
||
|
|
@@ -131,16 +150,25 @@ pub fn copy_files_except_ext( | |
| } | ||
| } | ||
|
|
||
| if let Some(ignore) = ignore { | ||
| let relative_path = | ||
| pathdiff::diff_paths(&entry, from).unwrap_or_else(|| entry.to_path_buf()); | ||
| if ignore.matched(&relative_path, true).is_ignore() { | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| // check if output dir already exists | ||
| if !target_file_path.exists() { | ||
| fs::create_dir(&target_file_path)?; | ||
| } | ||
|
|
||
| copy_files_except_ext(&entry, &target_file_path, true, avoid_dir, ext_blacklist)?; | ||
| copy_files_except_ignored(&entry, &target_file_path, true, avoid_dir, ignore)?; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The way this handles relative paths seems to break root-anchored patterns like When descending, I think maybe the fix is to always compute relative paths against Also, can you add some tests that include this scenario? |
||
| } else if metadata.is_file() { | ||
| // Check if it is in the blacklist | ||
| if let Some(ext) = entry.extension() { | ||
| if ext_blacklist.contains(&ext.to_str().unwrap()) { | ||
| if let Some(ignore) = ignore { | ||
| let relative_path = | ||
| pathdiff::diff_paths(&entry, from).unwrap_or_else(|| entry.to_path_buf()); | ||
| if ignore.matched(&relative_path, false).is_ignore() { | ||
| continue; | ||
| } | ||
| } | ||
|
|
@@ -210,6 +238,7 @@ fn copy<P: AsRef<Path>, Q: AsRef<Path>>(from: P, to: Q) -> Result<()> { | |
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use ignore::gitignore::GitignoreBuilder; | ||
| use std::io::Result; | ||
| use std::path::Path; | ||
|
|
||
|
|
@@ -270,4 +299,56 @@ mod tests { | |
| panic!("output/symlink.png should exist") | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn copy_files_except_ignored_test() { | ||
| let tmp = match tempfile::TempDir::new() { | ||
| Ok(t) => t, | ||
| Err(e) => panic!("Could not create a temp dir: {e}"), | ||
| }; | ||
|
|
||
| // Create files and directories | ||
| write(tmp.path().join("file.txt"), "").unwrap(); | ||
| write(tmp.path().join("file.json"), "").unwrap(); | ||
| write(tmp.path().join("ignored_dir/nested.txt"), "").unwrap(); | ||
| write(tmp.path().join("included_dir/file.json"), "").unwrap(); | ||
| write(tmp.path().join("included_dir/file.txt"), "").unwrap(); | ||
|
|
||
| // Create a gitignore that ignores *.txt and ignored_dir/ | ||
| let mut builder = GitignoreBuilder::new(tmp.path()); | ||
| builder.add_line(None, "*.txt").unwrap(); | ||
| builder.add_line(None, "ignored_dir/").unwrap(); | ||
| let ignore = builder.build().unwrap(); | ||
|
|
||
| // Create output dir | ||
| create_dir_all(tmp.path().join("output")).unwrap(); | ||
|
|
||
| copy_files_except_ignored( | ||
| tmp.path(), | ||
| &tmp.path().join("output"), | ||
| true, | ||
| None, | ||
| Some(&ignore), | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| // Check that .txt files are ignored | ||
| if tmp.path().join("output/file.txt").exists() { | ||
| panic!("output/file.txt should not exist") | ||
| } | ||
| if tmp.path().join("output/included_dir/file.txt").exists() { | ||
| panic!("output/included_dir/file.txt should not exist") | ||
| } | ||
| // Check that ignored_dir is not copied | ||
| if tmp.path().join("output/ignored_dir").exists() { | ||
| panic!("output/ignored_dir should not exist") | ||
| } | ||
| // Check that non-ignored files are copied | ||
| if !tmp.path().join("output/file.json").exists() { | ||
| panic!("output/file.json should exist") | ||
| } | ||
| if !tmp.path().join("output/included_dir/file.json").exists() { | ||
| panic!("output/included_dir/file.json should exist") | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ use crate::theme::Theme; | |
| use crate::utils::ToUrlPath; | ||
| use anyhow::{Context, Result, bail}; | ||
| use handlebars::Handlebars; | ||
| use ignore::gitignore::GitignoreBuilder; | ||
| use mdbook_core::book::{Book, BookItem, Chapter}; | ||
| use mdbook_core::config::{BookConfig, Config, HtmlConfig}; | ||
| use mdbook_core::utils::fs; | ||
|
|
@@ -444,7 +445,31 @@ impl Renderer for HtmlHandlebars { | |
| .context("Unable to emit redirects")?; | ||
|
|
||
| // Copy all remaining files, avoid a recursive copy from/to the book build dir | ||
| fs::copy_files_except_ext(&src_dir, destination, true, Some(&build_dir), &["md"])?; | ||
| let mut builder = GitignoreBuilder::new(&src_dir); | ||
|
|
||
| // Add default ignores | ||
| builder.add_line(None, "/.mdbookignore")?; | ||
| builder.add_line(None, "/.gitignore")?; | ||
| builder.add_line(None, "/.git/")?; | ||
| builder.add_line(None, "/book.toml")?; | ||
| builder.add_line(None, "/theme/")?; | ||
| builder.add_line(None, "*.md")?; | ||
|
Comment on lines
+451
to
+456
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure why all of these extra ignores were added. For example, this would break a book that has images or other content inside a directory called theme. Can this be trimmed down to just *.md and |
||
|
|
||
| let mdbook_ignore = src_dir.join(".mdbookignore"); | ||
| if mdbook_ignore.exists() { | ||
| if let Some(err) = builder.add(mdbook_ignore) { | ||
| warn!("Unable to load '.mdbookignore' file: {}", err); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would this be a warning and not an error? |
||
| } | ||
| } | ||
| let ignore = builder.build()?; | ||
|
|
||
| fs::copy_files_except_ignored( | ||
| &src_dir, | ||
| destination, | ||
| true, | ||
| Some(&build_dir), | ||
| Some(&ignore), | ||
| )?; | ||
|
|
||
| info!("HTML book written to `{}`", destination.display()); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,3 +85,18 @@ fn dest_dir_relative_path() { | |
| }); | ||
| assert!(current_dir.join("foo/index.html").exists()); | ||
| } | ||
|
|
||
| // Test that .mdbookignore files are respected (single files, glob patterns, and directories). | ||
| #[test] | ||
| fn mdbookignore() { | ||
| let mut test = BookTest::from_dir("build/mdbookignore"); | ||
| test.build(); | ||
| // Single file listed by name should not be copied. | ||
| assert!(!test.dir.join("book/ignored_file").exists()); | ||
| // Files matching *.txt glob pattern should not be copied. | ||
| assert!(!test.dir.join("book/ignored.txt").exists()); | ||
| // Directories listed in .mdbookignore should not be copied. | ||
| assert!(!test.dir.join("book/ignored_dir").exists()); | ||
| // Non-ignored files should be copied. | ||
| assert!(test.dir.join("book/included.json").exists()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's some files in the book that this test isn't checking. For example, |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| [book] | ||
| title = "mdbookignore test" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| ignored_file | ||
| *.txt | ||
| ignored_dir/ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| # Summary | ||
|
|
||
| - [Chapter 1](./chapter_1.md) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| # Chapter 1 | ||
|
|
||
| This is chapter 1. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| This txt file should not be copied. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| This file in ignored dir should not be copied. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| This file should not be copied. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| This file should be copied. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| /* custom theme */ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| @font-face { } |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you say more why pathdiff is needed? Would it be possible to drop the
pathdiffdependency by usingentry.strip_prefix(from)? Entries should always be underfrom, so I'm not sure what scenario this is needed.View changes since the review