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

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

[Jacob Abel]

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.

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
 		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) &&
 					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 &&
 					grep "$orphan_hint" actual &&
 					! grep "$info_text" actual
 				fi &&

base-commit: 830b4a04c45bf0a6db26defe02ed1f490acd18ee
-- 
2.39.3

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

[Phillip Wood]

Hi Jocab

On 15/07/2023 03:55, Jacob Abel wrote:
> 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.

> 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.

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

>   					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

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

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

[Jacob Abel]

On 23/07/15 09:59AM, Phillip Wood wrote:
> Hi Jocab
> 
> On 15/07/2023 03:55, Jacob Abel wrote:
> > [...]
> 
> 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.
> 
> > [...]
> >
> > -			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.

This wouldn't work as it's `hint: ` followed by a `\t` as the command
is indented in the text block. So I just went with `[[:space:]]+` as I
didn't want to have to worry about whether some platforms expand the
tab to spaces or how many spaces. I'll make the rest of the suggested
changes though.

> > [...]
> > @@ -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? 

To be honest I can't remember if this specifically needs to be in
quotes or not however I had a lot of trouble during the development of
that patchset with things escaping quotes and causing breakages in the
tests so if it isn't currently harmful I'd personally prefer to leave
it as is.

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

If there is documentation (or discussions) on how to use this backend
properly I'd appreciate a link and I can try workshopping a better
solution then. The warning included in the original patchset reads
from that HEAD file as well so it would also need to be adapted. 

The reason I did it this way is because I didn't see any easy way to
get the raw contents of the HEAD when it was invalid. If there is a
cleaner/safer/more portable way to view those contents when HEAD
points to an invalid or unborn reference, I'd be willing to work on a
followup patch down the line.

> > [...]
> 
> 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

I'll make these changes.

> [...]

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

[Junio C Hamano]

Jacob Abel <jacobabel@nullpo.dev> writes:

>> > @@ -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? 
>
> To be honest I can't remember if this specifically needs to be in
> quotes or not however I had a lot of trouble during the development of
> that patchset with things escaping quotes and causing breakages in the
> tests so if it isn't currently harmful I'd personally prefer to leave
> it as is.

Quoting is sometimes tricky enough that "this happens to work for me
but I do not know why it works" is asking for trouble in somebody
else's environment.  If the form in the patch is correct, but tricky
for others to understand, you'd need to pick it apart and document
how it works (and if you cannot do so, ask for help by somebody who
can, or simplify it enough so that you can explain it yourself).

    headpath=$(git $dashc_args rev-parse --sq --path-format=absolute --git-path HEAD) &&

In this case, "--sq" is a noop that only confuses readers, I think,
and I would drop it if I were you.  "--git-path HEAD" is given by
this call chain:

   builtin/rev-parse.c:cmd_rev_parse() 
   -> builtin/rev-parse.c:print_path()
      -> transform path depending on the path format
         -> puts()

and nowhere in this chain "output_sq" (which is set by "--sq") is
even checked.  The transformations are all about relative, prefix,
etc., and never about quoting.

The original test script t2400 (before your patch) does look crappy
with full of long lines and coding style violations (none of which
is your fault), and it may need to be cleaned up once this patch
settles.

Thanks.

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

[Jacob Abel]

On 23/07/15 06:08PM, Junio C Hamano wrote:
> Jacob Abel <jacobabel@nullpo.dev> writes:
> 
> >> > @@ -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?
> >
> > To be honest I can't remember if this specifically needs to be in
> > quotes or not however I had a lot of trouble during the development of
> > that patchset with things escaping quotes and causing breakages in the
> > tests so if it isn't currently harmful I'd personally prefer to leave
> > it as is.
> 
> Quoting is sometimes tricky enough that "this happens to work for me
> but I do not know why it works" is asking for trouble in somebody
> else's environment.  If the form in the patch is correct, but tricky
> for others to understand, you'd need to pick it apart and document
> how it works (and if you cannot do so, ask for help by somebody who
> can, or simplify it enough so that you can explain it yourself).

Yes Apologies. That was kind of a cop-out on my part as I was hesitant
to add additional changes that could potentially introduce new issues
to this patch as it is already addressing a fairly obscure issue.

