| Message ID | 20200928154953.30396-3-rafaeloliveira.cs@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | teach `worktree list` to mark locked worktrees | expand |
Rafael Silva <rafaeloliveira.cs@gmail.com> 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@gmail.com> > --- > 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. > 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.
On Mon, Sep 28, 2020 at 02:54:24PM -0700, Junio C Hamano wrote: > Rafael Silva <rafaeloliveira.cs@gmail.com> 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@gmail.com> > > --- > > 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.
On Mon, Sep 28, 2020 at 11:50 AM Rafael Silva <rafaeloliveira.cs@gmail.com> wrote: > 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. In addition to Junio's review comments... > Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com> > --- > diff --git a/t/t2402-worktree-list.sh b/t/t2402-worktree-list.sh > @@ -61,6 +61,19 @@ test_expect_success '"list" all worktrees --porcelain' ' > +test_expect_success 'show locked worktree with (locked)' ' Existing test titles in this script seem to follow a particular pattern (for the most part). Perhaps this could say instead: test_expect_success '"list" all worktrees with "locked"' ' > + echo "$(git rev-parse --show-toplevel) $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]" >expect && I see that this is following existing practice in this script of piling up $(git ...) invocations which can lose the individual exit codes, so I can't complain about it, but I'll note that these days we'd more likely than not try to preserve the exit codes, perhaps like this: path=$(git rev-parse --show-toplevel) && rev=$(git rev-parse --short HEAD) && sym=$(git symbolic-ref --short HEAD) && echo "$path $rev [$sym]" >expect && However, that gets verbose since you're doing it for multiple lines (below). > + 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 > +' I realize that Junio proposed an alternate simpler approach which checks specifically the attribute in which you are interested, but here's yet another (typed-in-email) approach you could use: test_when_finished "..." && git worktree add --detach locked master && git worktree add --detach unlocked master && git worktree list >out && sed '/\/locked/s/$/ (locked)/' out >expect && git worktree lock locked && git worktree list >actual && test_cmp expect actual
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 +' + test_expect_success 'bare repo setup' ' git init --bare bare1 && echo "data" >file1 &&
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@gmail.com> --- t/t2402-worktree-list.sh | 13 +++++++++++++ 1 file changed, 13 insertions(+)