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