> 
>     headpath=$(git $dashc_args rev-parse --sq --path-format=absolute --git-path HEAD) &&
> 
> In this case, "--sq" is a noop that only confuses readers, I think,
> and I would drop it if I were you.  "--git-path HEAD" is given by
> this call chain:
> 
>    builtin/rev-parse.c:cmd_rev_parse()
>    -> builtin/rev-parse.c:print_path()
>       -> transform path depending on the path format
>          -> puts()
> 
> and nowhere in this chain "output_sq" (which is set by "--sq") is
> even checked.  The transformations are all about relative, prefix,
> etc., and never about quoting.

Understood. I tried running it with `--sq` removed and it seems to
work as you and Phillip expected so I'm adding that to v2.

> 
> The original test script t2400 (before your patch) does look crappy
> with full of long lines and coding style violations (none of which
> is your fault), and it may need to be cleaned up once this patch
> settles.
> 
> Thanks.

I may give that cleanup a shot some time down the line if nobody else
takes a crack at it first.

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

[Phillip Wood]

Hi Jacob

On 16/07/2023 00:15, Jacob Abel wrote:
> On 23/07/15 09:59AM, Phillip Wood wrote:
>> Hi Jocab
>>
>> On 15/07/2023 03:55, Jacob Abel wrote:
>>> [...]
>>
>> 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.

One thing I forgot to mention was that I think it would be better to 
explain in the commit message that "\s" etc. are not part of POSIX EREs 
and that is why they do not work.

>>> [...]
>>>
>>> -			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.
> 
> This wouldn't work as it's `hint: ` followed by a `\t` as the command
> is indented in the text block.

Oh so we need to search for a space followed by a tab after "hint:" 
then. As an aside we often just use four spaces to indent commands in 
advice messages (see the output of git -C .. grep '"    git' \*.c)

> So I just went with `[[:space:]]+` as I
> didn't want to have to worry about whether some platforms expand the
> tab to spaces or how many spaces.

Is that a thing?

> I'll make the rest of the suggested
> changes though.
> 
>>> [...]
>>> @@ -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?
> 
> To be honest I can't remember if this specifically needs to be in
> quotes or not however I had a lot of trouble during the development of
> that patchset with things escaping quotes and causing breakages in the
> tests so if it isn't currently harmful I'd personally prefer to leave
> it as is.
> 
>> 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).
> 
> If there is documentation (or discussions) on how to use this backend
> properly I'd appreciate a link and I can try workshopping a better
> solution then. The warning included in the original patchset reads
> from that HEAD file as well so it would also need to be adapted.

I'm afraid I don't have anything specific, there were some patches a 
while ago such as dd8468ef00 (t5601: read HEAD using rev-parse, 
2021-05-31) that stopped reading HEAD from the filesystem.

> The reason I did it this way is because I didn't see any easy way to
> get the raw contents of the HEAD when it was invalid. If there is a
> cleaner/safer/more portable way to view those contents when HEAD
> points to an invalid or unborn reference, I'd be willing to work on a
> followup patch down the line.

I think it might be better to just diagnose if HEAD is a dangling 
symbolic-ref or contains an invalid oid and leave it at that. See the 
documentation in refs.h for refs_resolve_ref_unsafe() for how to check 
if HEAD is a dangling symbolic ref - if rego_get_oid(repo, "HEAD") fails 
and it is not a dangling symbolic ref then it contains an invalid oid.

Best Wishes

Phillip

>>> [...]
>>
>> 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
> 
> I'll make these changes.
> 
>> [...]
> 

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

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> One thing I forgot to mention was that I think it would be better to
> explain in the commit message that "\s" etc. are not part of POSIX
> EREs and that is why they do not work.

Yes, that is a very good point.  We have been burned by regular
expression implementations that use or do not use "enhanced" bit in
the recent past, IIRC.

> I think it might be better to just diagnose if HEAD is a dangling
> symbolic-ref or contains an invalid oid and leave it at that. See the
> documentation in refs.h for refs_resolve_ref_unsafe() for how to check
> if HEAD is a dangling symbolic ref - if rego_get_oid(repo, "HEAD")
> fails and it is not a dangling symbolic ref then it contains an
> invalid oid.

