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