Re: [PATCH v3 3/3] t2400: rewrite regex to avoid unintentional PCRE

From: Jacob Abel <jacobabel@nullpo.dev>
Date: Jul 22, 2023, 02:36 UTC
Message-ID: <axnxvnmo6ekhhccppinji73ivlandwuqs44epmq4pdefm7ukiv@ejz7bee5xjli>
In-Reply-To: <xmqqv8edwb0k.fsf@gitster.g>

On 23/07/21 08:16AM, Junio C Hamano wrote:

Show 7 quoted lines

> Jacob Abel <jacobabel@nullpo.dev> writes:
> 
> > Replace all cases of `\s` with ` ` as it is not part of POSIX BRE or ERE
> > and therefore not all versions of grep handle it without PCRE support.
> 
> Good point.  But the patch replaces them with "[ ]" instead, which
> probably is not a good idea for readability.

Using `[ ]` over ` ` is just a personal thing I picked up to keep myself from forgetting the space was intentional. I can see how that can come across as confusing though so I'll make sure to update that.

Show 7 quoted lines

> Technically speaking, there is no regular expression library that
> supports PCRE per-se; treating \S, \s, \d and the like the same way
> as PCRE is a GNU extension in the glibc land, and a simlar "enhanced
> mode" can be requested by passing REG_ENHANCED bit to regcomp(3) at
> runtime in the BSD land including macOS.  I would suggest just
> dropping "without PCRE support" for brevity, as "not all versions of
> grep handle it" is sufficient here.

Good point. Will do.

Show 5 quoted lines

> [...]
> 
> Just a single space would be fine without [bracket].  I think older
> tests use (literally) HT and SP inside [], many of them may still
> survive.

Noted.

Show 10 quoted lines

> > @@ -709,7 +709,7 @@ test_dwim_orphan () {
> >  	local info_text="No possible source branch, inferring '--orphan'" &&
> >  	local fetch_error_text="fatal: No local or remote refs exist despite at least one remote" &&
> >  	local orphan_hint="hint: If you meant to create a worktree containing a new orphan branch" &&
> > -	local invalid_ref_regex="^fatal: invalid reference:\s\+.*" &&
> > +	local invalid_ref_regex="^fatal: invalid reference: .*" &&
> 
> Feeding "<something>\+" to BRE (this pattern is later used with
> 'grep' but not with 'egrep' or 'grep -E') and expecting it to mean 1
> or more is a GNU extension,

Oh it is. I've really gotta reread the chapters of the POSIX standard on regex again.

Show 6 quoted lines

> and in this case "there must be a SP
> after colon" is much easier to see, which is what the updated one
> uses.  Good.
> 
> By the way, you can drop the ".*" at the end of the pattern, because
> the match is not anchored at the tail end.

Understood.

Show 11 quoted lines

> >  	local bad_combo_regex="^fatal: '[a-z-]\+' and '[a-z-]\+' cannot be used together" &&
> 
> This should also be corrected, I think.
> 
> 	"fatal: '[a-z-]\{1,\}' and '[a-z-]\{1,\}' cannot be used together"
> 
> or even simpler,
> 
> 	"fatal: '[a-z-]*' and '[a-z-]*' cannot be used together"
> 
> to avoid \+ in BRE (see above).

I definitely prefer the latter so I'll update it to use that one.

Show 5 quoted lines

> "[-a-z]" (to show '-' at the
> beginning) may make it easier to read by letting the hyphen-minus
> stand out more, as we know we are giving two command line option
> names and in a command line option name, the first letter is always
> hyphen-minus.  But that is more of personal taste, not correctness.

Certainly a matter of personal preference but I can see why this could be preferable so I'll update it to this.

> [...]