Sounds doable and sensible.  If we can easily test it without
peeking into the filesystem, that would be very good.

Thanks.

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

[Jacob Abel]

On 23/07/16 04:34PM, Phillip Wood wrote:
> Hi Jacob
> 
> [...]
> 
> One thing I forgot to mention was that I think it would be better to
> explain in the commit message that "\s" etc. are not part of POSIX EREs
> and that is why they do not work.

Noted. Will do.

> [...]
> 
> Oh so we need to search for a space followed by a tab after "hint:"
> then. 

Okay. I think `\t` is PCRE so I'll just update the string in 
`builtin/worktree.c` so we can just do `[ ]+` instead. 

> As an aside we often just use four spaces to indent commands in
> advice messages (see the output of git -C .. grep '"    git' \*.c)

Apologies. When writing up that original patchset I based the
formatting of the advice based on the ones in `builtin/add.c` which
seems to also use `\t`.

> 
> > So I just went with `[[:space:]]+` as I
> > didn't want to have to worry about whether some platforms expand the
> > tab to spaces or how many spaces.
> 
> Is that a thing?

It might be? I know copying text through tmux tends to expand tabs to
spaces for me so I figured some other tools or those same tools on
different platforms might do things like that as well. To be honest I
have no idea and figured that I'd just CYA by making it work in the
case that it did than trying to guarantee that it wouldn't happen.

> > [...]
> >
> > If there is documentation (or discussions) on how to use this backend
> > properly I'd appreciate a link and I can try workshopping a better
> > solution then. The warning included in the original patchset reads
> > from that HEAD file as well so it would also need to be adapted.
> 
> I'm afraid I don't have anything specific, there were some patches a
> while ago such as dd8468ef00 (t5601: read HEAD using rev-parse,
> 2021-05-31) that stopped reading HEAD from the filesystem.

Noted.

> > [...]
> 
> I think it might be better to just diagnose if HEAD is a dangling
> symbolic-ref or contains an invalid oid and leave it at that. See the
> documentation in refs.h for refs_resolve_ref_unsafe() for how to check
> if HEAD is a dangling symbolic ref - if rego_get_oid(repo, "HEAD") fails
> and it is not a dangling symbolic ref then it contains an invalid oid.

Understood. I'll start working on a separate patch to update that
warning once this patch settles then.

> 
> [...]

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

[Phillip Wood]

On 18/07/2023 01:44, Jacob Abel wrote:
> On 23/07/16 04:34PM, Phillip Wood wrote:
>> Oh so we need to search for a space followed by a tab after "hint:"
>> then.
> 
> Okay. I think `\t` is PCRE so I'll just update the string in
> `builtin/worktree.c` so we can just do `[ ]+` instead.
> 
>> As an aside we often just use four spaces to indent commands in
>> advice messages (see the output of git -C .. grep '"    git' \*.c)
> 
> Apologies. When writing up that original patchset I based the
> formatting of the advice based on the ones in `builtin/add.c` which
> seems to also use `\t`.

The existing code is not consistent on this point but I think there are 
more instances of "    " than "\t". Using "    " makes the indentation 
consistent as the "hint: " prefix is translated so we don't know how far 
the next tab stop will be from the end of the prefix.

>>
>>> So I just went with `[[:space:]]+` as I
>>> didn't want to have to worry about whether some platforms expand the
>>> tab to spaces or how many spaces.
>>
>> Is that a thing?
> 
> It might be? I know copying text through tmux tends to expand tabs to
> spaces for me so I figured some other tools or those same tools on
> different platforms might do things like that as well. To be honest I
> have no idea and figured that I'd just CYA by making it work in the
> case that it did than trying to guarantee that it wouldn't happen.

In the test we are redirecting the output to a file so things like tmux 
do not come into play. I think it would be a bit odd for the system libc 
to convert tabs to spaces.

