beyondgrep / ack2

**ack 2 is no longer being maintained. ack 3 is the latest version.**
https://github.com/beyondgrep/ack3/
Other
1.48k stars 138 forks source link

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

Closed epa closed 7 years ago

petdance commented 9 years ago

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

epa commented 9 years ago

I will add some tests.

petdance commented 9 years ago

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 commented 9 years ago

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

petdance commented 9 years ago

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 commented 9 years ago

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

epa commented 9 years ago

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

epa commented 9 years ago

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

petdance commented 9 years ago

https://groups.google.com/forum/#!topic/ack-users/HVKAvkO4G3U

petdance commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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. https://github.com/petdance/ack2/issues/14 is an older bug covering the same issues. Contributor hoelzro reviewed an earlier version of the patch in https://github.com/petdance/ack2/issues/445

hoelzro commented 8 years ago

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

petdance commented 8 years ago

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 commented 8 years ago
$ echo must | ack -w mu.
must
$ echo must | grep -w mu.
(no output)
epa commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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

epa commented 8 years ago

Somewhat related: https://rt.perl.org/Public/Bug/Display.html?id=127670

petdance commented 7 years ago

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