{"thread":{"id":"42249","subject":"[PATCH v3 0/2] [PATCH v3 0/2] rev-parse: fix some options when executed from subpath of main tree","startedAt":"2016-05-06T13:35:18Z","lastAt":"2016-05-10T07:38:34Z","messageCount":8,"participants":["Michael Rappazzo","Junio C Hamano","Mike Rappazzo","Eric Sunshine"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"285700","messageId":"1462541720-79553-1-git-send-email-rappazzo@gmail.com","threadId":"42249","inReplyTo":null,"subject":"[PATCH v3 0/2] [PATCH v3 0/2] rev-parse: fix some options when executed from subpath of main tree","fromName":"Michael Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2016-05-06T13:35:18Z","receivedAt":"2016-05-06T13:35:18Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"Differenced from v2[1]:\n - Rewrote the commits to just two; one that introduces tests, and \n   one which fixes the problem.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/292272\n\nMichael Rappazzo (2):\n  rev-parse tests: add tests executed from a subdirectory\n  rev-parse: fix some options when executed from subpath of main tree\n\n builtin/rev-parse.c      | 19 ++++++++++++++-----\n t/t1500-rev-parse.sh     | 38 ++++++++++++++++++++++++++++++++++++++\n t/t1700-split-index.sh   | 17 +++++++++++++++++\n t/t2027-worktree-list.sh | 10 +++++++++-\n 4 files changed, 78 insertions(+), 6 deletions(-)\n\n-- \n2.8.0\n"},{"id":"285702","messageId":"1462541720-79553-2-git-send-email-rappazzo@gmail.com","threadId":"42249","inReplyTo":"1462541720-79553-1-git-send-email-rappazzo@gmail.com","subject":"[PATCH v3 1/2] rev-parse tests: add tests executed from a subdirectory","fromName":"Michael Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2016-05-06T13:35:19Z","receivedAt":"2016-05-06T13:35:19Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"t1500-rev-parse contains envrionment leaks (changing dir without\nchanging back, setting config variables, etc).  Add a test to clean this\nup up so that future tests can be added without worry of any setting\nfrom a previous test.\n\nt2027-worktree-list has an incorrect expectation for --git-common-dir\nwhich has been adjusted and marked to expect failure.\n\nSome of the tests added have been marked to expect failure.  These\ndemonstrate a problem with the way that some options to git rev-parse\nbehave when executed from a subdirectory of the main worktree.\n\nSigned-off-by: Michael Rappazzo <rappazzo@gmail.com>\n---\n t/t1500-rev-parse.sh     | 38 ++++++++++++++++++++++++++++++++++++++\n t/t1700-split-index.sh   | 17 +++++++++++++++++\n t/t2027-worktree-list.sh | 12 ++++++++++--\n 3 files changed, 65 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex 48ee077..442ca46 100755\n--- a/t/t1500-rev-parse.sh\n+++ b/t/t1500-rev-parse.sh\n@@ -36,6 +36,7 @@ test_rev_parse() {\n # label is-bare is-inside-git is-inside-work prefix git-dir\n \n ROOT=$(pwd)\n+original_core_bare=$(git config core.bare)\n \n test_rev_parse toplevel false false true '' .git\n \n@@ -84,4 +85,41 @@ test_rev_parse 'GIT_DIR=../repo.git, core.bare = true' true false false ''\n git config --unset core.bare\n test_rev_parse 'GIT_DIR=../repo.git, core.bare undefined' false false true ''\n \n+test_expect_success 'cleanup from the previous tests' '\n+\tcd .. &&\n+\trm -r work &&\n+\tmv repo.git .git &&\n+\tunset GIT_DIR &&\n+\tunset GIT_CONFIG &&\n+\tgit config core.bare $original_core_bare\n+'\n+\n+test_expect_success 'git-common-dir from worktree root' '\n+\techo .git >expect &&\n+\tgit rev-parse --git-common-dir >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'git-common-dir inside sub-dir' '\n+\tmkdir -p path/to/child &&\n+\ttest_when_finished \"rm -rf path\" &&\n+\techo \"$(git -C path/to/child rev-parse --show-cdup).git\" >expect &&\n+\tgit -C path/to/child rev-parse --git-common-dir >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'git-path from worktree root' '\n+\techo .git/objects >expect &&\n+\tgit rev-parse --git-path objects >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'git-path inside sub-dir' '\n+\tmkdir -p path/to/child &&\n+\ttest_when_finished \"rm -rf path\" &&\n+\techo \"$(git -C path/to/child rev-parse --show-cdup).git/objects\" >expect &&\n+\tgit -C path/to/child rev-parse --git-path objects >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\nindex 8aef49f..8ca21bd 100755\n--- a/t/t1700-split-index.sh\n+++ b/t/t1700-split-index.sh\n@@ -200,4 +200,21 @@ EOF\n \ttest_cmp expect actual\n '\n \n+test_expect_failure 'rev-parse --shared-index-path' '\n+\trm -rf .git &&\n+\ttest_create_repo . &&\n+\tgit update-index --split-index &&\n+\tls -t .git/sharedindex* | tail -n 1 >expect &&\n+\tgit rev-parse --shared-index-path >actual &&\n+\ttest_cmp expect actual &&\n+\tmkdir work &&\n+\ttest_when_finished \"rm -rf work\" &&\n+\t(\n+\t\tcd work &&\n+\t\tls -t ../.git/sharedindex* | tail -n 1 >expect &&\n+\t\tgit rev-parse --shared-index-path >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\ndiff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh\nindex 1b1b65a..53cc5d3 100755\n--- a/t/t2027-worktree-list.sh\n+++ b/t/t2027-worktree-list.sh\n@@ -8,16 +8,24 @@ test_expect_success 'setup' '\n \ttest_commit init\n '\n \n-test_expect_success 'rev-parse --git-common-dir on main worktree' '\n+test_expect_failure 'rev-parse --git-common-dir on main worktree' '\n \tgit rev-parse --git-common-dir >actual &&\n \techo .git >expected &&\n \ttest_cmp expected actual &&\n \tmkdir sub &&\n \tgit -C sub rev-parse --git-common-dir >actual2 &&\n-\techo sub/.git >expected2 &&\n+\techo ../.git >expected2 &&\n \ttest_cmp expected2 actual2\n '\n \n+test_expect_failure 'rev-parse --git-path objects linked worktree' '\n+\techo \"$(git rev-parse --show-toplevel)/.git/worktrees/linked-tree/objects\" >expect &&\n+\ttest_when_finished \"rm -rf linked-tree && git worktree prune\" &&\n+\tgit worktree add --detach linked-tree master &&\n+\tgit -C linked-tree rev-parse --git-path objects >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '\"list\" all worktrees from main' '\n \techo \"$(git rev-parse --show-toplevel) $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]\" >expect &&\n \ttest_when_finished \"rm -rf here && git worktree prune\" &&\n-- \n2.8.0\n"},{"id":"285701","messageId":"1462541720-79553-3-git-send-email-rappazzo@gmail.com","threadId":"42249","inReplyTo":"1462541720-79553-1-git-send-email-rappazzo@gmail.com","subject":"[PATCH v3 2/2] rev-parse: fix some options when executed from subpath of main tree","fromName":"Michael Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2016-05-06T13:35:20Z","receivedAt":"2016-05-06T13:35:20Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"Executing `git-rev-parse` with `--git-common-dir`, `--git-path <path>`,\nor `--shared-index-path` from the root of the main worktree results in\na relative path to the git dir.\n\nWhen executed from a subdirectory of the main tree, it can incorrectly\nreturn a path which starts 'sub/path/.git'.  Change this to return the\nproper relative path to the git directory.\n\nRelated tests marked to expect failure are updated to expect success\n\nSigned-off-by: Michael Rappazzo <rappazzo@gmail.com>\n---\n builtin/rev-parse.c      | 19 ++++++++++++++-----\n t/t1500-rev-parse.sh     |  4 ++--\n t/t1700-split-index.sh   |  2 +-\n t/t2027-worktree-list.sh |  4 ++--\n 4 files changed, 19 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex c961b74..1da2e10 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -564,10 +564,13 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t}\n \n \t\tif (!strcmp(arg, \"--git-path\")) {\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n \t\t\tif (!argv[i + 1])\n \t\t\t\tdie(\"--git-path requires an argument\");\n-\t\t\tputs(git_path(\"%s\", argv[i + 1]));\n-\t\t\ti++;\n+\n+\t\t\tputs(relative_path(xstrfmt(\"%s/%s\", get_git_dir(), argv[++i]),\n+\t\t\t\tprefix, &sb));\n+\t\t\tstrbuf_release(&sb);\n \t\t\tcontinue;\n \t\t}\n \t\tif (as_is) {\n@@ -787,8 +790,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--git-common-dir\")) {\n-\t\t\t\tconst char *pfx = prefix ? prefix : \"\";\n-\t\t\t\tputs(prefix_filename(pfx, strlen(pfx), get_git_common_dir()));\n+\t\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\t\tputs(relative_path(get_git_common_dir(), prefix, &sb));\n+\t\t\t\tstrbuf_release(&sb);\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--is-inside-git-dir\")) {\n@@ -811,7 +815,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n \t\t\t\t\tdie(_(\"Could not read the index\"));\n \t\t\t\tif (the_index.split_index) {\n \t\t\t\t\tconst unsigned char *sha1 = the_index.split_index->base_sha1;\n-\t\t\t\t\tputs(git_path(\"sharedindex.%s\", sha1_to_hex(sha1)));\n+\t\t\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\n+\t\t\t\t\tputs(relative_path(\n+\t\t\t\t\t\txstrfmt(\"%s/sharedindex.%s\", get_git_dir(), sha1_to_hex(sha1)),\n+\t\t\t\t\t\tprefix, &sb));\n+\t\t\t\t\tstrbuf_release(&sb);\n \t\t\t\t}\n \t\t\t\tcontinue;\n \t\t\t}\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex 442ca46..cc89392 100755\n--- a/t/t1500-rev-parse.sh\n+++ b/t/t1500-rev-parse.sh\n@@ -100,7 +100,7 @@ test_expect_success 'git-common-dir from worktree root' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'git-common-dir inside sub-dir' '\n+test_expect_success 'git-common-dir inside sub-dir' '\n \tmkdir -p path/to/child &&\n \ttest_when_finished \"rm -rf path\" &&\n \techo \"$(git -C path/to/child rev-parse --show-cdup).git\" >expect &&\n@@ -114,7 +114,7 @@ test_expect_success 'git-path from worktree root' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'git-path inside sub-dir' '\n+test_expect_success 'git-path inside sub-dir' '\n \tmkdir -p path/to/child &&\n \ttest_when_finished \"rm -rf path\" &&\n \techo \"$(git -C path/to/child rev-parse --show-cdup).git/objects\" >expect &&\ndiff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\nindex 8ca21bd..d2d9e02 100755\n--- a/t/t1700-split-index.sh\n+++ b/t/t1700-split-index.sh\n@@ -200,7 +200,7 @@ EOF\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'rev-parse --shared-index-path' '\n+test_expect_success 'rev-parse --shared-index-path' '\n \trm -rf .git &&\n \ttest_create_repo . &&\n \tgit update-index --split-index &&\ndiff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh\nindex 53cc5d3..16eec6e 100755\n--- a/t/t2027-worktree-list.sh\n+++ b/t/t2027-worktree-list.sh\n@@ -8,7 +8,7 @@ test_expect_success 'setup' '\n \ttest_commit init\n '\n \n-test_expect_failure 'rev-parse --git-common-dir on main worktree' '\n+test_expect_success 'rev-parse --git-common-dir on main worktree' '\n \tgit rev-parse --git-common-dir >actual &&\n \techo .git >expected &&\n \ttest_cmp expected actual &&\n@@ -18,7 +18,7 @@ test_expect_failure 'rev-parse --git-common-dir on main worktree' '\n \ttest_cmp expected2 actual2\n '\n \n-test_expect_failure 'rev-parse --git-path objects linked worktree' '\n+test_expect_success 'rev-parse --git-path objects linked worktree' '\n \techo \"$(git rev-parse --show-toplevel)/.git/worktrees/linked-tree/objects\" >expect &&\n \ttest_when_finished \"rm -rf linked-tree && git worktree prune\" &&\n \tgit worktree add --detach linked-tree master &&\n-- \n2.8.0\n"},{"id":"285785","messageId":"xmqqy47m25z4.fsf@gitster.mtv.corp.google.com","threadId":"42249","inReplyTo":"1462541720-79553-2-git-send-email-rappazzo@gmail.com","subject":"Re: [PATCH v3 1/2] rev-parse tests: add tests executed from a subdirectory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-06T22:10:07Z","receivedAt":"2016-05-06T22:10:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Rappazzo <rappazzo@gmail.com> writes:\n\n> t1500-rev-parse contains envrionment leaks (changing dir without\n> changing back, setting config variables, etc).  Add a test to clean this\n> up up so that future tests can be added without worry of any setting\n> from a previous test.\n\nThis is a wonderful thing to do, but...\n\n>  test_rev_parse toplevel false false true '' .git\n>  \n> @@ -84,4 +85,41 @@ test_rev_parse 'GIT_DIR=../repo.git, core.bare = true' true false false ''\n>  git config --unset core.bare\n>  test_rev_parse 'GIT_DIR=../repo.git, core.bare undefined' false false true ''\n>  \n> +test_expect_success 'cleanup from the previous tests' '\n> +\tcd .. &&\n> +\trm -r work &&\n\nInstead of cleaning things up like this, could you please please\nplease fix these existing tests that chdir around without being in a\nsubshell?  If the \"previous tests\" failed before going down as this\nstep expects, the \"cd .. && rm -r\" can make things worse.\n\n> +\tmv repo.git .git &&\n> +\tunset GIT_DIR &&\n> +\tunset GIT_CONFIG &&\n\nThe spirit of this change is to make the test more independent from\nthe effects of what happened previously.  Use sane_unset so that\nwe do not have to worry about previous step that may have failed\nbefore it has a chance to set GIT_DIR and GIT_CONFIG (which would\ncause these unset to fail).\n\n> +\tgit config core.bare $original_core_bare\n\nIs this (rather, the capturing of $original_core_bare up above)\nnecessary?  We are in the default 'trash' repository when the\ncapturing happens, and we know it is not a bare repository, right?\n"},{"id":"285818","messageId":"CANoM8SVr1_G6KevbGSHifGyQS-ei57q+5D+GE_QmKvf_ysF2Sg@mail.gmail.com","threadId":"42249","inReplyTo":"xmqqy47m25z4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 1/2] rev-parse tests: add tests executed from a subdirectory","fromName":"Mike Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2016-05-07T13:35:29Z","receivedAt":"2016-05-07T13:35:29Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"On Fri, May 6, 2016 at 6:10 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Michael Rappazzo <rappazzo@gmail.com> writes:\n>\n>> t1500-rev-parse contains envrionment leaks (changing dir without\n>> changing back, setting config variables, etc).  Add a test to clean this\n>> up up so that future tests can be added without worry of any setting\n>> from a previous test.\n>\n> This is a wonderful thing to do, but...\n>\n>>  test_rev_parse toplevel false false true '' .git\n>>\n>> @@ -84,4 +85,41 @@ test_rev_parse 'GIT_DIR=../repo.git, core.bare = true' true false false ''\n>>  git config --unset core.bare\n>>  test_rev_parse 'GIT_DIR=../repo.git, core.bare undefined' false false true ''\n>>\n>> +test_expect_success 'cleanup from the previous tests' '\n>> +     cd .. &&\n>> +     rm -r work &&\n>\n> Instead of cleaning things up like this, could you please please\n> please fix these existing tests that chdir around without being in a\n> subshell?  If the \"previous tests\" failed before going down as this\n> step expects, the \"cd .. && rm -r\" can make things worse.\n\nI still have fixing this test up on my do-to list.  My previous\nattempt[1] had some flaws\nin addition to some objection to the approach I took to expand each test. Eric\nSunshine suggested using a table approach, but I am not sure if that can be done\ncleanly.\n\nI figured that a fix to the rev-parse code would supersede test\ncleanup, so I separated\nmy efforts.\n\nI originally copied the pattern from above this code:\n\n> +#cleanup from the above\n> +cd ..\n> +rm -r work\n> +mv repo.git .git || exit 1\n\nbut Gábor had an objection to it [2].  So I went with this simple cleanup test.\n\nI could move it back to outside of a test, and do some checks around\nit.  Something\nlike:\n\n    dir=$(pwd)\n    target=${dir##*/}\n    if [ \"$target\" == \"work\" ]\n    then\n        cd ..\n        rm -r \"work\"\n    fi\n\n>\n>> +     mv repo.git .git &&\n>> +     unset GIT_DIR &&\n>> +     unset GIT_CONFIG &&\n>\n> The spirit of this change is to make the test more independent from\n> the effects of what happened previously.  Use sane_unset so that\n> we do not have to worry about previous step that may have failed\n> before it has a chance to set GIT_DIR and GIT_CONFIG (which would\n> cause these unset to fail).\n>\n>> +     git config core.bare $original_core_bare\n>\n> Is this (rather, the capturing of $original_core_bare up above)\n> necessary?  We are in the default 'trash' repository when the\n> capturing happens, and we know it is not a bare repository, right?\n\nMy goal was to have the test be in the state exactly as it was at the\nbeginning of\nthe test.  Right above my cleanup test this line is executed:\n\n> git config --unset core.bare\n\nI just wanted to be absolutely sure that the value was the same.  I\ncould certainly\nsimplify it to assume core.bare is \"true\" though.\n\nThanks,\n_Mike\n\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/291729\n[2] http://article.gmane.org/gmane.comp.version-control.git/293003\n"},{"id":"285892","messageId":"xmqqvb2ozapp.fsf@gitster.mtv.corp.google.com","threadId":"42249","inReplyTo":"CANoM8SVr1_G6KevbGSHifGyQS-ei57q+5D+GE_QmKvf_ysF2Sg@mail.gmail.com","subject":"Re: [PATCH v3 1/2] rev-parse tests: add tests executed from a subdirectory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-08T18:05:54Z","receivedAt":"2016-05-08T18:05:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Rappazzo <rappazzo@gmail.com> writes:\n\n>> Instead of cleaning things up like this, could you please please\n>> please fix these existing tests that chdir around without being in a\n>> subshell?  If the \"previous tests\" failed before going down as this\n>> step expects, the \"cd .. && rm -r\" can make things worse.\n\n> I originally copied the pattern from above this code:\n> ...\n> but Gábor had an objection to it [2].  So I went with this simple cleanup test.\n\nThe \"|| exit 1\" you see everywhere is a sure sign that these tests\nare not designed correctly.  These existing tests that do too many\nthings outside test_expect_success block needs to be redone, so that\neach of them can do what it wants to test even when previous ones\nfailed.\n\nWhat I had in mind was more a long the lines of this change. I only\ndid the first several just for illustration (I added an 'exit' to\nmark where I stopped), but I think you get the idea.  The point is\nthat by doing things in subprocess inside test_expect_success you\ncan avoid disrupting the main test process even when a test fails as\nmuch as possible, and this illustrates how you would deal with the\n\"cd\" and \"export\".  What I didn't handle was the updates to\n.git/config whose effects by earlier tests are relied on later tests\nin this illustration.\n\nAs to \"table driven\" vs \"explicitly spelling out scripts, accepting\nsome code repetition\", I tend to favor the latter slightly, unless\nthe table driven approach is done in such a way that each of its\ntests are truly independent from one another, i.e. if a row of the\ntable represents one test, the tests would not be disrupted by\nreordering the rows, inserting a new row in the middle, or removing\nan existing row.  From my experience, \"table-driven\" sets of tests\nwhose rows have inter-dependency has been much harder to debug when\nI had to hunt down a bug that manifests itself only in one test in\nthe middle.\n\nHope this helps clarify what I meant.\n\n t/t1500-rev-parse.sh | 82 ++++++++++++++++++++++++++++++----------------------\n 1 file changed, 48 insertions(+), 34 deletions(-)\n\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex 48ee077..22b52c0 100755\n--- a/t/t1500-rev-parse.sh\n+++ b/t/t1500-rev-parse.sh\n@@ -5,65 +5,79 @@ test_description='test git rev-parse'\n \n test_rev_parse() {\n \tname=$1\n-\tshift\n-\n-\ttest_expect_success \"$name: is-bare-repository\" \\\n-\t\"test '$1' = \\\"\\$(git rev-parse --is-bare-repository)\\\"\"\n+\tprep=$2\n+\tshift 2\n+\n+\ttest_expect_success \"$name: is-bare-repository\" \"\n+\t(\n+\t\t$prep &&\n+\t\ttest '$1' = \\\"\\$(git rev-parse --is-bare-repository)\\\"\n+\t)\"\n \tshift\n \t[ $# -eq 0 ] && return\n \n-\ttest_expect_success \"$name: is-inside-git-dir\" \\\n-\t\"test '$1' = \\\"\\$(git rev-parse --is-inside-git-dir)\\\"\"\n+\ttest_expect_success \"$name: is-inside-git-dir\" \"\n+\t(\n+\t\t$prep &&\n+\t\ttest '$1' = \\\"\\$(git rev-parse --is-inside-git-dir)\\\"\n+\t)\"\n \tshift\n \t[ $# -eq 0 ] && return\n \n-\ttest_expect_success \"$name: is-inside-work-tree\" \\\n-\t\"test '$1' = \\\"\\$(git rev-parse --is-inside-work-tree)\\\"\"\n+\ttest_expect_success \"$name: is-inside-work-tree\" \"\n+\t(\n+\t\t$prep &&\n+\t\ttest '$1' = \\\"\\$(git rev-parse --is-inside-work-tree)\\\"\n+\t)\"\n \tshift\n \t[ $# -eq 0 ] && return\n \n-\ttest_expect_success \"$name: prefix\" \\\n-\t\"test '$1' = \\\"\\$(git rev-parse --show-prefix)\\\"\"\n+\ttest_expect_success \"$name: prefix\" \"\n+\t(\n+\t\t$prep &&\n+\t\ttest '$1' = \\\"\\$(git rev-parse --show-prefix)\\\"\n+\t)\"\n \tshift\n \t[ $# -eq 0 ] && return\n \n-\ttest_expect_success \"$name: git-dir\" \\\n-\t\"test '$1' = \\\"\\$(git rev-parse --git-dir)\\\"\"\n+\ttest_expect_success \"$name: git-dir\" \"\n+\t(\n+\t\t$prep &&\n+\t\ttest '$1' = \\\"\\$(git rev-parse --git-dir)\\\"\n+\t)\"\n \tshift\n \t[ $# -eq 0 ] && return\n }\n \n-# label is-bare is-inside-git is-inside-work prefix git-dir\n+# label prep is-bare is-inside-git is-inside-work prefix git-dir\n \n ROOT=$(pwd)\n+mkdir -p sub/dir work\n \n-test_rev_parse toplevel false false true '' .git\n+test_rev_parse toplevel : false false true '' .git\n \n-cd .git || exit 1\n-test_rev_parse .git/ false true false '' .\n-cd objects || exit 1\n-test_rev_parse .git/objects/ false true false '' \"$ROOT/.git\"\n-cd ../.. || exit 1\n+test_rev_parse .git 'cd .git' false true false '' .\n \n-mkdir -p sub/dir || exit 1\n-cd sub/dir || exit 1\n-test_rev_parse subdirectory false false true sub/dir/ \"$ROOT/.git\"\n-cd ../.. || exit 1\n+test_rev_parse .git/objects/ 'cd .git/objects' false true false '' \"$ROOT/.git\"\n \n-git config core.bare true\n-test_rev_parse 'core.bare = true' true false false\n+test_rev_parse subdirectory 'cd sub/dir' \\\n+\tfalse false true sub/dir/ \"$ROOT/.git\"\n \n-git config --unset core.bare\n-test_rev_parse 'core.bare undefined' false false true\n+test_rev_parse 'core.bare = true' 'git config core.bare true' \\\n+\ttrue false false\n \n-mkdir work || exit 1\n-cd work || exit 1\n-GIT_DIR=../.git\n-GIT_CONFIG=\"$(pwd)\"/../.git/config\n-export GIT_DIR GIT_CONFIG\n+test_rev_parse 'core.bare undefined' 'git config --unset core.bare || :' \\\n+\tfalse false true\n \n-git config core.bare false\n-test_rev_parse 'GIT_DIR=../.git, core.bare = false' false false true ''\n+test_rev_parse 'GIT_DIR=../.git, core.bare = false' '\n+\tcd work && \n+\tGIT_DIR=../.git &&\n+\tGIT_CONFIG=\"$(pwd)\"/../.git/config &&\n+\texport GIT_DIR GIT_CONFIG &&\n+\tgit config core.bare false' \\\n+\tfalse false true ''\n+\n+exit\n \n git config core.bare true\n test_rev_parse 'GIT_DIR=../.git, core.bare = true' true false false ''\n"},{"id":"285897","messageId":"xmqq37psz8u8.fsf@gitster.mtv.corp.google.com","threadId":"42249","inReplyTo":"xmqqvb2ozapp.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 1/2] rev-parse tests: add tests executed from a subdirectory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-08T18:46:23Z","receivedAt":"2016-05-08T18:46:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> What I had in mind was more a long the lines of this change. I only\n> did the first several just for illustration (I added an 'exit' to\n> mark where I stopped), but I think you get the idea.  The point is\n> that by doing things in subprocess inside test_expect_success you\n> can avoid disrupting the main test process even when a test fails as\n> much as possible, and this illustrates how you would deal with the\n> \"cd\" and \"export\".  What I didn't handle was the updates to\n> .git/config whose effects by earlier tests are relied on later tests\n> in this illustration.\n>\n> As to \"table driven\" vs \"explicitly spelling out scripts, accepting\n> some code repetition\", I tend to favor the latter slightly, unless\n> the table driven approach is done in such a way that each of its\n> tests are truly independent from one another, i.e. if a row of the\n> table represents one test, the tests would not be disrupted by\n> reordering the rows, inserting a new row in the middle, or removing\n> an existing row.  From my experience, \"table-driven\" sets of tests\n> whose rows have inter-dependency has been much harder to debug when\n> I had to hunt down a bug that manifests itself only in one test in\n> the middle.\n>\n> Hope this helps clarify what I meant.\n\n\nIf you want to go forward in this direction, three are a few\nadditional things to note.\n\n> +\ttest_expect_success \"$name: is-bare-repository\" \"\n> +\t(\n> +\t\t$prep &&\n> +\t\ttest '$1' = \\\"\\$(git rev-parse --is-bare-repository)\\\"\n> +\t)\"\n\nThis is not a new problem, but quoting the test body with dq instead\nof sq pair make it ugly to read.  We might want to do something like\n\n\texpect=$1\n\ttest_expect_success \"$name: is-bare-repository\" '\n\t(\n\t\t$prep &&\n\t\ttest \"$outcome\" = \"$(git rev-parse --is-bare-repository)\"\n\t)'\n\n>  \tshift\n>  \t[ $# -eq 0 ] && return\n\nAlso, let's avoid []; i.e.\n\n\ttest $# = 0 && return\n\n> -# label is-bare is-inside-git is-inside-work prefix git-dir\n> +# label prep is-bare is-inside-git is-inside-work prefix git-dir\n>  \n>  ROOT=$(pwd)\n> +mkdir -p sub/dir work\n\nRemember this place in the script, I may refer to it later.\n\n> -git config core.bare true\n> -test_rev_parse 'core.bare = true' true false false\n> +test_rev_parse subdirectory 'cd sub/dir' \\\n> +\tfalse false true sub/dir/ \"$ROOT/.git\"\n>  \n> -git config --unset core.bare\n> -test_rev_parse 'core.bare undefined' false false true\n> +test_rev_parse 'core.bare = true' 'git config core.bare true' \\\n> +\ttrue false false\n\nI didn't use \"test_config\" and \"test_unconfig\" here, because you\ncannot use them in a subprocess.  If we truly want to make these\ntests independent from each other, we'd need to come up with a way\nfor the tests to revert the configuration state back to where it was\nbefore they started.\n>  \n> -mkdir work || exit 1\n> -cd work || exit 1\n> -GIT_DIR=../.git\n> -GIT_CONFIG=\"$(pwd)\"/../.git/config\n> -export GIT_DIR GIT_CONFIG\n>  \n> -git config core.bare false\n> -test_rev_parse 'GIT_DIR=../.git, core.bare = false' false false true ''\n> +test_rev_parse 'GIT_DIR=../.git, core.bare = false' '\n> +\tcd work && \n> +\tGIT_DIR=../.git &&\n> +\tGIT_CONFIG=\"$(pwd)\"/../.git/config &&\n> +\texport GIT_DIR GIT_CONFIG &&\n> +\tgit config core.bare false' \\\n> +\tfalse false true ''\n\nThe \"prep\" step for this test is long and I'd assume that the later\ntests would need to do something similar themselves.  To avoid\nrepetition, create a helper shell function to reduce boilerplate\nbefore the series of these tests start (i.e. the place above) and\ncall it from the prep parts with different arguments, e.g. making\nthe above test into\n\ntest_rev_parse 'GIT_DIR=../.git, core.bare = false' '\n\tcd work && \n\tset_common_env ../.git false' \\\n\tfalse false true ''\n\nby having something like\n\nset_dir_and_core_bare () {\n\tGIT_DIR=$1 &&\n\tGIT_CONFIG=\"$(pwd)/$GIT_DIR/config\" &&\n\texport GIT_DIR GIT_CONFIG &&\n\tgit config core.bare \"$2\"\n}\n\n> +exit\n>  \n>  git config core.bare true\n>  test_rev_parse 'GIT_DIR=../.git, core.bare = true' true false false ''\n"},{"id":"286035","messageId":"CAPig+cQ9+=JcNguvuFN9GwHf1M1aLDNeyY8dJQLMkEGSXrU6Tg@mail.gmail.com","threadId":"42249","inReplyTo":"1462541720-79553-3-git-send-email-rappazzo@gmail.com","subject":"Re: [PATCH v3 2/2] rev-parse: fix some options when executed from subpath of main tree","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-10T07:38:34Z","receivedAt":"2016-05-10T07:38:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 6, 2016 at 9:35 AM, Michael Rappazzo <rappazzo@gmail.com> wrote:\n> Executing `git-rev-parse` with `--git-common-dir`, `--git-path <path>`,\n> or `--shared-index-path` from the root of the main worktree results in\n> a relative path to the git dir.\n>\n> When executed from a subdirectory of the main tree, it can incorrectly\n> return a path which starts 'sub/path/.git'.  Change this to return the\n> proper relative path to the git directory.\n>\n> Related tests marked to expect failure are updated to expect success\n>\n> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>\n> ---\n> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\n> @@ -564,10 +564,13 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n>                 if (!strcmp(arg, \"--git-path\")) {\n> +                       struct strbuf sb = STRBUF_INIT;\n>                         if (!argv[i + 1])\n>                                 die(\"--git-path requires an argument\");\n> -                       puts(git_path(\"%s\", argv[i + 1]));\n> -                       i++;\n> +\n> +                       puts(relative_path(xstrfmt(\"%s/%s\", get_git_dir(), argv[++i]),\n> +                               prefix, &sb));\n\nThis is leaking the result of xstrfmt().\n\n> +                       strbuf_release(&sb);\n>                         continue;\n>                 }\n> @@ -811,7 +815,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)\n>                                 if (the_index.split_index) {\n>                                         const unsigned char *sha1 = the_index.split_index->base_sha1;\n> -                                       puts(git_path(\"sharedindex.%s\", sha1_to_hex(sha1)));\n> +                                       struct strbuf sb = STRBUF_INIT;\n> +\n> +                                       puts(relative_path(\n> +                                               xstrfmt(\"%s/sharedindex.%s\", get_git_dir(), sha1_to_hex(sha1)),\n> +                                               prefix, &sb));\n\nLikewise leaking xstrfmt().\n\n> +                                       strbuf_release(&sb);\n>                                 }\n>                                 continue;\n>                         }\n"}]}