On Tue, Jan 17 2023, Carlo Arenas wrote: > On Tue, Jan 17, 2023 at 7:19 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: >> >> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: >> >> > To argue with myself here, I'm not so sure that just making this the >> > default isn't the right move, especially as the GNU grep maintainer >> > seems to be convinced that that's the right thing for grep(1). >> >> OK. > > I think that is definitely the right thing to do for grep, because the > current behaviour can only be described as a bug (and a bad one at > it), but after all the push back and performance testing, I am also > not convinced anymore it needs to be the default for git, because the > negatives outweigh the positives. > > First there is the performance hit, which is inevitable because there > are just a lot more characters to match when UCP tables are being > used,[...] I'm less concerned about the performance, we should aim for correctness first. We can always provide an opt-out (and the locale setting is already that opt-out). > and second there is the fact that PCRE2_UCP itself might not be > what you want when matching code, because for example numbers are > never going to be using digits outside what ASCII provides, and > identifiers have a narrow set of characters as valid than what you > would expect from all written human languages in history. [0-9] will be ASCII, but \d will use [^0-9] Unicode numbers. I agree it might not be expected by some, but I can't really square that view in my mind with the desire to match "\bÆvar" :). After all that "Æ" is also arbitrary byte garbage in the ASCII-view of the world. I can see how it might be more practical in some cases to have "\b" have Unicode semantics, but to specifically make "\d" an exception. But the ship has sailed on that in Perl & PCRE land years (or more than a decade ago). I think us coming up with some exception to that would probably suck more than going with their behavior. > Lastly, even with PCRE2_UCP enabled, our current logic for word > matches is still broken, because the current code still uses a > definition of word that was done outside what the regex engines > provide and that roughly matches what you would expect of identifiers > from C in the ASCII times. Yes, FWIW I have some WIP patches somewhere to get rid of that bit of grep.c if we're using PCRE. I.e. the "-w" should be powered by just adding "\b" to the start/end of the provided string. That'll then be correct, and faster. I can't remember if there were some subtle bugs in that, or why I didn't finish that... >> > Of course all of this is predicated on us wanting to leave this as an >> > opt-in, which I'm not so sure about. If it's opt-out we'll avoid this >> > entire question, >> >> Making it opt-out would also require a similar knob to turn the >> "flag" off, be it a configuration variable or a command line option, >> wouldn't it? I tend to agree with you that it makes sense to make >> it a goal to take us closer to "grep -P" from GNU---do they have >> such an opt-out knob? If not, let's make it simple by turning it >> always on, which would be the simplest ;-) > > GNU grep -P has no knob and would likely never have one. I think the general knob in not just GNU grep but GNU utils and the wider *nix landscape is "tweak your LC_ALL and/or other locale varibales". Which works for it, and will work for us once we're using PCRE2_UCP too. > So for now, I think we should acknowledge the bug, provide an option > for people that might need the fix, and fix all other problems we > have, which will include changes in PCRE2 as well to better fit our > use case. Hrm, what are those PCRE2 changes? The one I saw so far (or was it a proposal) was to just make its "grep" utility use the PCRE2_UCP like GNU grep is now doing in its unreleased version in its git repo...
