Skip to content

Conversation

@incase
Copy link
Contributor

@incase incase commented Sep 7, 2017

Otherwise this is over-matching (matches anything between ASCII A and ASCII z, which is much more than just characters).

Otherwise this is over-matching (matches anything between ASCII A and ASCII z, which is much more than just characters).
@incase
Copy link
Contributor Author

incase commented Sep 7, 2017

Note: Send a matching pull request to upstream for pathspec: highb/pathspec-ruby#16

@hlindberg
Copy link
Contributor

@incase Thanks for your contribution!

It would help a lot if you could do the following:

  • File a ticket for the PUP project that describes the problem (that helps us write a release note among other things)
  • Make the first line of the commit message reference the PUP ticket, for example (PUP-1234)

Example commit message (PUP-1234) Fix problem of matching too much in pathspec with A-z regexp

This is needed as we match up all commits with tickets at release time.

@puppetcla
Copy link

CLA signed by all contributors.

@incase incase changed the title Fix capitalization of a regex (A-Z not A-z) (PUP-7926) Fix capitalization of a regex (A-Z not A-z) Sep 8, 2017
@incase
Copy link
Contributor Author

incase commented Sep 8, 2017

Ticket filed, commit message updated.

@hlindberg hlindberg requested a review from Iristyle September 10, 2017 17:34
Copy link
Contributor

@hlindberg hlindberg left a comment

Choose a reason for hiding this comment

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

Looks ok and legit to me, ping @Iristyle since this is about windows drive letters...

@hlindberg
Copy link
Contributor

@incase thanks for logging a ticket - you still have the commit itself to deal with (that is why the travis tests shows a failure). The commit message should have the same form as the title of this PR - i.e. (PUP-7926) Fix....

@Iristyle
Copy link
Contributor

I'm not even sure where match_path is used (it doesn't appear to be called in the Puppet code anywhere) - but perhaps 3rd parties are consuming it in tests?

Anyhow, the fix looks good to me, as long as we don't want to delete the code altogether.

@hlindberg
Copy link
Contributor

@Iristyle in that case we need a major version bump (don't think this was marked as private). Merging this, it is at least better than before this fix.

@hlindberg
Copy link
Contributor

@incase Can you fix the message of the commit so that it conforms to the policy? That is the only thing that stops this from being merged.

@incase
Copy link
Contributor Author

incase commented Sep 12, 2017

Now, if I knew how to add either add a commit to this and remove the old one, or how to edit the commit message on the existing change, I would be happy to do it. But I purely used the web interface to create this and don't know anything useful about the commandline client and don't have a local client. So it seems the most viable path for me would be to create a new pull request with a fixed commit?

(PUP-7926) Fix capitalization of a regex (A-Z not A-z)
@incase
Copy link
Contributor Author

incase commented Sep 17, 2017

After all I tried, I only ended up polluting this pull request with stuff that doesn't belong here.
I'll withdraw this one and create a new one.

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.

4 participants