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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jul 16, 2023, 15:34 UTC
Message-ID: <3f3a3f5b-70fd-ec3f-acbb-d585b5eb6cbc@gmail.com>
In-Reply-To: <vn5sylull5lqpitsanlyan5fafxj5dhrxgo6k65c462dhqjbno@uwghfyfdixtk>

Hi Jacob

On 16/07/2023 00:15, Jacob Abel wrote:

Show 9 quoted lines

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

Show 16 quoted lines

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

Show 24 quoted lines

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

Show 5 quoted lines

> 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

Show 19 quoted lines

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