{"thread":{"id":"48532","subject":"[PATCH] t: make many tests depend less on the refs being files","startedAt":"2018-05-21T05:52:23Z","lastAt":"2018-05-23T05:33:30Z","messageCount":8,"participants":["Christian Couder","Johannes Schindelin","SZEDER Gábor","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"348180","messageId":"20180521055143.14701-1-chriscool@tuxfamily.org","threadId":"48532","inReplyTo":null,"subject":"[PATCH] t: make many tests depend less on the refs being files","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-05-21T05:51:43Z","receivedAt":"2018-05-21T05:52:23Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: David Turner <dturner@twopensource.com>\n\nSo that they work under alternate ref storage backends.\n\nThis will be really needed when such alternate ref storage backends are\ndeveloped. But this could already help by making clear to readers that\nsome tests do not depend on which ref backend is used.\n\nThis patch just takes care of many low hanging fruits. It does not try\nto completely solves the issue.\n\nSigned-off-by: David Turner <dturner@twopensource.com>\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n\nThanks for all the great feedback regarding implementing reftable [1].\n\nLooking at David Turner's tests in [2], it seems that they could indeed\nbe already valuable, so let's start by extracting most of the simple\nimprovements they make. \n\n[1] https://public-inbox.org/git/CAP8UFD0PPZSjBnxCA7ez91vBuatcHKQ+JUWvTD1iHcXzPBjPBg@mail.gmail.com/\n[2] https://github.com/dturner-tw/git/tree/dturner/pluggable-backends\n\n t/lib-t6000.sh                   |  5 ++---\n t/t1401-symbolic-ref.sh          |  2 +-\n t/t3200-branch.sh                | 18 +++++++++---------\n t/t3903-stash.sh                 |  2 +-\n t/t5500-fetch-pack.sh            | 10 +++++-----\n t/t5510-fetch.sh                 |  6 +++---\n t/t6010-merge-base.sh            |  2 +-\n t/t7201-co.sh                    |  2 +-\n t/t9104-git-svn-follow-parent.sh |  3 ++-\n t/t9903-bash-prompt.sh           |  2 +-\n 10 files changed, 26 insertions(+), 26 deletions(-)\n\ndiff --git a/t/lib-t6000.sh b/t/lib-t6000.sh\nindex 3f2d873fec..b8567cdf94 100644\n--- a/t/lib-t6000.sh\n+++ b/t/lib-t6000.sh\n@@ -4,11 +4,10 @@ mkdir -p .git/refs/tags\n \n >sed.script\n \n-# Answer the sha1 has associated with the tag. The tag must exist in .git/refs/tags\n+# Answer the sha1 has associated with the tag. The tag must exist under refs/tags\n tag () {\n \t_tag=$1\n-\ttest -f \".git/refs/tags/$_tag\" || error \"tag: \\\"$_tag\\\" does not exist\"\n-\tcat \".git/refs/tags/$_tag\"\n+\tgit rev-parse --verify \"refs/tags/$_tag\" || error \"tag: \\\"$_tag\\\" does not exist\"\n }\n \n # Generate a commit using the text specified to make it unique and the tree\ndiff --git a/t/t1401-symbolic-ref.sh b/t/t1401-symbolic-ref.sh\nindex 9e782a8122..a4ebb0b65f 100755\n--- a/t/t1401-symbolic-ref.sh\n+++ b/t/t1401-symbolic-ref.sh\n@@ -65,7 +65,7 @@ reset_to_sane\n test_expect_success 'symbolic-ref fails to delete real ref' '\n \techo \"fatal: Cannot delete refs/heads/foo, not a symbolic ref\" >expect &&\n \ttest_must_fail git symbolic-ref -d refs/heads/foo >actual 2>&1 &&\n-\ttest_path_is_file .git/refs/heads/foo &&\n+\tgit rev-parse --verify refs/heads/foo &&\n \ttest_cmp expect actual\n '\n reset_to_sane\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex c0ef946811..222dc2c377 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -234,34 +234,34 @@ test_expect_success 'git branch -M master2 master2 should work when master is ch\n \n test_expect_success 'git branch -v -d t should work' '\n \tgit branch t &&\n-\ttest_path_is_file .git/refs/heads/t &&\n+\tgit rev-parse --verify refs/heads/t &&\n \tgit branch -v -d t &&\n-\ttest_path_is_missing .git/refs/heads/t\n+\ttest_must_fail git rev-parse --verify refs/heads/t\n '\n \n test_expect_success 'git branch -v -m t s should work' '\n \tgit branch t &&\n-\ttest_path_is_file .git/refs/heads/t &&\n+\tgit rev-parse --verify refs/heads/t &&\n \tgit branch -v -m t s &&\n-\ttest_path_is_missing .git/refs/heads/t &&\n-\ttest_path_is_file .git/refs/heads/s &&\n+\ttest_must_fail git rev-parse --verify refs/heads/t &&\n+\tgit rev-parse --verify refs/heads/s &&\n \tgit branch -d s\n '\n \n test_expect_success 'git branch -m -d t s should fail' '\n \tgit branch t &&\n-\ttest_path_is_file .git/refs/heads/t &&\n+\tgit rev-parse refs/heads/t &&\n \ttest_must_fail git branch -m -d t s &&\n \tgit branch -d t &&\n-\ttest_path_is_missing .git/refs/heads/t\n+\ttest_must_fail git rev-parse refs/heads/t\n '\n \n test_expect_success 'git branch --list -d t should fail' '\n \tgit branch t &&\n-\ttest_path_is_file .git/refs/heads/t &&\n+\tgit rev-parse refs/heads/t &&\n \ttest_must_fail git branch --list -d t &&\n \tgit branch -d t &&\n-\ttest_path_is_missing .git/refs/heads/t\n+\ttest_must_fail git rev-parse refs/heads/t\n '\n \n test_expect_success 'git branch --list -v with --abbrev' '\ndiff --git a/t/t3903-stash.sh b/t/t3903-stash.sh\nindex aefde7b172..1f871d3cca 100755\n--- a/t/t3903-stash.sh\n+++ b/t/t3903-stash.sh\n@@ -726,7 +726,7 @@ test_expect_success 'store updates stash ref and reflog' '\n \tgit reset --hard &&\n \t! grep quux bazzy &&\n \tgit stash store -m quuxery $STASH_ID &&\n-\ttest $(cat .git/refs/stash) = $STASH_ID &&\n+\ttest $(git rev-parse stash) = $STASH_ID &&\n \tgit reflog --format=%H stash| grep $STASH_ID &&\n \tgit stash pop &&\n \tgrep quux bazzy\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex 0680dec808..886a9e3b72 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -30,7 +30,7 @@ add () {\n \ttest_tick &&\n \tcommit=$(echo \"$text\" | git commit-tree $tree $parents) &&\n \teval \"$name=$commit; export $name\" &&\n-\techo $commit > .git/refs/heads/$branch &&\n+\tgit update-ref refs/heads/$branch $commit &&\n \teval ${branch}TIP=$commit\n }\n \n@@ -45,10 +45,10 @@ pull_to_client () {\n \n \t\t\tcase \"$heads\" in\n \t\t\t    *A*)\n-\t\t\t\t    echo $ATIP > .git/refs/heads/A;;\n+\t\t\t\t    git update-ref refs/heads/A $ATIP;;\n \t\t\tesac &&\n \t\t\tcase \"$heads\" in *B*)\n-\t\t\t    echo $BTIP > .git/refs/heads/B;;\n+\t\t\t    git update-ref refs/heads/B $BTIP;;\n \t\t\tesac &&\n \t\t\tgit symbolic-ref HEAD refs/heads/$(echo $heads \\\n \t\t\t\t| sed -e \"s/^\\(.\\).*$/\\1/\") &&\n@@ -92,8 +92,8 @@ test_expect_success 'setup' '\n \t\tcur=$(($cur+1))\n \tdone &&\n \tadd B1 $A1 &&\n-\techo $ATIP > .git/refs/heads/A &&\n-\techo $BTIP > .git/refs/heads/B &&\n+\tgit update-ref refs/heads/A $ATIP &&\n+\tgit update-ref refs/heads/B $BTIP &&\n \tgit symbolic-ref HEAD refs/heads/B\n '\n \ndiff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh\nindex ae5a530a2d..e402aee6a2 100755\n--- a/t/t5510-fetch.sh\n+++ b/t/t5510-fetch.sh\n@@ -63,7 +63,7 @@ test_expect_success \"fetch test\" '\n \tgit commit -a -m \"updated by origin\" &&\n \tcd two &&\n \tgit fetch &&\n-\ttest -f .git/refs/heads/one &&\n+\tgit rev-parse --verify refs/heads/one &&\n \tmine=$(git rev-parse refs/heads/one) &&\n \this=$(cd ../one && git rev-parse refs/heads/master) &&\n \ttest \"z$mine\" = \"z$his\"\n@@ -73,8 +73,8 @@ test_expect_success \"fetch test for-merge\" '\n \tcd \"$D\" &&\n \tcd three &&\n \tgit fetch &&\n-\ttest -f .git/refs/heads/two &&\n-\ttest -f .git/refs/heads/one &&\n+\tgit rev-parse --verify refs/heads/two &&\n+\tgit rev-parse --verify refs/heads/one &&\n \tmaster_in_two=$(cd ../two && git rev-parse master) &&\n \tone_in_two=$(cd ../two && git rev-parse one) &&\n \t{\ndiff --git a/t/t6010-merge-base.sh b/t/t6010-merge-base.sh\nindex 31db7b5f91..56c4d91812 100755\n--- a/t/t6010-merge-base.sh\n+++ b/t/t6010-merge-base.sh\n@@ -34,7 +34,7 @@ doit () {\n \n \tcommit=$(echo $NAME | git commit-tree $T $PARENTS) &&\n \n-\techo $commit >.git/refs/tags/$NAME &&\n+\tgit update-ref refs/tags/$NAME $commit &&\n \techo $commit\n }\n \ndiff --git a/t/t7201-co.sh b/t/t7201-co.sh\nindex 76c223c967..ab9da61da3 100755\n--- a/t/t7201-co.sh\n+++ b/t/t7201-co.sh\n@@ -65,7 +65,7 @@ test_expect_success setup '\n test_expect_success \"checkout from non-existing branch\" '\n \n \tgit checkout -b delete-me master &&\n-\trm .git/refs/heads/delete-me &&\n+\tgit update-ref -d --no-deref refs/heads/delete-me &&\n \ttest refs/heads/delete-me = \"$(git symbolic-ref HEAD)\" &&\n \tgit checkout master &&\n \ttest refs/heads/master = \"$(git symbolic-ref HEAD)\"\ndiff --git a/t/t9104-git-svn-follow-parent.sh b/t/t9104-git-svn-follow-parent.sh\nindex a735fa3717..9c49b6c1fe 100755\n--- a/t/t9104-git-svn-follow-parent.sh\n+++ b/t/t9104-git-svn-follow-parent.sh\n@@ -215,7 +215,8 @@ test_expect_success \"multi-fetch continues to work\" \"\n \t\"\n \n test_expect_success \"multi-fetch works off a 'clean' repository\" '\n-\trm -r \"$GIT_DIR/svn\" \"$GIT_DIR/refs/remotes\" \"$GIT_DIR/logs\" &&\n+\trm -rf \"$GIT_DIR/svn\" \"$GIT_DIR/refs/remotes\" &&\n+\tgit reflog expire --all --expire=all &&\n \tmkdir \"$GIT_DIR/svn\" &&\n \tgit svn multi-fetch\n \t'\ndiff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh\nindex 8f5c811dd7..c3b89ae783 100755\n--- a/t/t9903-bash-prompt.sh\n+++ b/t/t9903-bash-prompt.sh\n@@ -148,7 +148,7 @@ test_expect_success 'prompt - inside .git directory' '\n test_expect_success 'prompt - deep inside .git directory' '\n \tprintf \" (GIT_DIR!)\" >expected &&\n \t(\n-\t\tcd .git/refs/heads &&\n+\t\tcd .git/objects &&\n \t\t__git_ps1 >\"$actual\"\n \t) &&\n \ttest_cmp expected \"$actual\"\n-- \n2.17.0.582.gccdcbd54c4.dirty\n\n"},{"id":"348184","messageId":"nycvar.QRO.7.76.6.1805211126140.77@tvgsbejvaqbjf.bet","threadId":"48532","inReplyTo":"20180521055143.14701-1-chriscool@tuxfamily.org","subject":"Re: [PATCH] t: make many tests depend less on the refs being files","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-05-21T09:41:33Z","receivedAt":"2018-05-21T09:41:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Chris,\n\nOn Mon, 21 May 2018, Christian Couder wrote:\n\n> From: David Turner <dturner@twopensource.com>\n\nI vaguely remember that Dave suggested using a different email address\nthese days...\n\n*clicketyclick*\n\nah, yes:\nhttps://public-inbox.org/git/1474935093-26757-3-git-send-email-dturner@twosigma.com/\n\nSo maybe update it here, too, to \n\n\tFrom: David Turner <novalis@novalis.org>\n\n?\n\n> So that they work under alternate ref storage backends.\n> \n> This will be really needed when such alternate ref storage backends are\n> developed. But this could already help by making clear to readers that\n> some tests do not depend on which ref backend is used.\n> \n> This patch just takes care of many low hanging fruits. It does not try\n> to completely solves the issue.\n> \n> Signed-off-by: David Turner <dturner@twopensource.com>\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n\nThis is great. I am all for making the tests better ;-)\n\n> diff --git a/t/lib-t6000.sh b/t/lib-t6000.sh\n> index 3f2d873fec..b8567cdf94 100644\n> --- a/t/lib-t6000.sh\n> +++ b/t/lib-t6000.sh\n> @@ -4,11 +4,10 @@ mkdir -p .git/refs/tags\n>  \n>  >sed.script\n>  \n> -# Answer the sha1 has associated with the tag. The tag must exist in .git/refs/tags\n> +# Answer the sha1 has associated with the tag. The tag must exist under refs/tags\n>  tag () {\n>  \t_tag=$1\n> -\ttest -f \".git/refs/tags/$_tag\" || error \"tag: \\\"$_tag\\\" does not exist\"\n> -\tcat \".git/refs/tags/$_tag\"\n> +\tgit rev-parse --verify \"refs/tags/$_tag\" || error \"tag: \\\"$_tag\\\" does not exist\"\n\nLine longer than 80 columns...\n\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index 0680dec808..886a9e3b72 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -30,7 +30,7 @@ add () {\n>  \ttest_tick &&\n>  \tcommit=$(echo \"$text\" | git commit-tree $tree $parents) &&\n>  \teval \"$name=$commit; export $name\" &&\n> -\techo $commit > .git/refs/heads/$branch &&\n> +\tgit update-ref refs/heads/$branch $commit &&\n\nI think we have to be careful here to quote both \"refs/heads/$branch\" and\n\"$commit\" here, as a bug that introduces spaces into $commit or $branch\nwould have been caught earlier, but not now, unless we quote.\n\nThis goes for all introduced `update-ref` calls.\n\nMaybe even for some `git rev-parse --verify` calls.\n\n> diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh\n> index 8f5c811dd7..c3b89ae783 100755\n> --- a/t/t9903-bash-prompt.sh\n> +++ b/t/t9903-bash-prompt.sh\n> @@ -148,7 +148,7 @@ test_expect_success 'prompt - inside .git directory' '\n>  test_expect_success 'prompt - deep inside .git directory' '\n>  \tprintf \" (GIT_DIR!)\" >expected &&\n>  \t(\n> -\t\tcd .git/refs/heads &&\n> +\t\tcd .git/objects &&\n>  \t\t__git_ps1 >\"$actual\"\n>  \t) &&\n>  \ttest_cmp expected \"$actual\"\n> -- \n\nThis one looks wrong.\n\nUpon cursory review, one might be tempted to assume that the file is now\nwritten into .git/objects/ instead of .git/refs/heads/. And the patch\ncontext provided in the email is not enough to see (gawd, I hate\nmail-based patch review, it really takes all my Git tools away from me).\nThe trick is that `actual` points at an absolute path:\n\n\t#!/bin/sh\n\t#\n\t# Copyright (c) 2012 SZEDER Gábor\n\t#\n\n\ttest_description='test git-specific bash prompt functions'\n\n\t. ./lib-bash.sh\n\n\t. \"$GIT_BUILD_DIR/contrib/completion/git-prompt.sh\"\n\n\tactual=\"$TRASH_DIRECTORY/actual\"\n\t[...]\n\nSo the remaining question (which probably wants to be added to the commit\nmessage together with a hint that `actual` points at an absolute path) is:\nWhy not `cd .git` instead?\n\nCiao,\nDscho"},{"id":"348193","messageId":"20180521114940.11288-1-szeder.dev@gmail.com","threadId":"48532","inReplyTo":"nycvar.QRO.7.76.6.1805211126140.77@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] t: make many tests depend less on the refs being files","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-05-21T11:49:40Z","receivedAt":"2018-05-21T11:50:09Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"> > diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh\n> > index 8f5c811dd7..c3b89ae783 100755\n> > --- a/t/t9903-bash-prompt.sh\n> > +++ b/t/t9903-bash-prompt.sh\n> > @@ -148,7 +148,7 @@ test_expect_success 'prompt - inside .git directory' '\n> >  test_expect_success 'prompt - deep inside .git directory' '\n> >  \tprintf \" (GIT_DIR!)\" >expected &&\n> >  \t(\n> > -\t\tcd .git/refs/heads &&\n> > +\t\tcd .git/objects &&\n> >  \t\t__git_ps1 >\"$actual\"\n> >  \t) &&\n> >  \ttest_cmp expected \"$actual\"\n> \n> This one looks wrong.\n\nIt's right, though.\n\n> Why not `cd .git` instead?\n\nThat case is covered in the previous test.\n\nOnce upon a time it mattered how deep we were in the .git directory\nwhen running __git_ps1(), because it executed different branches of a\nlong-ish if-elif chain.  For the prompt it doesn't matter anymore,\nbecause those ifs were eliminated, but for the completion script it\nstill does, which brings us to:\n\nChristian!  There is a similar test, '__git_find_repo_path - parent is\na .git directory' in 't9902-completion.sh', which, too, performs a 'cd\n.git/refs/heads'.\n\n"},{"id":"348196","messageId":"CAP8UFD3FZP3UqoSwL4z5haxfzPkONCX7m=o6PhfVDMhrasSAwA@mail.gmail.com","threadId":"48532","inReplyTo":"nycvar.QRO.7.76.6.1805211126140.77@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] t: make many tests depend less on the refs being files","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-05-21T12:30:35Z","receivedAt":"2018-05-21T12:30:40Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi Dscho,\n\nOn Mon, May 21, 2018 at 11:41 AM, Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n>\n> On Mon, 21 May 2018, Christian Couder wrote:\n>\n>> From: David Turner <dturner@twopensource.com>\n>\n> I vaguely remember that Dave suggested using a different email address\n> these days...\n>\n> *clicketyclick*\n>\n> ah, yes:\n> https://public-inbox.org/git/1474935093-26757-3-git-send-email-dturner@twosigma.com/\n\nYeah, I actually used \"David Turner <novalis@novalis.org>\" in the --cc\noption I gave to `git send-email`...\n\n> So maybe update it here, too, to\n>\n>         From: David Turner <novalis@novalis.org>\n\n...but I thought it was better to keep the original author and\nSigned-off-by as they are in the original commit:\n\nhttps://github.com/dturner-tw/git/commit/0a3fa7fbd7f99280b5f128be3e1ed04e045da671\n\nNow I am ok to update them if it is preferred.\n\n>> So that they work under alternate ref storage backends.\n>>\n>> This will be really needed when such alternate ref storage backends are\n>> developed. But this could already help by making clear to readers that\n>> some tests do not depend on which ref backend is used.\n>>\n>> This patch just takes care of many low hanging fruits. It does not try\n>> to completely solves the issue.\n>>\n>> Signed-off-by: David Turner <dturner@twopensource.com>\n>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n>> ---\n>\n> This is great. I am all for making the tests better ;-)\n\n;-)\n\n>> diff --git a/t/lib-t6000.sh b/t/lib-t6000.sh\n>> index 3f2d873fec..b8567cdf94 100644\n>> --- a/t/lib-t6000.sh\n>> +++ b/t/lib-t6000.sh\n>> @@ -4,11 +4,10 @@ mkdir -p .git/refs/tags\n>>\n>>  >sed.script\n>>\n>> -# Answer the sha1 has associated with the tag. The tag must exist in .git/refs/tags\n>> +# Answer the sha1 has associated with the tag. The tag must exist under refs/tags\n>>  tag () {\n>>       _tag=$1\n>> -     test -f \".git/refs/tags/$_tag\" || error \"tag: \\\"$_tag\\\" does not exist\"\n>> -     cat \".git/refs/tags/$_tag\"\n>> +     git rev-parse --verify \"refs/tags/$_tag\" || error \"tag: \\\"$_tag\\\" does not exist\"\n>\n> Line longer than 80 columns...\n\nOk, the 'error \"...\"' will be on another line in the next version.\n\n>> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n>> index 0680dec808..886a9e3b72 100755\n>> --- a/t/t5500-fetch-pack.sh\n>> +++ b/t/t5500-fetch-pack.sh\n>> @@ -30,7 +30,7 @@ add () {\n>>       test_tick &&\n>>       commit=$(echo \"$text\" | git commit-tree $tree $parents) &&\n>>       eval \"$name=$commit; export $name\" &&\n>> -     echo $commit > .git/refs/heads/$branch &&\n>> +     git update-ref refs/heads/$branch $commit &&\n>\n> I think we have to be careful here to quote both \"refs/heads/$branch\" and\n> \"$commit\" here, as a bug that introduces spaces into $commit or $branch\n> would have been caught earlier, but not now, unless we quote.\n>\n> This goes for all introduced `update-ref` calls.\n\nOk, they will all have quoted arguments in the next version.\n\n> Maybe even for some `git rev-parse --verify` calls.\n\nOk, I will take a look at that.\n\n>> diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh\n>> index 8f5c811dd7..c3b89ae783 100755\n>> --- a/t/t9903-bash-prompt.sh\n>> +++ b/t/t9903-bash-prompt.sh\n>> @@ -148,7 +148,7 @@ test_expect_success 'prompt - inside .git directory' '\n>>  test_expect_success 'prompt - deep inside .git directory' '\n>>       printf \" (GIT_DIR!)\" >expected &&\n>>       (\n>> -             cd .git/refs/heads &&\n>> +             cd .git/objects &&\n>>               __git_ps1 >\"$actual\"\n>>       ) &&\n>>       test_cmp expected \"$actual\"\n>> --\n>\n> This one looks wrong.\n>\n> Upon cursory review, one might be tempted to assume that the file is now\n> written into .git/objects/ instead of .git/refs/heads/. And the patch\n> context provided in the email is not enough to see (gawd, I hate\n> mail-based patch review, it really takes all my Git tools away from me).\n> The trick is that `actual` points at an absolute path:\n>\n>         #!/bin/sh\n>         #\n>         # Copyright (c) 2012 SZEDER Gábor\n>         #\n>\n>         test_description='test git-specific bash prompt functions'\n>\n>         . ./lib-bash.sh\n>\n>         . \"$GIT_BUILD_DIR/contrib/completion/git-prompt.sh\"\n>\n>         actual=\"$TRASH_DIRECTORY/actual\"\n>         [...]\n>\n> So the remaining question (which probably wants to be added to the commit\n> message together with a hint that `actual` points at an absolute path) is:\n> Why not `cd .git` instead?\n\nI think anywhere inside \".git/\" should work, so I guess\n\".git/refs/heads\" was chosen to make sure that adding anything after\n\".git/\" does not change the result.\n\nI think I can drop this for now and change it later in its own commit\nwith related explanations.\n\nThanks,\nChristian.\n"},{"id":"348198","messageId":"nycvar.QRO.7.76.6.1805211436230.77@tvgsbejvaqbjf.bet","threadId":"48532","inReplyTo":"nycvar.QRO.7.76.6.1805211126140.77@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] t: make many tests depend less on the refs being files","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-05-21T12:37:12Z","receivedAt":"2018-05-21T12:37:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nof course I messed up and did not fix the Cc: list... now fixed ;-)\n\n\nOn Mon, 21 May 2018, Johannes Schindelin wrote:\n\n> Hi Chris,\n> \n> On Mon, 21 May 2018, Christian Couder wrote:\n> \n> > From: David Turner <dturner@twopensource.com>\n> \n> I vaguely remember that Dave suggested using a different email address\n> these days...\n> \n> *clicketyclick*\n> \n> ah, yes:\n> https://public-inbox.org/git/1474935093-26757-3-git-send-email-dturner@twosigma.com/\n> \n> So maybe update it here, too, to \n> \n> \tFrom: David Turner <novalis@novalis.org>\n> \n> ?\n> \n> > So that they work under alternate ref storage backends.\n> > \n> > This will be really needed when such alternate ref storage backends are\n> > developed. But this could already help by making clear to readers that\n> > some tests do not depend on which ref backend is used.\n> > \n> > This patch just takes care of many low hanging fruits. It does not try\n> > to completely solves the issue.\n> > \n> > Signed-off-by: David Turner <dturner@twopensource.com>\n> > Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> > ---\n> \n> This is great. I am all for making the tests better ;-)\n> \n> > diff --git a/t/lib-t6000.sh b/t/lib-t6000.sh\n> > index 3f2d873fec..b8567cdf94 100644\n> > --- a/t/lib-t6000.sh\n> > +++ b/t/lib-t6000.sh\n> > @@ -4,11 +4,10 @@ mkdir -p .git/refs/tags\n> >  \n> >  >sed.script\n> >  \n> > -# Answer the sha1 has associated with the tag. The tag must exist in .git/refs/tags\n> > +# Answer the sha1 has associated with the tag. The tag must exist under refs/tags\n> >  tag () {\n> >  \t_tag=$1\n> > -\ttest -f \".git/refs/tags/$_tag\" || error \"tag: \\\"$_tag\\\" does not exist\"\n> > -\tcat \".git/refs/tags/$_tag\"\n> > +\tgit rev-parse --verify \"refs/tags/$_tag\" || error \"tag: \\\"$_tag\\\" does not exist\"\n> \n> Line longer than 80 columns...\n> \n> > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> > index 0680dec808..886a9e3b72 100755\n> > --- a/t/t5500-fetch-pack.sh\n> > +++ b/t/t5500-fetch-pack.sh\n> > @@ -30,7 +30,7 @@ add () {\n> >  \ttest_tick &&\n> >  \tcommit=$(echo \"$text\" | git commit-tree $tree $parents) &&\n> >  \teval \"$name=$commit; export $name\" &&\n> > -\techo $commit > .git/refs/heads/$branch &&\n> > +\tgit update-ref refs/heads/$branch $commit &&\n> \n> I think we have to be careful here to quote both \"refs/heads/$branch\" and\n> \"$commit\" here, as a bug that introduces spaces into $commit or $branch\n> would have been caught earlier, but not now, unless we quote.\n> \n> This goes for all introduced `update-ref` calls.\n> \n> Maybe even for some `git rev-parse --verify` calls.\n> \n> > diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh\n> > index 8f5c811dd7..c3b89ae783 100755\n> > --- a/t/t9903-bash-prompt.sh\n> > +++ b/t/t9903-bash-prompt.sh\n> > @@ -148,7 +148,7 @@ test_expect_success 'prompt - inside .git directory' '\n> >  test_expect_success 'prompt - deep inside .git directory' '\n> >  \tprintf \" (GIT_DIR!)\" >expected &&\n> >  \t(\n> > -\t\tcd .git/refs/heads &&\n> > +\t\tcd .git/objects &&\n> >  \t\t__git_ps1 >\"$actual\"\n> >  \t) &&\n> >  \ttest_cmp expected \"$actual\"\n> > -- \n> \n> This one looks wrong.\n> \n> Upon cursory review, one might be tempted to assume that the file is now\n> written into .git/objects/ instead of .git/refs/heads/. And the patch\n> context provided in the email is not enough to see (gawd, I hate\n> mail-based patch review, it really takes all my Git tools away from me).\n> The trick is that `actual` points at an absolute path:\n> \n> \t#!/bin/sh\n> \t#\n> \t# Copyright (c) 2012 SZEDER Gábor\n> \t#\n> \n> \ttest_description='test git-specific bash prompt functions'\n> \n> \t. ./lib-bash.sh\n> \n> \t. \"$GIT_BUILD_DIR/contrib/completion/git-prompt.sh\"\n> \n> \tactual=\"$TRASH_DIRECTORY/actual\"\n> \t[...]\n> \n> So the remaining question (which probably wants to be added to the commit\n> message together with a hint that `actual` points at an absolute path) is:\n> Why not `cd .git` instead?\n> \n> Ciao,\n> Dscho"},{"id":"348243","messageId":"CAGZ79kbdVm-U16rMjtgnGL+TOVRqkZz+J5W_eqo=dh42xRgKBQ@mail.gmail.com","threadId":"48532","inReplyTo":"20180521055143.14701-1-chriscool@tuxfamily.org","subject":"Re: [PATCH] t: make many tests depend less on the refs being files","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-05-21T19:34:57Z","receivedAt":"2018-05-21T19:41:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, May 20, 2018 at 10:51 PM, Christian Couder\n<christian.couder@gmail.com> wrote:\n> From: David Turner <dturner@twopensource.com>\n>\n> So that they work under alternate ref storage backends.\n\nSometimes I have a disconnect between the subject and the commit\nmessage, (e.g. in an email reader the subject is not displayed accurately on\ntop of the message).\n\nSo I would prefer if the first part of the body message is an actual\nsentence, and\nnot a continuum from the subject.\n\nMaybe elaborate a bit more:\n\n  The current tests are very focused on the file system representation of\n  the loose and packed refs code.  As there are plans to implement other\n  ref storage systems, migrate most tests to a form that test the intent of the\n  refs storage system instead of it internals. The internals of the loose and\n  packed refs are tested in <TODO>, whereas the tests in this patch focus\n  on testing other aspects.\n\n>\n> This will be really needed when such alternate ref storage backends are\n> developed. But this could already help by making clear to readers that\n> some tests do not depend on which ref backend is used.\n\nAh, this is what I picked up already in the suggested edit above. :/\n\n> This patch just takes care of many low hanging fruits. It does not try\n> to completely solves the issue.\n>\n> Signed-off-by: David Turner <dturner@twopensource.com>\n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n\n\n\n> ---\n>\n> Thanks for all the great feedback regarding implementing reftable [1].\n>\n> Looking at David Turner's tests in [2], it seems that they could indeed\n> be already valuable, so let's start by extracting most of the simple\n> improvements they make.\n\nThanks for tackling refstables!\n\nStefan\n"},{"id":"348353","messageId":"CAP8UFD1-dWLfK4EQMKrabneG+GcscBVZANpfZaOymQ4sWKzRnA@mail.gmail.com","threadId":"48532","inReplyTo":"20180521114940.11288-1-szeder.dev@gmail.com","subject":"Re: [PATCH] t: make many tests depend less on the refs being files","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-05-23T05:27:56Z","receivedAt":"2018-05-23T05:28:04Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, May 21, 2018 at 1:49 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:\n>> > diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh\n>> > index 8f5c811dd7..c3b89ae783 100755\n>> > --- a/t/t9903-bash-prompt.sh\n>> > +++ b/t/t9903-bash-prompt.sh\n>> > @@ -148,7 +148,7 @@ test_expect_success 'prompt - inside .git directory' '\n>> >  test_expect_success 'prompt - deep inside .git directory' '\n>> >     printf \" (GIT_DIR!)\" >expected &&\n>> >     (\n>> > -           cd .git/refs/heads &&\n>> > +           cd .git/objects &&\n>> >             __git_ps1 >\"$actual\"\n>> >     ) &&\n>> >     test_cmp expected \"$actual\"\n>>\n>> This one looks wrong.\n>\n> It's right, though.\n>\n>> Why not `cd .git` instead?\n>\n> That case is covered in the previous test.\n>\n> Once upon a time it mattered how deep we were in the .git directory\n> when running __git_ps1(), because it executed different branches of a\n> long-ish if-elif chain.  For the prompt it doesn't matter anymore,\n> because those ifs were eliminated, but for the completion script it\n> still does, which brings us to:\n\nThanks for the explanations!\n\n> Christian!  There is a similar test, '__git_find_repo_path - parent is\n> a .git directory' in 't9902-completion.sh', which, too, performs a 'cd\n> .git/refs/heads'.\n\nOk, I will take care of both of these tests in another patch.\n"},{"id":"348354","messageId":"CAP8UFD1c2oWd6SN+bWRmARnJCSmf+UuQpDWURxKYEwJyQ8oprA@mail.gmail.com","threadId":"48532","inReplyTo":"CAGZ79kbdVm-U16rMjtgnGL+TOVRqkZz+J5W_eqo=dh42xRgKBQ@mail.gmail.com","subject":"Re: [PATCH] t: make many tests depend less on the refs being files","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-05-23T05:33:25Z","receivedAt":"2018-05-23T05:33:30Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, May 21, 2018 at 9:34 PM, Stefan Beller <sbeller@google.com> wrote:\n> On Sun, May 20, 2018 at 10:51 PM, Christian Couder\n> <christian.couder@gmail.com> wrote:\n>> From: David Turner <dturner@twopensource.com>\n>>\n>> So that they work under alternate ref storage backends.\n>\n> Sometimes I have a disconnect between the subject and the commit\n> message, (e.g. in an email reader the subject is not displayed accurately on\n> top of the message).\n>\n> So I would prefer if the first part of the body message is an actual\n> sentence, and\n> not a continuum from the subject.\n>\n> Maybe elaborate a bit more:\n>\n>   The current tests are very focused on the file system representation of\n>   the loose and packed refs code.  As there are plans to implement other\n>   ref storage systems, migrate most tests to a form that test the intent of the\n>   refs storage system instead of it internals. The internals of the loose and\n>   packed refs are tested in <TODO>, whereas the tests in this patch focus\n>   on testing other aspects.\n\nThanks for this suggestion!\n\n>> This will be really needed when such alternate ref storage backends are\n>> developed. But this could already help by making clear to readers that\n>> some tests do not depend on which ref backend is used.\n>\n> Ah, this is what I picked up already in the suggested edit above. :/\n\nI actually mixed parts of your suggested message with parts of the\nexisting message in the V2 I just sent:\n\nhttps://public-inbox.org/git/20180523052517.4443-1-chriscool@tuxfamily.org/\n"}]}