| Message ID | 20190129051859.12830-1-koraktor@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/3] Add tests for describe with --work-tree | expand |
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@gmail.com> > --- > 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
Jeff King <peff@peff.net> writes: > 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. Good. >> + grep 'A-\d\+-g[0-9a-f]\+' '$TRASH_DIRECTORY/out' > > Using "\d" isn't portable. True, but not just \d. I think using \ before special characters to force an otherwise basic regular expression to be ERE (i.e. \+ at the end) is a GNUism. > 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 ... Good.
Am Di., 29. Jan. 2019 um 18:47 Uhr schrieb Junio C Hamano <gitster@pobox.com>: > > Jeff King <peff@peff.net> writes: > > > 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. > > Good. Ok, thanks. I’ll change this in the next reroll. > > >> + grep 'A-\d\+-g[0-9a-f]\+' '$TRASH_DIRECTORY/out' > > > > Using "\d" isn't portable. > > True, but not just \d. I think using \ before special characters to > force an otherwise basic regular expression to be ERE (i.e. \+ at > the end) is a GNUism. > I guess I’ll use the even broader but apparently more portable A-*[0-9a-f] then. It’s used in the other checks, so this should be OK? > > 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 ... > > Good. This was copied and pasted from the existing check for describe with a clean working tree. So this should be changed, too.
On Wed, Jan 30, 2019 at 11:23:14AM +0100, Sebastian Staudt wrote: > > >> + grep 'A-\d\+-g[0-9a-f]\+' '$TRASH_DIRECTORY/out' > > > > > > Using "\d" isn't portable. > > > > True, but not just \d. I think using \ before special characters to > > force an otherwise basic regular expression to be ERE (i.e. \+ at > > the end) is a GNUism. > > > > I guess I’ll use the even broader but apparently more portable A-*[0-9a-f] > then. It’s used in the other checks, so this should be OK? Yeah, that is OK, as long as you use it with `case` like check_describe does. Because it's a glob and not a regex, you do not have to worry about the anchoring issue. Or if you mean to do an anchored but more general regex like: ^A-.*[0-9a-f]$ that is OK, too. > > > If that's indeed what we're checking, then an easier check is perhaps: > > > > > > ! grep dirty ... > > > > Good. > > This was copied and pasted from the existing check for describe with a > clean working tree. So this should be changed, too. I do think that makes what we're checking more obvious, so I wouldn't mind seeing the other tests converted to use this. But it may be hard if they are relying on check_describe(). I'm OK with anything that works and is robust. :) -Peff
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' " + ( + cd '$TEST_DIRECTORY' && + git --git-dir '$TRASH_DIRECTORY/.git' --work-tree '$TRASH_DIRECTORY' describe --dirty >'$TRASH_DIRECTORY/out' + ) && + grep 'A-\d\+-g[0-9a-f]\+' '$TRASH_DIRECTORY/out' +" + test_expect_success 'set-up dirty work tree' ' echo >>file ' check_describe "A-*[0-9a-f]-dirty" --dirty +test_expect_success 'describe --dirty with --work-tree' " + ( + cd '$TEST_DIRECTORY' && + git --git-dir '$TRASH_DIRECTORY/.git' --work-tree '$TRASH_DIRECTORY' describe --dirty >'$TRASH_DIRECTORY/out' + ) && + grep 'A-\d\+-g[0-9a-f]\+-dirty' '$TRASH_DIRECTORY/out' +" + check_describe "A-*[0-9a-f].mod" --dirty=.mod +test_expect_success 'describe --dirty=.mod with --work-tree' " + ( + cd '$TEST_DIRECTORY' && + git --git-dir '$TRASH_DIRECTORY/.git' --work-tree '$TRASH_DIRECTORY' describe --dirty=.mod >'$TRASH_DIRECTORY/out' + ) && + grep 'A-\d\+-g[0-9a-f]\+.mod' '$TRASH_DIRECTORY/out' +" + test_expect_success 'describe --dirty HEAD' ' test_must_fail git describe --dirty HEAD ' @@ -303,12 +327,21 @@ test_expect_success 'describe chokes on severely broken submodules' ' mv .git/modules/sub1/ .git/modules/sub_moved && test_must_fail git describe --dirty ' + test_expect_success 'describe ignoring a broken submodule' ' git describe --broken >out && test_when_finished "mv .git/modules/sub_moved .git/modules/sub1" && grep broken out ' +test_expect_success 'describe with --work-tree ignoring a broken submodule' " + ( + cd '$TEST_DIRECTORY' && + git --git-dir '$TRASH_DIRECTORY/.git' --work-tree '$TRASH_DIRECTORY' describe --broken >'$TRASH_DIRECTORY/out' + ) && + grep broken '$TRASH_DIRECTORY/out' +" + test_expect_success 'describe a blob at a directly tagged commit' ' echo "make it a unique blob" >file && git add file && git commit -m "content in file" &&
The dirty ones are already passing, but just because describe is comparing with the wrong working tree. Signed-off-by: Sebastian Staudt <koraktor@gmail.com> --- t/t6120-describe.sh | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)