Re: [PATCH 1/3] test-diff-suppr: Improve regexes in tests.

[Dodji Seketeli <dodji@seketeli.org>]

Hello Giuliano,

Giuliano Procida <gprocida@google.com> a écrit:

> The are some (mostly inconsequential) issues with regexes:
>
>   - Leading and trailing .* are redundant and can be removed.

They are redundant I agree, but that redundancy can be useful, I think.
The reason why I have put them in the test is that in the past, I have
tried different regular expression engines.  Some of them would
implement the quantifier in a greedy manner, rather than in a lazy
manner.  Whereas, what I find more useful is the lazy variant.  So that
writing ".*blah" would do what you would expect (rather than having to
write .*?blah)

So putting that in those tests explicitly will hopefully help us catch
potential issues if we are to switch to a different engine tomorrow.  So
I'd prefer keeping that redundancy in those tests.

[...]

>   - File name matches are full path

Are they?

It seems to me file_suppression::suppress_file() acts on the base name
of the file, rather than on its full path.

I might be missing something here, please tell me.

> so should start with (^|/) if trying to match a base name, assuming a
> Unix-like filesystem.

I'd need the above to be clarified to say for sure.

>     Given these are just tests, it's not that important, but they
>     still serve as examples.
>     - In cases where the ^ anchor was used, full paths would usually
>       fail to match. In such cases, the regex was being ignored for
>       other reasons (see later patch) or is expected not to match
>       anyway.
>     - In many cases, the $ anchor could be considered to be missing.
>   - The .ini parser unescapes string values, so escaping regex
>     metacharacters requires a double backslash. Single backslashes
>     are pointless.
>   - The dot metacharacter is used unescaped in a few places where a
>     literal was likely intended, so should be escaped.
>   - The characters [ and ] don't need to be (.ini) escaped.

They don't *need* to (in a string) I agree.  But it they are escaped,
that should work.  It's a way to test the init parser as well.  So I'd
keep the redundancy there as well.

[...]

Cheers,

-- 
		Dodji