On Mon, Sep 28, 2020 at 02:54:24PM -0700, Junio C Hamano wrote: > Rafael Silva <rafaeloliveira.cs@xxxxxxxxx> writes: > > > Test the output of the `worktree list` command to show when > > a linked worktree is locked and test to not mistakenly > > mark main or unlocked worktrees. > > > > Signed-off-by: Rafael Silva <rafaeloliveira.cs@xxxxxxxxx> > > --- > > t/t2402-worktree-list.sh | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > I think this should be part of [1/2], as the change necessary to > implement the new feature is small enough that there is no reason > to split the test part out. Sounds good > > > diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh > > index 52585ec2aa..07bd9a3350 100755 > > --- a/t/t2402-worktree-list.sh > > +++ b/t/t2402-worktree-list.sh > > @@ -61,6 +61,19 @@ test_expect_success '"list" all worktrees --porcelain' ' > > test_cmp expect actual > > ' > > > > +test_expect_success 'show locked worktree with (locked)' ' > > + echo "$(git rev-parse --show-toplevel) $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]" >expect && > > + test_when_finished "rm -rf locked unlocked out actual expect && git worktree prune" && > > + git worktree add --detach locked master && > > + git worktree add --detach unlocked master && > > + git worktree lock locked && > > + echo "$(git -C locked rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD) (locked)" >>expect && > > + echo "$(git -C unlocked rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >>expect && > > + git worktree list >out && > > + sed "s/ */ /g" <out >actual && > > + test_cmp expect actual > > +' > > This seems to prescribe the output from the command too strictly > (you do avoid being overly too strict by removing the indentation > with 's/ */ /g' though). > > If the leading path to the $TRASH_DIRECTORY has two or more > consecutive SPs (and that is not something under our control), the > 'expect' file would keep such a double-SP, but such a double-SP in > 'out' would have been squashed out in the 'actual' file. > > I wonder if > > grep '/locked *[0-9a-f].* (locked)' out && > ! grep '/unlocked *[0-9a-f].* (locked)' out > > might be a better way to test? That is > > - we do not care what the leading directories are called > - we do not care what branch is checked out or how they are presented > - we care the one that ends with /locked is (locked) > - we care the one that ends with /unlocked is not (locked) > > After all, this new test piece is not about verifying that the > object name or branch name is correct. Sounds good and I agree with the all, and seems even easier to understand the test code this way. Will address this change on the next patch, although I'm not sure whether somebody could run into any issues when running the `grep` command in other platforms. I'll look into it.
