Skip to content

Recursively apply preprocessor #682

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

Jump to bottom
Merged
merged 1 commit into from
May 20, 2018
Merged

Recursively apply preprocessor #682

merged 1 commit into from
May 20, 2018

Conversation

AndyGauge
Copy link
Contributor

Sends the content inserted through the links processor.

fixes #678

@Michael-F-Bryan
Copy link
Contributor

This seems fairly reasonable. If I'm wanting to include a document it makes sense to also expand any {#include ...} statements inside it.

Should we bother to guard against accidental include-cycles? It's certainly an edge case of an edge case, but we'd end up with infinite recursion and a crash due to stack overflow. Which would be kinda annoying.

@AndyGauge
Copy link
Contributor Author

I created a book that in chapter_1.md, I {{#include chapter_1.md}} and this was the message:

2018-05-01 08:24:58 [INFO] (mdbook::book): Book building has started

thread 'main' has overflowed its stack
fatal runtime error: stack overflow
Aborted (core dumped)

What this doesn't tell me is what file caused the overflow, so maybe some messaging around it could curtail issues.

@AndyGauge
Copy link
Contributor Author 

2018-05-01 11:06:58 [INFO] (mdbook::book): Book building has started                                                      
2018-05-01 11:06:58 [ERROR] (mdbook::preprocess::links): Stack depth exceeded in chapter_1.md. Check for cyclic includes  
2018-05-01 11:06:58 [INFO] (mdbook::book): Running the html backend                                                       
2018-05-01 11:06:58 [INFO] (mdbook::serve): Serving on: http://localhost:3000                                             
2018-05-01 11:06:58 [INFO] (ws): Listening for new connections on 127.0.0.1:3001.                                         
2018-05-01 11:06:58 [INFO] (mdbook::watch): Listening for changes...

@AndyGauge AndyGauge force-pushed the recursive-include branch from 81ce2b5 to b1d6592 Compare May 1, 2018 18:29
Michael-F-Bryan
Michael-F-Bryan suggested changes May 2, 2018
View reviewed changes
Copy link
Contributor

@Michael-F-Bryan Michael-F-Bryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the error message and stack depth approach taken here. There are only two small knits I could see.

Would you also be able to add a test which contains an include cycle to make sure we don't blow the stack? It should be enough to add a cyclic include to the dummy book in the tests/ directory and then assert the original page's contents are as we expect.

src/preprocess/links.rs Outdated
}
else {
if let Some(source_path) = source.as_os_str().to_str() {
error!("Stack depth exceeded in {}. Check for cyclic includes",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this error message, it's quite helpful and user friendly.

src/preprocess/links.rs Outdated
previous_end_index = playpen.end_index;
}
else {
if let Some(source_path) = source.as_os_str().to_str() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can actually remove the need for the if let Some(...) and two branches by using source.display() in the error!() message.

src/preprocess/links.rs Outdated
@@ -58,8 +59,18 @@ fn replace_all<P: AsRef<Path>>(s: &str, path: P) -> String {

match playpen.render_with_path(&path) {
Ok(new_content) => {
replaced.push_str(&new_content);
previous_end_index = playpen.end_index;
if depth < 200 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can probably be pulled out into its own private const (e.g. MAX_NESTED_INCLUDES_DEPTH) so it's not a magic number.

I highly doubt anyone's going to have more than 2 or 3 nested includes, so we can probably cap it off at 10 instead of 200. It'd be annoying if we actually overflowed the stack before hitting the 200 depth limit.

AndyGauge
AndyGauge commented May 2, 2018
View reviewed changes
tests/dummy_book/src/first/recursive.md
@@ -0,0 +1,2 @@
Around the world, around the world
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://genius.com/Daft-punk-around-the-world-lyrics

@AndyGauge
Copy link
Contributor Author

AndyGauge commented May 3, 2018 •
edited
Loading

So I assume the unwraps() have to go, but I wanted to get some feedback on this last change. What was happening is I included a file like {{#include data/adata-file.md}} then adata-file.md had {{#include ../links.md}} and the relative path was parsing from the original file, not from the context of the included file's path.

If I looked at the output from the file vs the output of the file when it was included, they would be different--one would have a broken include and one would do it right.

@Michael-F-Bryan
Copy link
Contributor

If I looked at the output from the file vs the output of the file when it was included, they would be different--one would have a broken include and one would do it right.

Ah I can see how that would happen. As you mentioned, the "current directory" changes when trying to include a link from a different directory. I think how you've done it (new source path = old current directory + new link) is a good solution and should hopefully be intuitive for people.

Once the unwraps are swapped with proper error handling I think this PR will be good to merge. I'm assuming hitting one of the unwraps while rendering recursive.md is what's causing travis to complain.

@AndyGauge AndyGauge force-pushed the recursive-include branch from 406d786 to c6fccc0 Compare May 4, 2018 04:37
@AndyGauge AndyGauge mentioned this pull request May 5, 2018
Merged
@Michael-F-Bryan
Copy link
Contributor

Oops, I didn't notice you'd added a commit to resolve those last couple nits. This should be good to merge now.

@Michael-F-Bryan Michael-F-Bryan merged commit 2a55ff6 into rust-lang:master May 20, 2018
@AndyGauge
Copy link
Contributor Author

Will this make it into v0.1.8?

Thanks!

Ruin0x11 pushed a commit to Ruin0x11/mdBook that referenced this pull request Aug 30, 2020
@AndyGauge @Michael-F-Bryan
Recursively apply preprocessor (rust-lang#682)
db97b9c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Michael-F-Bryan Michael-F-Bryan Michael-F-Bryan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Include a file that includes a file renders the handlebars
2 participants
@AndyGauge @Michael-F-Bryan