>>> [...]
>>
>> I think it might be better to just diagnose if HEAD is a dangling
>> symbolic-ref or contains an invalid oid and leave it at that. See the
>> documentation in refs.h for refs_resolve_ref_unsafe() for how to check
>> if HEAD is a dangling symbolic ref - if rego_get_oid(repo, "HEAD") fails
>> and it is not a dangling symbolic ref then it contains an invalid oid.
> 
> Understood. I'll start working on a separate patch to update that
> warning once this patch settles then.

That's great. I think just telling the user something like

    branch 'main' does not exist

when HEAD contains the dangling symbolic ref "refs/heads/main" and

     HEAD is corrupt

when it is not a symbolic ref and repo_get_oid() fails would be fine.

Best Wishes

Phillip

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

[Jacob Abel]

On 23/07/18 02:36PM, Phillip Wood wrote:
> [...]
> 
> The existing code is not consistent on this point but I think there are
> more instances of "    " than "\t". Using "    " makes the indentation
> consistent as the "hint: " prefix is translated so we don't know how far
> the next tab stop will be from the end of the prefix.

Agreed.

> > [...]
> 
> In the test we are redirecting the output to a file so things like tmux
> do not come into play. I think it would be a bit odd for the system libc
> to convert tabs to spaces.

Understood. It was just a bit of paranoia on my part then.

> [...]
>
> > Understood. I'll start working on a separate patch to update that
> > warning once this patch settles then.
> 
> That's great. I think just telling the user something like
> 
>     branch 'main' does not exist
> 
> when HEAD contains the dangling symbolic ref "refs/heads/main" and
> 
>      HEAD is corrupt
> 
> when it is not a symbolic ref and repo_get_oid() fails would be fine.

Noted. I'll workshop it a bit before I put v1 of that patch out.

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

[Jacob Abel]

On 23/07/15 07:15PM, Jacob Abel wrote:
> On 23/07/15 09:59AM, Phillip Wood wrote:
> >
> > [...]
>
> [...]
>
> > 
> > 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
> 
> I'll make these changes.

Actually now that I'm sitting down to make these changes, I'm a bit
hesitant as to whether this would be a cleaner solution than what is
currently there. I say this as the contents of `actual` are about
10-12 lines in length and some of those lines would vary between the
different tests that use this test function. Those specific details
don't need to be tested for as they are validated in prior tests and
building a perfectly matching `expected` would further complicate that
fairly large test function.

> 
> > [...]

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

[Jacob Abel]

Replace all cases of `\s` with `[[:blank:]]` or ` ` 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 `[^ ]`. It's not
an exact replacement (as it does not match tabs) but it is close enough
for this use case.

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.

This commit also drops `--sq` from a rev-parse call as it appears to be
a no-op.

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]. 

Changes from v1:
  * Change `[[:space:]]` to ` ` where possible and `[[:blank:]]` 
    (tabs and spaces) otherwise as it is more accurate than `[[:space:]]` [4].
  * Change `[^[:space:]]` to `[^ ]` [4]. Technically `[^[:blank:]]` would be
    more accurate but tabs shouldn't be present where this `[^ ]` is used.
  * Drop `--sq` from rev-parse [4] (after further discussion [5]).
  * Update commit message to match changes.

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
4. https://lore.kernel.org/git/vn5sylull5lqpitsanlyan5fafxj5dhrxgo6k65c462dhqjbno@uwghfyfdixtk/
5. https://lore.kernel.org/git/bj27nq5aputhd66rkqer37vuc7qogpmn6nqyusladdy4k5it7k@u3yvvivrixsy/

