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

[Phillip Wood <phillip.wood123@gmail.com>]

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