feat: ignore files listed in .mdbookignore during build#3040
Conversation
|
@ehuss, I'd appreciate a review when you get a chance. |
This comment has been minimized.
This comment has been minimized.
jhult
force-pushed
the
feat/mdbookignore
branch
from
a42c81d to
6059e1b
Compare
This comment has been minimized.
This comment has been minimized.
jhult
force-pushed
the
feat/mdbookignore
branch
2 times, most recently
from
553101f to
4bbff36
Compare
This comment has been minimized.
This comment has been minimized.
Load .mdbookignore file from src directory and use it to filter files copied during the HTML build process.
Add a new function that uses the crate to filter files during copy operations, supporting gitignore-style patterns. build: update Cargo.lock for ignore and pathdiff dependency
jhult
force-pushed
the
feat/mdbookignore
branch
from
4bbff36 to
9620641
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@ehuss - any thoughts on this? |
| 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")?; |
There was a problem hiding this comment.
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 .mdbookignore?
| // 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()); |
There was a problem hiding this comment.
There's some files in the book that this test isn't checking. For example, file.json, custom.css, etc. Can we make sure everything gets checked?
| 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); |
There was a problem hiding this comment.
Why would this be a warning and not an error?
| } | ||
|
|
||
| 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)?; |
There was a problem hiding this comment.
The way this handles relative paths seems to break root-anchored patterns like /subdir/foo.txt.
When descending, from is now the subdirectory (e.g., src/subdir/), but ignore was built with the original root (e.g., src). In the recursive invocation, pathdiff::diff_paths(&entry, from) computes the path relative to the subdirectory, not relative to the gitignore root. So a root-anchored pattern like /subdir/file.txt will never match file.txt once inside subdir/.
I think maybe the fix is to always compute relative paths against ignore.path() (the gitignore's own root), not against from.
Also, can you add some tests that include this scenario?
|
|
||
| if let Some(ignore) = ignore { | ||
| let relative_path = | ||
| pathdiff::diff_paths(&entry, from).unwrap_or_else(|| entry.to_path_buf()); |
There was a problem hiding this comment.
Can you say more why pathdiff is needed? Would it be possible to drop the pathdiff dependency by using entry.strip_prefix(from)? Entries should always be under from, so I'm not sure what scenario this is needed.
3 participants
Rebase/update of #1908 on top of
master.