Range-diff against v1:
1:  39f57add45 ! 1:  ef4ebd7350 t2400: Fix test failures when using grep 2.5
    @@ Metadata
      ## Commit message ##
         t2400: Fix test failures when using grep 2.5
     
    -    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`.
    +    Replace all cases of `\s` with `[[:blank:]]` or ` ` 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 `[^ ]`. It's not
    +    an exact replacement (as it does not match tabs) but it is close enough
    +    for this use case.
     
    -    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.
     
    +    This commit also drops `--sq` from a rev-parse call as it appears to be
    +    a no-op.
    +
         Signed-off-by: Jacob Abel <jacobabel@nullpo.dev>
     
      ## t/t2400-worktree-add.sh ##
    @@ t/t2400-worktree-add.sh: test_wt_add_orphan_hint () {
      		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
    ++			grep -E "^hint:[[:blank:]]+git worktree add --orphan -b [^ ]+ [^ ]+$" actual
      		else
     -			grep -E "^hint:\s+git worktree add --orphan \S+\s*$" actual
    -+			grep -E "^hint:[[:space:]]+git worktree add --orphan [^[:space:]]+[[:space:]]*$" actual
    ++			grep -E "^hint:[[:blank:]]+git worktree add --orphan [^ ]+$" actual
      		fi
      
      	'
    @@ t/t2400-worktree-add.sh: test_dwim_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 invalid_ref_regex="^fatal: invalid reference: .*" &&
      	local bad_combo_regex="^fatal: '[a-z-]\+' and '[a-z-]\+' cannot be used together" &&
      
      	local git_ns="repo" &&
     @@ t/t2400-worktree-add.sh: test_dwim_orphan () {
    - 					headpath=$(git $dashc_args rev-parse --sq --path-format=absolute --git-path HEAD) &&
    + 					grep "$invalid_ref_regex" actual &&
    + 					! grep "$orphan_hint" actual
    + 				else
    +-					headpath=$(git $dashc_args rev-parse --sq --path-format=absolute --git-path HEAD) &&
    ++					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:[[:space:]]*.$headpath." actual &&
    -+					grep "HEAD contents:[[:space:]]*.$headcontents." actual &&
    ++					grep "HEAD path: .$headpath." actual &&
    ++					grep "HEAD contents: .$headcontents." actual &&
      					grep "$orphan_hint" actual &&
      					! grep "$info_text" actual
      				fi &&

 t/t2400-worktree-add.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index 0ac468e69e..1b693dfca9 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:[[:blank:]]+git worktree add --orphan -b [^ ]+ [^ ]+$" actual
 		else
-			grep -E "^hint:\s+git worktree add --orphan \S+\s*$" actual
+			grep -E "^hint:[[:blank:]]+git worktree add --orphan [^ ]+$" 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: .*" &&
 	local bad_combo_regex="^fatal: '[a-z-]\+' and '[a-z-]\+' cannot be used together" &&
 
 	local git_ns="repo" &&
@@ -995,11 +995,11 @@ test_dwim_orphan () {
 					grep "$invalid_ref_regex" actual &&
 					! grep "$orphan_hint" actual
 				else
-					headpath=$(git $dashc_args rev-parse --sq --path-format=absolute --git-path HEAD) &&
+					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 &&
-- 
2.39.3

[PATCH v3 0/3] t2400: Fix test failures when using grep 2.5

[Jacob Abel]

This patchset 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]. 

Note: I jumped the gun on v2 a bit as discussions for v1 were still in
progress so these are all changes suggested off v1.

Changes from v2:
  * Split `--sq` change out into separate patch (from 3/3 to 1/3).
  * Convert tab in advice to space to match coding convention and to
    allow regex to be further simplified [4].
  * Simplified regex [4].
  * Reworded commit message for patch 3/3 to better document reason for
    change [4].

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
4. https://lore.kernel.org/git/3f3a3f5b-70fd-ec3f-acbb-d585b5eb6cbc@gmail.com/

Jacob Abel (3):
  t2400: drop no-op `--sq` from rev-parse call
  builtin/worktree.c: convert tab in advice to space
  t2400: rewrite regex to avoid unintentional PCRE

 builtin/worktree.c      |  4 ++--
 t/t2400-worktree-add.sh | 12 ++++++------
 2 files changed, 8 insertions(+), 8 deletions(-)

Range-diff against v2:
-:  ---------- > 1:  96c21c5bee t2400: drop no-op `--sq` from rev-parse call
-:  ---------- > 2:  ebfba2d602 builtin/worktree.c: convert tab in advice to space
1:  ef4ebd7350 ! 3:  dee0c8f350 t2400: Fix test failures when using grep 2.5
    @@ Metadata
     Author: Jacob Abel <jacobabel@nullpo.dev>
     
      ## Commit message ##
    -    t2400: Fix test failures when using grep 2.5
    +    t2400: rewrite regex to avoid unintentional PCRE
     
    -    Replace all cases of `\s` with `[[:blank:]]` or ` ` as older versions
    -    of GNU grep (and from what it seems most versions of BSD grep) do not
    -    handle `\s`.
    +    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.
     
         For the same reason all cases of `\S` are replaced with `[^ ]`. It's not
    -    an exact replacement (as it does not match tabs) but it is close enough
    -    for this use case.
    -
    -    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.
    -
    -    This commit also drops `--sq` from a rev-parse call as it appears to be
    -    a no-op.
    +    an exact replacement but it is close enough for this use case.
     
         Signed-off-by: Jacob Abel <jacobabel@nullpo.dev>
     
    @@ t/t2400-worktree-add.sh: test_wt_add_orphan_hint () {
      		if [ $use_branch -eq 1 ]
      		then
     -			grep -E "^hint:\s+git worktree add --orphan -b \S+ \S+\s*$" actual
    -+			grep -E "^hint:[[:blank:]]+git worktree add --orphan -b [^ ]+ [^ ]+$" 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:[[:blank:]]+git worktree add --orphan [^ ]+$" actual
    ++			grep -E "^hint:[ ]+git worktree add --orphan [^ ]+$" actual
      		fi
      
      	'
    @@ t/t2400-worktree-add.sh: test_dwim_orphan () {
      
      	local git_ns="repo" &&
     @@ t/t2400-worktree-add.sh: test_dwim_orphan () {
    - 					grep "$invalid_ref_regex" actual &&
    - 					! grep "$orphan_hint" actual
    - 				else
    --					headpath=$(git $dashc_args rev-parse --sq --path-format=absolute --git-path HEAD) &&
    -+					headpath=$(git $dashc_args rev-parse --path-format=absolute --git-path HEAD) &&
    + 					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 &&
-- 
2.39.3

[PATCH v3 1/3] t2400: drop no-op `--sq` from rev-parse call

[Jacob Abel]

Signed-off-by: Jacob Abel <jacobabel@nullpo.dev>
---
 t/t2400-worktree-add.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index 0ac468e69e..e106540c6d 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -995,7 +995,7 @@ test_dwim_orphan () {
 					grep "$invalid_ref_regex" actual &&
 					! grep "$orphan_hint" actual
 				else
-					headpath=$(git $dashc_args rev-parse --sq --path-format=absolute --git-path HEAD) &&
+					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 &&
-- 
2.39.3

[PATCH v3 2/3] builtin/worktree.c: convert tab in advice to space

[Jacob Abel]

Signed-off-by: Jacob Abel <jacobabel@nullpo.dev>
---
 builtin/worktree.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 7c114d56a3..3cdcb86cd4 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -54,14 +54,14 @@
 	"(branch with no commits) for this repository, you can do so\n" \
 	"using the --orphan flag:\n" \
 	"\n" \
-	"	git worktree add --orphan -b %s %s\n")
+	"    git worktree add --orphan -b %s %s\n")
 
 #define WORKTREE_ADD_ORPHAN_NO_DASH_B_HINT_TEXT \
 	_("If you meant to create a worktree containing a new orphan branch\n" \
 	"(branch with no commits) for this repository, you can do so\n" \
 	"using the --orphan flag:\n" \
 	"\n" \
-	"	git worktree add --orphan %s\n")
+	"    git worktree add --orphan %s\n")
 
 static const char * const git_worktree_usage[] = {
 	BUILTIN_WORKTREE_ADD_USAGE,
-- 
2.39.3

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

[Jacob Abel]

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.

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.

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
 
 	'
@@ -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: .*" &&
 	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 --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 &&
-- 
2.39.3

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

[Junio C Hamano]

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.

> 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.

> @@ -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.

> @@ -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.

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

[Junio C Hamano]

Just for reference, here is a summary, as a squashable commit, of
the message I am responding to.

---- >8 ----
Subject: [PATCH] SQUASH???

(remove this part of the message while squashing)
Use ` `, not `[ ]`, as the proposed log message described.

(append to the proposed log message of the previous)
Also, do not write `\+` in BRE and expect it to mean 1 or more;
it is a GNU extension that may not work everywhere.

Remove '.*' at the end of a pattern that is not right-anchored.

Helped-by: Junio C Hamano <gitster@pobox.com>
---
 t/t2400-worktree-add.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index eafecdf7ce..aee5eba980 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:[ ]+git worktree add --orphan -b [^ ]+ [^ ]+$" actual
+			grep -E "^hint: +git worktree add --orphan -b [^ ]+ [^ ]+$" actual
 		else
-			grep -E "^hint:[ ]+git worktree add --orphan [^ ]+$" actual
+			grep -E "^hint: +git worktree add --orphan [^ ]+$" actual
 		fi
 
 	'
@@ -709,8 +709,8 @@ 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: .*" &&
-	local bad_combo_regex="^fatal: '[a-z-]\+' and '[a-z-]\+' cannot be used together" &&
+	local invalid_ref_regex="^fatal: invalid reference: " &&
+	local bad_combo_regex="^fatal: '[-a-z]*' and '[-a-z-]*' cannot be used together" &&
 
 	local git_ns="repo" &&
 	local dashc_args="-C $git_ns" &&
-- 
2.41.0-376-gcba07a324d

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

[Jacob Abel]

On 23/07/21 08:16AM, Junio C Hamano wrote:
> 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.

> 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.

> [...]
> 
> 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.

> > @@ -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.

> 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.

> >  	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.

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

> [...]

[PATCH v4 0/3] t2400: Fix test failures when using grep 2.5

[Jacob Abel]

This patchset 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]. 

Changes from v3:
  * Replace `[ ]` with ` ` in regex for `test_wt_add_orphan_hint()` [4][5].
  * Drop trailing `.*` from `invalid_ref_regex` [4][5].
  * Change `[a-z-]` to `[-a-z]` in `bad_combo_regex` to better portray
    intent [4][5].
  * Replace `\+` with `*` in `bad_combo_regex` as `\+` is not POSIX
    BRE and is a GNU extension [4][5].
  * Drop "without PCRE support" from commit message [4].
  * Reword commit message to reflect changes.

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
4. https://lore.kernel.org/git/axnxvnmo6ekhhccppinji73ivlandwuqs44epmq4pdefm7ukiv@ejz7bee5xjli/
5. https://lore.kernel.org/git/xmqqiladw9h7.fsf@gitster.g/

Jacob Abel (3):
  t2400: drop no-op `--sq` from rev-parse call
  builtin/worktree.c: convert tab in advice to space
  t2400: rewrite regex to avoid unintentional PCRE

 builtin/worktree.c      |  4 ++--
 t/t2400-worktree-add.sh | 14 +++++++-------
 2 files changed, 9 insertions(+), 9 deletions(-)

Range-diff against v3:
1:  96c21c5bee = 1:  96c21c5bee t2400: drop no-op `--sq` from rev-parse call
2:  ebfba2d602 = 2:  ebfba2d602 builtin/worktree.c: convert tab in advice to space
3:  dee0c8f350 ! 3:  13f61cd15a t2400: rewrite regex to avoid unintentional PCRE
    @@ Commit message
         t2400: rewrite regex to avoid unintentional PCRE
     
         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.
    +    and therefore not all versions of grep handle it.
     
    -    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.
    +    For the same reason all cases of `\S` are replaced with `[^ ]`. It is
    +    not an exact replacement but it is close enough for this use case.
    +
    +    Also, do not write `\+` in BRE and expect it to mean 1 or more;
    +    it is a GNU extension that may not work everywhere.
    +
    +    Remove `.*` from the end of a pattern that is not right-anchored.
     
         Signed-off-by: Jacob Abel <jacobabel@nullpo.dev>
    +    Helped-by: Junio C Hamano <gitster@pobox.com>
     
      ## t/t2400-worktree-add.sh ##
     @@ t/t2400-worktree-add.sh: test_wt_add_orphan_hint () {
    @@ t/t2400-worktree-add.sh: test_wt_add_orphan_hint () {
      		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
    ++			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
    ++			grep -E "^hint: +git worktree add --orphan [^ ]+$" actual
      		fi
      
      	'
    @@ t/t2400-worktree-add.sh: test_dwim_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: .*" &&
    - 	local bad_combo_regex="^fatal: '[a-z-]\+' and '[a-z-]\+' cannot be used together" &&
    +-	local bad_combo_regex="^fatal: '[a-z-]\+' and '[a-z-]\+' cannot be used together" &&
    ++	local invalid_ref_regex="^fatal: invalid reference: " &&
    ++	local bad_combo_regex="^fatal: '[-a-z]*' and '[-a-z]*' cannot be used together" &&
      
      	local git_ns="repo" &&
    + 	local dashc_args="-C $git_ns" &&
     @@ t/t2400-worktree-add.sh: test_dwim_orphan () {
      					headpath=$(git $dashc_args rev-parse --path-format=absolute --git-path HEAD) &&
      					headcontents=$(cat "$headpath") &&
-- 
2.41.0

[PATCH v4 1/3] t2400: drop no-op `--sq` from rev-parse call

[Jacob Abel]

Signed-off-by: Jacob Abel <jacobabel@nullpo.dev>
---
 t/t2400-worktree-add.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index 0ac468e69e..e106540c6d 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -995,7 +995,7 @@ test_dwim_orphan () {
 					grep "$invalid_ref_regex" actual &&
 					! grep "$orphan_hint" actual
 				else
-					headpath=$(git $dashc_args rev-parse --sq --path-format=absolute --git-path HEAD) &&
+					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 &&
-- 
2.41.0

[PATCH v4 2/3] builtin/worktree.c: convert tab in advice to space

[Jacob Abel]

Signed-off-by: Jacob Abel <jacobabel@nullpo.dev>
---
 builtin/worktree.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 7c114d56a3..3cdcb86cd4 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -54,14 +54,14 @@
 	"(branch with no commits) for this repository, you can do so\n" \
 	"using the --orphan flag:\n" \
 	"\n" \
-	"	git worktree add --orphan -b %s %s\n")
+	"    git worktree add --orphan -b %s %s\n")
 
 #define WORKTREE_ADD_ORPHAN_NO_DASH_B_HINT_TEXT \
 	_("If you meant to create a worktree containing a new orphan branch\n" \
 	"(branch with no commits) for this repository, you can do so\n" \
 	"using the --orphan flag:\n" \
 	"\n" \
-	"	git worktree add --orphan %s\n")
+	"    git worktree add --orphan %s\n")
 
 static const char * const git_worktree_usage[] = {
 	BUILTIN_WORKTREE_ADD_USAGE,
-- 
2.41.0

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

[Jacob Abel]

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.

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

Also, do not write `\+` in BRE and expect it to mean 1 or more;
it is a GNU extension that may not work everywhere.

Remove `.*` from the end of a pattern that is not right-anchored.

Signed-off-by: Jacob Abel <jacobabel@nullpo.dev>
Helped-by: Junio C Hamano <gitster@pobox.com>
---
 t/t2400-worktree-add.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index e106540c6d..051363acbb 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
 
 	'
@@ -709,8 +709,8 @@ 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 bad_combo_regex="^fatal: '[a-z-]\+' and '[a-z-]\+' cannot be used together" &&
+	local invalid_ref_regex="^fatal: invalid reference: " &&
+	local bad_combo_regex="^fatal: '[-a-z]*' and '[-a-z]*' cannot be used together" &&
 
 	local git_ns="repo" &&
 	local dashc_args="-C $git_ns" &&
@@ -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 &&
-- 
2.41.0

Re: [PATCH v4 0/3] t2400: Fix test failures when using grep 2.5

[Junio C Hamano]

Jacob Abel <jacobabel@nullpo.dev> writes:

> This patchset 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]. 

Looking very good.

Will queue.  Let's plan to merge it down to 'next' shortly.

Thanks.

Re: [PATCH v4 0/3] t2400: Fix test failures when using grep 2.5

[Phillip Wood]

On 26/07/2023 23:09, Junio C Hamano wrote:
> Jacob Abel <jacobabel@nullpo.dev> writes:
> 
>> This patchset 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].
> 
> Looking very good.
> 
> Will queue.  Let's plan to merge it down to 'next' shortly.
> 
> Thanks.

I read through the patches and agree they're looking good now, thanks Jacob.

Best Wishes

Phillip