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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jul 18, 2023, 13:36 UTC
Message-ID: <44671697-e9e6-d75a-30b9-7dccffcc792a@gmail.com>
In-Reply-To: <dyzkftugvd5b4f4wxsg6773fkrdrnbync6idvvi6h7cuuto36w@dbzjnkj3mh2l>

On 18/07/2023 01:44, Jacob Abel wrote:

Show 13 quoted lines

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

Show 12 quoted lines

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

Show 10 quoted lines

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