| Message ID | 20230721044012.24360-4-jacobabel@nullpo.dev (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | t2400: Fix test failures when using grep 2.5 | expand |
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.
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" &&
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. > [...]
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 &&
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(-)