From: Phillip Wood Date: Sun, 16 Jul 2023 15:34:52 GMT Subject: Re: [PATCH] t2400: Fix test failures when using grep 2.5 Message-ID: <3f3a3f5b-70fd-ec3f-acbb-d585b5eb6cbc@gmail.com> In-Reply-To: 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. > >> [...] >