Re: [PATCH] t2400: Fix test failures when using grep 2.5

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jul 15, 2023, 08:59 UTC
Message-ID: <2e22a23f-576f-7a42-ace8-624a5362d9f4@gmail.com>
In-Reply-To: <20230715025512.7574-1-jacobabel@nullpo.dev>

Hi Jocab

On 15/07/2023 03:55, Jacob Abel wrote:

Show 7 quoted lines

> Replace all cases of `\s` with `[[:space:]]` as older versions of GNU
> grep (and from what it seems most versions of BSD grep) do not handle
> `\s`.
>
> For the same reason all cases of `\S` are replaced with `[^[:space:]]`.
> Replacing `\S` also needs to occur as `\S` is technically PCRE and not
> part of ERE even though most modern versions of grep accept it as ERE.

Thanks for working on this fix. Having looked at the changes I think it would be better just be using a space character in a lot of these expressions - see below.

Show 26 quoted lines

> Signed-off-by: Jacob Abel <jacobabel@nullpo.dev>
> ---
> This patch is in response to build failures on GGG's Cirrus CI
> freebsd_12 build jobs[1] and was prompted by a discussion thread [2].
> 
> These failures seem to be caused by the behavior outlined in [3].
> Weirdly however they only seem to occur on the FreeBSD CI but not the
> Mac OS CI for some reason despite Mac OS using FreeBSD grep.
> 
> 1. https://github.com/gitgitgadget/git/pull/1550/checks?check_run_id=14949695859
> 2. https://lore.kernel.org/git/CALnO6CDryTsguLshcQxx97ZxyY42Twu2hC2y1bLOsS-9zbqXMA@mail.gmail.com/
> 3. https://stackoverflow.com/questions/4233159/grep-regex-whitespace-behavior
> 
>   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 0ac468e69e..7f19bdabff 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:[[:space:]]+git worktree add --orphan -b [^[:space:]]+ [^[:space:]]+[[:space:]]*$" actual

We know that "hint:" is followed by a single space and all we're really interested in is that we print something after the "-b " so we can simplify this to

	grep "^hint: git worktree add --orphan -b [^ ]"

I think the same applies to most of the other expressions changed in this patch.

Show 17 quoted lines

>   		else
> -			grep -E "^hint:\s+git worktree add --orphan \S+\s*$" actual
> +			grep -E "^hint:[[:space:]]+git worktree add --orphan [^[:space:]]+[[:space:]]*$" actual
>   		fi
>   
>   	'
> @@ -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:[[:space:]]\+.*" &&
>   	local bad_combo_regex="^fatal: '[a-z-]\+' and '[a-z-]\+' cannot be used together" &&
>   
>   	local git_ns="repo" &&
> @@ -998,8 +998,8 @@ test_dwim_orphan () {
>   					headpath=$(git $dashc_args rev-parse --sq --path-format=absolute --git-path HEAD) &&

I'm a bit confused by the --sq here - why does it need to be shell quoted when it is always used inside double quotes? Also when the reftable backend is used I'm not sure that HEAD is actually a file in $GIT_DIR anymore (that's less of an issue at the moment as that backend is not is use yet).

Show 6 quoted lines

>   					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:[[:space:]]*.$headpath." actual &&
> +					grep "HEAD contents:[[:space:]]*.$headcontents." actual &&

Using grep like this makes it harder to debug test failures as one has to run the test with "-x" in order to try and figure out which grep actually failed. I think here we can replace the sequence of "grep"s with "test_cmp"

	cat >expect <<-EOF &&
	HEAD points to an invalid (or orphaned) reference
	HEAD path: $headpath
	HEAD contents: $headcontents
	EOF

	test_cmp expect actual

Best Wishes

Phillip

Show 5 quoted lines

>   					grep "$orphan_hint" actual &&
>   					! grep "$info_text" actual
>   				fi &&
> 
> base-commit: 830b4a04c45bf0a6db26defe02ed1f490acd18ee