Re: [PATCH] t: make many tests depend less on the refs being files

[Christian Couder <christian.couder@gmail.com>]

Hi Dscho,

On Mon, May 21, 2018 at 11:41 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> On Mon, 21 May 2018, Christian Couder wrote:
>
>> From: David Turner <dturner@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-dturner@twosigma.com/

Yeah, I actually used "David Turner <novalis@novalis.org>" in the --cc
option I gave to `git send-email`...

> So maybe update it here, too, to
>
>         From: David Turner <novalis@novalis.org>

...but I thought it was better to keep the original author and
Signed-off-by as they are in the original commit:

https://github.com/dturner-tw/git/commit/0a3fa7fbd7f99280b5f128be3e1ed04e045da671

Now I am ok to update them if it is preferred.

>> 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 <dturner@twopensource.com>
>> Signed-off-by: Christian Couder <chriscool@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...

Ok, the 'error "..."' will be on another line in the next version.

>> 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.

Ok, they will all have quoted arguments in the next version.

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

Ok, I will take a look at that.

>> 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?

I think anywhere inside ".git/" should work, so I guess
".git/refs/heads" was chosen to make sure that adding anything after
".git/" does not change the result.

I think I can drop this for now and change it later in its own commit
with related explanations.

Thanks,
Christian.