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

[Christian Couder]

On Fri, May 25, 2018 at 11:05 AM, Michael Haggerty  wrote:
> On Fri, May 25, 2018 at 10:59 AM, Jeff King  wrote:
>> On Fri, May 25, 2018 at 10:48:04AM +0200, Michael Haggerty wrote:
>>
>>> >  test_expect_success "multi-fetch works off a 'clean' repository" '
>>> > -   rm -r "$GIT_DIR/svn" "$GIT_DIR/refs/remotes" "$GIT_DIR/logs" &&
>>> > +   rm -rf "$GIT_DIR/svn" "$GIT_DIR/refs/remotes" &&
>>> > +   git reflog expire --all --expire=all &&
>>> > mkdir "$GIT_DIR/svn" &&
>>> > git svn multi-fetch
>>> > '
>>> >
>>>
>>> `rm -rf "$GIT_DIR/refs/remotes"` is not kosher. I think it can be written
>>>
>>> printf 'option no-deref\ndelete %s\n' $(git for-each-ref
>>> --format='%(refname)' refs/remotes) | git update-ref --stdin
>>>
>>> as long as the number of references doesn't exceed command-line limits.
>>> This will also take care of the reflogs. Another alternative would be to
>>> write it as a loop.
>>
>> Perhaps:
>>
>>   git for-each-ref --format="option no-deref%0adelete %(refname)" 
>> refs/remotes |
>>   git update-ref --stdin
>
> Ah yes, that's nicer. I tried with `\n`, but that's not supported
> (wouldn't it be nice if it were?). I didn't think to try `%0a` (let
> alone look in the documentation!)

Thanks both for this suggestion. I plan to use it in another patch.

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

[Christian Couder]

On Fri, May 25, 2018 at 10:48 AM, Michael Haggerty  wrote:
> On 05/23/2018 07:25 AM, Christian Couder wrote:
>>
>> diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
>> index 9e782a8122..a4ebb0b65f 100755
>> --- a/t/t1401-symbolic-ref.sh
>> +++ b/t/t1401-symbolic-ref.sh
>> @@ -65,7 +65,7 @@ reset_to_sane
>>  test_expect_success 'symbolic-ref fails to delete real ref' '
>>   echo "fatal: Cannot delete refs/heads/foo, not a symbolic ref" >expect 
>> &&
>>   test_must_fail git symbolic-ref -d refs/heads/foo >actual 2>&1 &&
>> - test_path_is_file .git/refs/heads/foo &&
>> + git rev-parse --verify refs/heads/foo &&
>>   test_cmp expect actual
>>  '
>>  reset_to_sane
>
> Should t1401 be considered a backend-agnostic test, or is it needed to
> ensure that symbolic refs are written correctly in the files backend?

I don't know. And I am ok to go either way. Another possibility would
be to split in two parts.

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

[Michael Haggerty]

On Fri, May 25, 2018 at 10:59 AM, Jeff King  wrote:
> On Fri, May 25, 2018 at 10:48:04AM +0200, Michael Haggerty wrote:
>
>> >  test_expect_success "multi-fetch works off a 'clean' repository" '
>> > -   rm -r "$GIT_DIR/svn" "$GIT_DIR/refs/remotes" "$GIT_DIR/logs" &&
>> > +   rm -rf "$GIT_DIR/svn" "$GIT_DIR/refs/remotes" &&
>> > +   git reflog expire --all --expire=all &&
>> > mkdir "$GIT_DIR/svn" &&
>> > git svn multi-fetch
>> > '
>> >
>>
>> `rm -rf "$GIT_DIR/refs/remotes"` is not kosher. I think it can be written
>>
>> printf 'option no-deref\ndelete %s\n' $(git for-each-ref
>> --format='%(refname)' refs/remotes) | git update-ref --stdin
>>
>> as long as the number of references doesn't exceed command-line limits.
>> This will also take care of the reflogs. Another alternative would be to
>> write it as a loop.
>
> Perhaps:
>
>   git for-each-ref --format="option no-deref%0adelete %(refname)" 
> refs/remotes |
>   git update-ref --stdin

Ah yes, that's nicer. I tried with `\n`, but that's not supported
(wouldn't it be nice if it were?). I didn't think to try `%0a` (let
alone look in the documentation!)

Michael

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

[Jeff King]

On Fri, May 25, 2018 at 10:48:04AM +0200, Michael Haggerty wrote:

> >  test_expect_success "multi-fetch works off a 'clean' repository" '
> > -   rm -r "$GIT_DIR/svn" "$GIT_DIR/refs/remotes" "$GIT_DIR/logs" &&
> > +   rm -rf "$GIT_DIR/svn" "$GIT_DIR/refs/remotes" &&
> > +   git reflog expire --all --expire=all &&
> > mkdir "$GIT_DIR/svn" &&
> > git svn multi-fetch
> > '
> > 
> 
> `rm -rf "$GIT_DIR/refs/remotes"` is not kosher. I think it can be written
> 
> printf 'option no-deref\ndelete %s\n' $(git for-each-ref
> --format='%(refname)' refs/remotes) | git update-ref --stdin
> 
> as long as the number of references doesn't exceed command-line limits.
> This will also take care of the reflogs. Another alternative would be to
> write it as a loop.

Perhaps:

  git for-each-ref --format="option no-deref%0adelete %(refname)" refs/remotes |
  git update-ref --stdin

-Peff

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

[Michael Haggerty]

On 05/23/2018 07:25 AM, Christian Couder wrote:
> From: David Turner 
> 
> Many tests are very focused on the file system representation of the
> loose and packed refs code. As there are plans to implement other
> ref storage systems, let's migrate these tests to a form that test
> the intent of the refs storage system instead of it internals.
> [...]
> 
> diff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh
> index 9e782a8122..a4ebb0b65f 100755
> --- a/t/t1401-symbolic-ref.sh
> +++ b/t/t1401-symbolic-ref.sh
> @@ -65,7 +65,7 @@ reset_to_sane
>  test_expect_success 'symbolic-ref fails to delete real ref' '
>   echo "fatal: Cannot delete refs/heads/foo, not a symbolic ref" >expect 
> &&
>   test_must_fail git symbolic-ref -d refs/heads/foo >actual 2>&1 &&
> - test_path_is_file .git/refs/heads/foo &&
> + git rev-parse --verify refs/heads/foo &&
>   test_cmp expect actual
>  '
>  reset_to_sane

Should t1401 be considered a backend-agnostic test, or is it needed to
ensure that symbolic refs are written correctly in the files backend?

> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index c0ef946811..222dc2c377 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -234,34 +234,34 @@ test_expect_success 'git branch -M master2 master2 
> should work when master is ch
>  
>  test_expect_success 'git branch -v -d t should work' '
>   git branch t &&
> - test_path_is_file .git/refs/heads/t &&
> + git rev-parse --verify refs/heads/t &&
>   git branch -v -d t &&
> - test_path_is_missing .git/refs/heads/t
> + test_must_fail git rev-parse --verify refs/heads/t
>  '
>  
>  test_expect_success 'git branch -v -m t s should work' '
>   git branch t &&
> - test_path_is_file .git/refs/heads/t &&
> + git rev-parse --verify refs/heads/t &&
>   git branch -v -m t s &&
> - test_path_is_missing .git/refs/heads/t &&
> - test_path_is_file .git/refs/heads/s &&
> + test_must_fail git rev-parse --verify refs/heads/t &&
> + git rev-parse --verify refs/heads/s &&
>   git branch -d s
>  '
>  
>  test_expect_success 'git branch -m -d t s should fail' '
>   git branch t &&
> - test_path_is_file .git/refs/heads/t &&
> + git rev-parse refs/heads/t &&
>   test_must_fail git branch -m -d t s &&
>   git branch -d t &&
> - test_path_is_missing .git/refs/heads/t
> + test_must_fail git rev-parse refs/heads/t
>  '
>  
>  test_expect_success 'git branch --list -d t should fail' '
>   git branch t &&
> - test_path_is_file .git/refs/heads/t &&
> + git rev-parse refs/heads/t &&
>   test_must_fail git branch --list -d t &&
>   git branch -d t &&
> - test_path_is_missing .git/refs/heads/t
> + test_must_fail git rev-parse refs/heads/t
>  '
>  
>  test_expect_success 'git branch --list -v with --abbrev' '
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index aefde7b172..1f871d3cca 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -726,7 +726,7 @@ test_expect_success 'store updates stash ref and reflog' '
>   git reset --hard &&
>   ! grep quux bazzy &&
>   git stash store -m quuxery $STASH_ID &&
> - test $(cat .git/refs/stash) = $STASH_ID &&
> + test $(git rev-parse stash) = $STASH_ID &&
>   git reflog --format=%H stash| grep $STASH_ID &&
>   git stash pop &&
>   grep quux bazzy
> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index 0680dec808..d4f435155f 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" &&
>   eval ${branch}TIP=$commit
>  }
>  
> @@ -45,10 +45,10 @@ pull_to_client () {
>  
>   case "$heads" in
>   *A*)
> - echo $ATIP > .git/refs/heads/A;;
> + git update-ref refs/heads/A "$ATIP";;
>   esac &&
>   case "$heads" in *B*)
> - echo $BTIP > .git/refs/heads/B;;
> + git update-ref refs/heads/B "$BTIP";;
>   esac &&
>   git symbolic-ref HEAD refs/heads/$(echo $heads \
>   | sed -e "s/^\(.\).*$/\1/") &&
> @@ -92,8 +92,8 @@ test_expect_success 'setup' '
>   cur=$(($cur+1))
>   done &&
>   add B1 $A1 &&
> - echo $ATIP > .git/refs/heads/A &&
> - echo $BTIP > .git/refs/heads/B &&
> + git update-ref refs/heads/A "$ATIP" &&
> + git update-ref refs/heads/B "$BTIP" &&
>   git symbolic-ref HEAD refs/heads/B
>  '
>  
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index ae5a530a2d..e402aee6a2 100755
> --- a/t/t5510-fetch.sh
> 

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

[Junio C Hamano]

Christian Couder  writes:

> The internals of the loose refs backend are still tested in
> t1400-update-ref.sh, whereas the tests changed in this patch focus
> on testing other aspects.
>
> This patch just takes care of many low hanging fruits. It does not
> try to completely solves the issue.

Thanks.  All conversions in this patch look correct to me.

Will queue.