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

Do not colorize commit logs #120

Merged
merged 2 commits into from
Dec 3, 2024
Merged

Do not colorize commit logs #120

merged 2 commits into from
Dec 3, 2024

Conversation

MathisMARION
Copy link
Contributor

Patch e-mails typically include a commit log as a header, which may contain arbitrary text including patterns that highlight.js can interpret as a diff. I have typically encountered this when a commit log line starts with a commandline option or a bullet point.

I am not sure if people actually send raw diffs without a commit log, so maybe this should be handled with a bit more care to handle these.

Patch e-mails typically include a commit log as a header, which may
contain arbitrary text including patterns that highlight.js can
interpret as a diff. I have typically encountered this when a commit
log line starts with a commandline option or a bullet point.
Copy link
Owner

@Qeole Qeole left a comment

Choose a reason for hiding this comment

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

Thanks! I've noticed the same issue, but it never annoyed me enough that I'd take the time to fix it. Thanks for working on it!

* man git-format-patch: The log message and the patch are
* separated by a line with a three-dash line.
*/
j = pre.childNodes[i].textContent.search(/^---$/m);
Copy link
Owner

Choose a reason for hiding this comment

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

I'm slightly concerned that this might be a bit large and catch other things than the git format-patch separator. I'm thinking that Markdown files can use ^---$ as separators, for example. On the other hand, if we end up colouring a raw Markdown file with diff syntax, it will be messed up anyway. So yeah, maybe we can do that and see if anyone complains that it break their patches.

@Qeole Qeole merged commit 55d70c8 into Qeole:master Dec 3, 2024
1 check passed
@Qeole
Copy link
Owner

Qeole commented Dec 17, 2024

Trying the add-on with this PR I noticed that we fail to colourise the patch for blocks where we don't have the --- delimiter. I think that if we don't have (j >= 0) after the for loop, we probably need to reset i to 0.

@Qeole
Copy link
Owner

Qeole commented Dec 17, 2024

Another comment: we're still highlighting list items when they're below the --- delimiter but separated from patch content by another delimiter, e.g. in:

This is v2 of my patch

I have a description here.
- Lots of details!
- Oh by the way GitHub colours these as well
---
Changes since v1:
- rework foobar
- These list items
- Will get colours with the add-on
---
 foobar | 1 +-
 1 file changed, 1 insertions(+), 1 deletions(-)

diff --git a/foobar b/foobar
index c55878bdd9dd..db53c8d7d112 100755
--- a/foobar
+++ b/foobar
@@ -37,10 +37,11 @@ this is my patch
 some random text
 more random text
 and even more
- remove this
+ add that
 lorem ipsum
 dolor sic amet
 blah

@MathisMARION
Copy link
Contributor Author

Trying the add-on with this PR I noticed that we fail to colourise the patch for blocks where we don't have the --- delimiter. I think that if we don't have (j >= 0) after the for loop, we probably need to reset i to 0.

That was one of my concerns with this patch: git format-patch always inserts a separator but I don't know if it is common to send raw diffs.

There is still an edge case with testing j: there might be no "---" before the diff, but one in the diff itself.

@MathisMARION
Copy link
Contributor Author

MathisMARION commented Dec 17, 2024

Another comment: we're still highlighting list items when they're below the --- delimiter but separated from patch content by another delimiter

We would have to copy the parser behavior from git am to handle all edge cases. I might look into it but I am not sure it is worth it.

@Qeole
Copy link
Owner

Qeole commented Dec 17, 2024

That was one of my concerns with this patch: git format-patch always inserts a separator but I don't know if it is common to send raw diffs.

I've stumbled on that with a patch that (I guess) the person copy-pasted to their email, without inserting the delimiter. But not highlighting without the --- means we also don't provide colours at all for any other format that would be not using git format-patch syntax, so this is something I want to fix.

As for the part of the patch between the two delimiters, I reported it to keep track of this, it doesn't mean we should fix it. I don't want another parser in the add-on: it would likely be fragile, wouldn't support other formats, and honestly I have no appetite for maintaining this.

By the way - I reported here because it was related to the PR, but it doesn't mean I'm expecting you to fix this 🙂. I'll look more into the first issue, eventually, unless you beat me to 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