On Tue, Jan 29, 2019 at 06:18:57AM +0100, Sebastian Staudt wrote: > The dirty ones are already passing, but just because describe is comparing > with the wrong working tree. > > Signed-off-by: Sebastian Staudt <koraktor@xxxxxxxxx> > --- > t/t6120-describe.sh | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh > index d639d94696..c863c4f600 100755 > --- a/t/t6120-describe.sh > +++ b/t/t6120-describe.sh > @@ -145,14 +145,38 @@ check_describe A-* HEAD > > check_describe "A-*[0-9a-f]" --dirty > > +test_expect_success 'describe --dirty with --work-tree' " This should be marked as test_expect_failure, I think, because it does not yet pass (and then flipped s/failure/success/ in the next patch). > + ( > + cd '$TEST_DIRECTORY' && > + git --git-dir '$TRASH_DIRECTORY/.git' --work-tree '$TRASH_DIRECTORY' describe --dirty >'$TRASH_DIRECTORY/out' > + ) && The quoting you've done here is unusual for our test suite, and not quite as robust. You've put the whole test snippet into double-quotes, which means that $TRASH_DIRECTORY, etc, will be expanded before we eval the code. By putting single-quotes around $TRASH_DIRECTORY, that makes it work when the path contains a space. But it would fail if somebody's filesystem path contains a single-quote. The usual style is to put the whole snippet into single-quotes, and then double-quote as appropriate within it. Like: test_expect_failure 'describe --dirty with --work-tree' ' ( cd "$TEST_DIRECTORY" && git --git-dir "$TRASH_DIRECTORY/.git" ...etc Those variables will be expanded when test_expect_failure eval's the snippet. > + grep 'A-\d\+-g[0-9a-f]\+' '$TRASH_DIRECTORY/out' Using "\d" isn't portable. This regex is pretty broad. What are we checking here? If I understand the previous discussion, we just care that it doesn't have "dirty" in it, right? I don't think this regex does that, because it doesn't anchor the end of string. If that's indeed what we're checking, then an easier check is perhaps: ! grep dirty ... As a side note, you can also shorten your references to "out" by referring to it from the trash directory itself. I.e.: ( cd "$TEST_DIRECTORY" && git --git-dir="$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" \ describe --dirty ) >out && ! grep dirty out Same thing, but IMHO a little easier to read. > check_describe "A-*[0-9a-f]-dirty" --dirty > > +test_expect_success 'describe --dirty with --work-tree' " > [...] Same comments apply to the other blocks you added. Other than those mechanical things, though, what the tests are actually trying to do seems quite reasonable to me. -Peff
