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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 21, 2023, 15:16 UTC
Message-ID: <xmqqv8edwb0k.fsf@gitster.g>
In-Reply-To: <20230721044012.24360-4-jacobabel@nullpo.dev>

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.

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.

> For the same reason all cases of `\S` are replaced with `[^ ]`.
> It's not an exact replacement but it is close enough for this
> use case.

Good.

Show 19 quoted lines

> Signed-off-by: Jacob Abel <jacobabel@nullpo.dev>
> ---
>  t/t2400-worktree-add.sh | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> index e106540c6d..eafecdf7ce 100755
> --- a/t/t2400-worktree-add.sh
> +++ b/t/t2400-worktree-add.sh
> @@ -417,9 +417,9 @@ test_wt_add_orphan_hint () {
>  		grep "hint: If you meant to create a worktree containing a new orphan branch" actual &&
>  		if [ $use_branch -eq 1 ]
>  		then
> -			grep -E "^hint:\s+git worktree add --orphan -b \S+ \S+\s*$" actual
> +			grep -E "^hint:[ ]+git worktree add --orphan -b [^ ]+ [^ ]+$" actual
>  		else
> -			grep -E "^hint:\s+git worktree add --orphan \S+\s*$" actual
> +			grep -E "^hint:[ ]+git worktree add --orphan [^ ]+$" actual
>  		fi

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.

Show 6 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, 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.

>  	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). "[-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.

Show 11 quoted lines

> @@ -998,8 +998,8 @@ test_dwim_orphan () {
>  					headpath=$(git $dashc_args rev-parse --path-format=absolute --git-path HEAD) &&
>  					headcontents=$(cat "$headpath") &&
>  					grep "HEAD points to an invalid (or orphaned) reference" actual &&
> -					grep "HEAD path:\s*.$headpath." actual &&
> -					grep "HEAD contents:\s*.$headcontents." actual &&
> +					grep "HEAD path: .$headpath." actual &&
> +					grep "HEAD contents: .$headcontents." actual &&
>  					grep "$orphan_hint" actual &&
>  					! grep "$info_text" actual
>  				fi &&

Thanks.