← 返回首页
Check for diff3-style merge conflict artifacts. by christhekeele · Pull Request #586 · pre-commit/pre-commit-hooks · GitHub
Skip to content

Navigation Menu

Toggle navigation
Sign in
Appearance settings
Search or jump to...

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Include my email address so I can be contacted

Saved searches

Use saved searches to filter your results more quickly

Appearance settings
Resetting focus

Check for diff3-style merge conflict artifacts.#586

Open
christhekeele wants to merge 2 commits into
pre-commit:mainfrom
christhekeele:check-diff3-conflict-artifacts
Open

Check for diff3-style merge conflict artifacts.#586
christhekeele wants to merge 2 commits into
pre-commit:mainfrom
christhekeele:check-diff3-conflict-artifacts

Conversation

Copy link
Copy Markdown

I noticed that the test suite checks for diff3-style merge conflicts, but the actual hook does not. Seems like an oversight?

CONFLICT_PATTERNS = [
b'<<<<<<< ',
b'======= ',
b'||||||| ',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

would you like to add a failing test for this? the testsuite passes because that particular string is part of a larger string which contains the other strings

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

Sure! Think I can get to that later tonight

Copy link
Copy Markdown
Author

christhekeele Apr 11, 2021
edited
Loading

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

@asottile I gave this a go, but am encountering some friction with running tests locally (test dependencies are missing from requirements-dev.test, at least ruamel.yaml, and I'm getting unexplicable tmpfile errors). The build is also telling me that there is something incorrect about either my understanding of the how the tests work. So not mergable yet, and I'm not pursuing for now, but leaving this open for others!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

I think all you need to do is add to this list: https://github.com/pre-commit/pre-commit-hooks/pull/586/files#diff-6224b62678ea600729955336d2e3f48b0c90be48e8ea57dbefe54e7c00131532R106

are you using tox when testing locally?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

A bit confused there, that commit... is where I added to that list? Then CI fails on this test that I'd expect to succeed.

I've never used tox before but I figured it out (tho still have odd tmpfile-caused test failures). Some dev-instruction that you should be using it, and how to use, may help with future contributors.

Speaking of which, thanks for the help!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality Hide comment

yeah you shouldn't need to touch that test -- it's testing a different behaviour (when --assume-in-merge is passed it should check for those strings) -- I think you can revert your test changes and just add to the tests on line 106

christhekeele force-pushed the check-diff3-conflict-artifacts branch from 618e847 to 0c6a81a Compare April 11, 2021 09:49
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Footer

© 2026 GitHub, Inc.