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.

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

Check for diff3-style merge conflict artifacts. #586

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Check for diff3-style merge conflict artifacts. #586

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8993f10
Check for diff3-style merge conflict artifacts.
christhekeele Apr 10, 2021
0c6a81a
Add negative test cases for various git artifacts.
christhekeele Apr 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pre_commit_hooks/check_merge_conflict.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
CONFLICT_PATTERNS = [
b'<<<<<<< ',
b'======= ',
b'||||||| ',
Copy link
Member

Choose a reason for hiding this comment

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

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
Author

Choose a reason for hiding this comment

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

Sure! Think I can get to that later tonight

Copy link
Author

@christhekeele 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.

@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
Member

Choose a reason for hiding this comment

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

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
Author

Choose a reason for hiding this comment

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

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
Member

Choose a reason for hiding this comment

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

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

b'=======\n',
b'>>>>>>> ',
]
Expand Down