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

Provides narrower regex for conflict marker detect #495

Merged
merged 2 commits into from
Jun 23, 2017
Merged

Provides narrower regex for conflict marker detect #495

merged 2 commits into from
Jun 23, 2017

Conversation

allenhumphreys
Copy link
Contributor

I recently ran into a situation when parsing the acknowledgements file within my Podfile. One of the pods (libidn v1.33) has the following in their license:

OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
THE SOFTWARE.

===============================================

Libidn COPYING -- Explanation of licensing conditions.
Copyright (C) 2002-2015 Simon Josefsson

The regex currently used in file_in_conflict is detecting this section separator as a merge conflict, thus Plist.read_from_path is refusing to open the file.

I'm proposing a new regex for matching the entire conflict marker sequence which is:

  1. beginning of line, followed by exactly 7 left angle brackets
  2. any characters
  3. beginning of line followed by exactly 7 equality symbols
  4. any characters
  5. beginning of line followed by exactly 7 right angle brackets

A trivial script for testing the regex

#!/usr/bin/env ruby

def file_in_conflict?(contents)

    contents.match(/^<{7}(?!<)[\w\W]*^={7}(?!=)[\w\W]*^>{7}(?!>)/m)
end

conflict_example = "<<<<<<< HEAD
words and stuff go here
=======
other, different words go here
>>>>>>> 8ab358aeba3f
"

if file_in_conflict?(conflict_example)
    puts "This is in conflict" # Expect this
else
    puts "This is not in conflict"
end

non_conflict_example = "<<<<<<<<
words and stuff go here
========
other, different words go here
>>>>>>>>
"

if file_in_conflict?(non_conflict_example)
    puts "This is in conflict"
else
    puts "This is not in conflict" # Expect this
end

@segiddins
Copy link
Member

Can you please break the regexp over multiple lines and add inline comments to it?
Looks 💯 with added test coverage

[\w\W]* # Anything
^={7}(?!=) # Exactly 7 equality symbols at the beginning of the line
[\w\W]* # Anything
^>{7}(?!>) # Exaxtly 7 right arrows at the beginning of the line
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, typo exactly

@allenhumphreys
Copy link
Contributor Author

@segiddins I'm not sure why the CI is failing. Reading the output it seems to be a setup issue, but I'm new to ruby/gem/rake/allofit. Is it something I need to address?

@segiddins
Copy link
Member

segiddins commented Jun 6, 2017

I don't know, @orta is the one who set up the danger integration, so I don't know why it's stopped working. we're tracking that over at #493

@dnkoutso
Copy link
Contributor

@allenhumphreys can you try rebasing?

Improves readability of conflict regex, adds test cases

Addresses style guide conformance

Fixes typo
@allenhumphreys
Copy link
Contributor Author

@dnkoutso Squashed and rebase on master. ✅

@dnkoutso
Copy link
Contributor

@allenhumphreys as the barista says, need to add a changelog entry ;)

@segiddins
Copy link
Member

👍 Thanks!

@dnkoutso
Copy link
Contributor

Looks like rebase did it. :shipit:

@allenhumphreys
Copy link
Contributor Author

@segiddins @dnkoutso It's been a pleasure learning and participating, hope to contribute more in the future!

@segiddins segiddins merged commit aec00f1 into CocoaPods:master Jun 23, 2017
@allenhumphreys
Copy link
Contributor Author

Apparently, in the changelog, the 2 trailing spaces at the end of the first line must mean something in markdown that I was unfamiliar with. So, that's my bad.

@segiddins
Copy link
Member

It just forces GitHub to render the next line of markdown on a new line, no big deal, we can fix it up during release

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.

3 participants