Hi Chris,

On Mon, 21 May 2018, Christian Couder wrote:

> From: David Turner <dtur...@twopensource.com>

I vaguely remember that Dave suggested using a different email address
these days...

*clicketyclick*

ah, yes:
https://public-inbox.org/git/1474935093-26757-3-git-send-email-dtur...@twosigma.com/

So maybe update it here, too, to 

        From: David Turner <nova...@novalis.org>

?

> So that they work under alternate ref storage backends.
> 
> This will be really needed when such alternate ref storage backends are
> developed. But this could already help by making clear to readers that
> some tests do not depend on which ref backend is used.
> 
> This patch just takes care of many low hanging fruits. It does not try
> to completely solves the issue.
> 
> Signed-off-by: David Turner <dtur...@twopensource.com>
> Signed-off-by: Christian Couder <chrisc...@tuxfamily.org>
> ---

This is great. I am all for making the tests better ;-)

> diff --git a/t/lib-t6000.sh b/t/lib-t6000.sh
> index 3f2d873fec..b8567cdf94 100644
> --- a/t/lib-t6000.sh
> +++ b/t/lib-t6000.sh
> @@ -4,11 +4,10 @@ mkdir -p .git/refs/tags
>  
>  >sed.script
>  
> -# Answer the sha1 has associated with the tag. The tag must exist in 
> .git/refs/tags
> +# Answer the sha1 has associated with the tag. The tag must exist under 
> refs/tags
>  tag () {
>       _tag=$1
> -     test -f ".git/refs/tags/$_tag" || error "tag: \"$_tag\" does not exist"
> -     cat ".git/refs/tags/$_tag"
> +     git rev-parse --verify "refs/tags/$_tag" || error "tag: \"$_tag\" does 
> not exist"

Line longer than 80 columns...

> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 0680dec808..886a9e3b72 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -30,7 +30,7 @@ add () {
>       test_tick &&
>       commit=$(echo "$text" | git commit-tree $tree $parents) &&
>       eval "$name=$commit; export $name" &&
> -     echo $commit > .git/refs/heads/$branch &&
> +     git update-ref refs/heads/$branch $commit &&

I think we have to be careful here to quote both "refs/heads/$branch" and
"$commit" here, as a bug that introduces spaces into $commit or $branch
would have been caught earlier, but not now, unless we quote.

This goes for all introduced `update-ref` calls.

Maybe even for some `git rev-parse --verify` calls.

> diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
> index 8f5c811dd7..c3b89ae783 100755
> --- a/t/t9903-bash-prompt.sh
> +++ b/t/t9903-bash-prompt.sh
> @@ -148,7 +148,7 @@ test_expect_success 'prompt - inside .git directory' '
>  test_expect_success 'prompt - deep inside .git directory' '
>       printf " (GIT_DIR!)" >expected &&
>       (
> -             cd .git/refs/heads &&
> +             cd .git/objects &&
>               __git_ps1 >"$actual"
>       ) &&
>       test_cmp expected "$actual"
> -- 

This one looks wrong.

Upon cursory review, one might be tempted to assume that the file is now
written into .git/objects/ instead of .git/refs/heads/. And the patch
context provided in the email is not enough to see (gawd, I hate
mail-based patch review, it really takes all my Git tools away from me).
The trick is that `actual` points at an absolute path:

        #!/bin/sh
        #
        # Copyright (c) 2012 SZEDER Gábor
        #

        test_description='test git-specific bash prompt functions'

        . ./lib-bash.sh

        . "$GIT_BUILD_DIR/contrib/completion/git-prompt.sh"

        actual="$TRASH_DIRECTORY/actual"
        [...]

So the remaining question (which probably wants to be added to the commit
message together with a hint that `actual` points at an absolute path) is:
Why not `cd .git` instead?

Ciao,
Dscho

Reply via email to