Re: [PATCH v3 1/3] Add tests for describe with --work-tree

[Jeff King <peff@peff.net>]

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