Skip to content
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

Work around an AsciiDoctor v2.0.19 regression #1880

Merged

Conversation

dscho
Copy link
Member

@dscho dscho commented Sep 26, 2024

Changes

This fixes the rendering of single plus characters that want to use a monospace font. Due to a regression in AsciiDoctor v2.0.19 (which is still present in AsciiDoctor v2.0.23, the version we're using since #1804, upgraded from v2.0.16), when multiple such constructs exist within the same line, AsciiDoctor misinterprets them to mean the start and end of literal monospace.

The work-around is to replace the `+` instances with `{plus}` (which even recent AsciiDoctor renders as intended).

Context

I propose this as an alternative to #1879 (which changes the auto-generated files, meaning that the fixes would be overridden the next time the book sources are updated).

The AsciiDoctor regression is tracked here.

In git#1879 a regression was
reported in the way the explanation of the refspec format is rendered;
It is now broken.

The reason is a regression in AsciiDoctor (reported at
asciidoctor/asciidoctor#4624).

As a work-around, replace the `+` instances (which are mis-interpreted
if more than one exists in the same line) by `{plus}` (which AsciiDoctor
renders exactly like AsciiDoc would).

Signed-off-by: Johannes Schindelin <[email protected]>
In git#1879 it was reported that at
least the English and Korean version of chapter 10 section 5 displayed a
regression where the explanation of the refspec format was formatted
incorrectly.

Turns out that this is a regression in AsciiDoctor v2.0.19. We just
adjusted the `update-book2.rb` script to work around this issue, and now
it is time to commit the result of re-running that script for all
variants of the ProGit book.

This commit is best viewed with `--word-diff`.

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho dscho self-assigned this Sep 26, 2024
@dscho
Copy link
Member Author

dscho commented Sep 26, 2024

Note that this PR looks bigger than it actually is: only 158a2c9 is interesting, the other commit basically re-renders the books, is all.

Copy link
Member

@ttaylorr ttaylorr left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. I wonder if we might want to mark the external directory as uninteresting for the purposes of the PR diff by setting the linguist-generated attribute to "true" for those files via .gitattributes [source]. What do you think?

@ttaylorr ttaylorr merged commit 09a0069 into git:gh-pages Sep 26, 2024
1 check passed
@dscho dscho deleted the work-around-asciidoctor-2.0.19-regression branch September 26, 2024 15:41
@dscho
Copy link
Member Author

dscho commented Sep 26, 2024

we might want to mark the external directory as uninteresting for the purposes of the PR diff

I fear that won't prevent contributors from modifying this part directly, though. And we'd want to see if anybody tries to sneak in links to some dubious site.

So while I like the idea of directing contributors away from the external/ tree, I suspect that we need to find a better way.

@ttaylorr
Copy link
Member

I fear that won't prevent contributors from modifying this part directly, though. And we'd want to see if anybody tries to sneak in links to some dubious site.

Mmm. That's a good call out.

So while I like the idea of directing contributors away from the external/ tree, I suspect that we need to find a better way.

Is there some kind of check we could run inside of Actions maybe that would validate that all of the changes were done via automation? Maybe checking out the tip of whatever PR it's running on, nuking all of the changes to external, regenerating them, and then comparing the two to ensure that they are tree-same?

@dscho
Copy link
Member Author

dscho commented Sep 26, 2024

Is there some kind of check we could run inside of Actions maybe that would validate that all of the changes were done via automation?

We could do that. It would be similar in spirit to the force-rebuild toggle of the manually-triggered workflows.

But then, why not simply use them workflows? Like, accept a PR (where the contributor verified in their own fork that things work as expected after running the scripts to re-render the pages), and then manually trigger the appropriate workflow with the force-rebuild toggle turned on.

And yes, we could add some automation that warns PR contributors if their changes touch auto-generated files. But I feel that's not welcoming. It is much nicer for contributors when a human comes in, thanks them for noticing an issue and doing something about it, and then guides them to the code where things need to be fixed.

@ttaylorr
Copy link
Member

But then, why not simply use them workflows? Like, accept a PR (where the contributor verified in their own fork that things work as expected after running the scripts to re-render the pages), and then manually trigger the appropriate workflow with the force-rebuild toggle turned on.

That sounds like a strict improvement to me: have contributors touch the non-generated parts of the repository only, and then let some Actions job rebuilt the generated components, failing the build if a human contributor modified the generated sources.

But I feel that's not welcoming. It is much nicer for contributors when a human comes in [...]

I don't think that one excludes the other here necessarily. I could imagine for instance a contributor mistakenly modifying some generated source, having the CI run fail, and then having someone with write access to the git-scm.com repo come in and explain in a friendly way what happened, where the right place to make the change is, etc.

But I certainly don't feel strongly about it one way or another. So if you think the idea is junk, feel free to ignore it ;-).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants