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

Fix -w behaviour and docs (<https://github.com/petdance/ack2/issues/445>) #558

Closed
wants to merge 1 commit into from

Conversation

epa
Copy link

@epa epa commented Jun 16, 2015

No description provided.

@petdance
Copy link
Collaborator

Thanks for the patch. Something of this magnitude needs some severe test coverage before I can even think of merging it in.

@epa
Copy link
Author

epa commented Jun 16, 2015

I will add some tests.

@petdance
Copy link
Collaborator

I'm also concerned about any potential speed degradations.

The change you're suggesting here needs to be discussed on the ack-users mailing list because it is potentially a huge breakage of existing behavior. The new -w may be "fixed", but it is changing existing behavior and we have to be very very very careful with that.

@epa
Copy link
Author

epa commented Jun 16, 2015

Would you prefer that I first document and test the existing behaviour, and then we can decide whether to change it?

@petdance
Copy link
Collaborator

I think that makes a lot of sense. If anything is going to be changed, then there will have to be a clear explanation of that change in the Changes file for any future users.

@epa
Copy link
Author

epa commented Jun 16, 2015

I've sent a pull request which documents and tests the current semantics.

@epa
Copy link
Author

epa commented Jun 20, 2015

I've posted on ack-user asking about this proposed change to -w semantics.

@epa
Copy link
Author

epa commented Jun 24, 2015

There haven't been any comments on ack-users. What is your opinion on the proposed change?

@petdance
Copy link
Collaborator

@petdance
Copy link
Collaborator

I've only skimmed it. First thing that comes to mind is that we can't change anything related to parentheses until we get the highlighting bug fixed.

@epa
Copy link
Author

epa commented Jun 25, 2015

I don't think this is really 'related to parentheses'; the parens are just one example test case that demonstrates the bug (where -w fails to require a whole word match). Here is another test case:

mkdir test; cd test
for i in box ox oxen; do echo $i >$i; done
ack -w '\w\w'

This should match a two-letter whole word, but it ends up matching all three strings. Or indeed

ack -w '[oa]x'

should match the whole words 'ox' and 'ax', but finds the not-whole-word match inside 'box' too.

@epa
Copy link
Author

epa commented Jul 9, 2015

Do you have time for a closer look at the fix? As I mentioned it is not related to parentheses or any other particular regexp feature; it fixes the problem that if the given regexp doesn't begin with a word character then the -w flag fails to work. #14 is an older bug covering the same issues. Contributor hoelzro reviewed an earlier version of the patch in #445

@hoelzro
Copy link
Collaborator

hoelzro commented Jan 6, 2016

@petdance What are your thoughts on this moving forward? No one on ack-users seems to mind that this would change.

@petdance
Copy link
Collaborator

petdance commented Jan 6, 2016

How does grep handle this? If you grep -w mu., what behavior do you get?

Test cases to consider:

grep -w mu
grep -w mu.
grep -w mu.*
grep -w mu.+
grep -w .mu.
grep -w (mu)

@hoelzro
Copy link
Collaborator

hoelzro commented Jan 6, 2016

$ echo must | ack -w mu.
must
$ echo must | grep -w mu.
(no output)

@epa
Copy link
Author

epa commented Jan 6, 2016

As far as I know grep handles it correctly, in that the -w causes it to match whole words (and doesn't depend on whether the regexp given begins or ends with a word character).

% mkdir test
% cd test
% echo mu >>t
% echo mux >>t
% echo muxy >>t
% grep -w mu. t
mux

@petdance
Copy link
Collaborator

petdance commented Jan 6, 2016

@epa: We need to stick to facts and not use words like "correct" or "wrong", because that's an opinion, and everyone has an opinion on things but they can get confusing.

@epa
Copy link
Author

epa commented Jan 6, 2016

I do agree, which is why I clarified exactly what I meant by 'correct' in this case.

@epa
Copy link
Author

epa commented Mar 8, 2016

@petdance
Copy link
Collaborator

The behavior of -w is not going to change in ack2. It is updated in ack3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants