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 <[email protected]>
> ---
> 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
