{"thread":{"id":"42115","subject":"[PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree","startedAt":"2016-04-22T21:53:08Z","lastAt":"2016-05-10T07:49:28Z","messageCount":12,"participants":["Michael Rappazzo","SZEDER Gábor","Mike Rappazzo","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"284195","messageId":"1461361992-91918-1-git-send-email-rappazzo@gmail.com","threadId":"42115","inReplyTo":null,"subject":"[PATCH v2 0/4] rev-parse: adjust results when they should be relative","fromName":"Michael Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2016-04-22T21:53:08Z","receivedAt":"2016-04-22T21:53:08Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"Differences from v1[1]:\n - Simplify implementation based on review comments\n - Included similar changes in rev-parse for --git-path and --shared-index-path\n - Added tests and separated them into individual commits\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/290669\n\nMichael Rappazzo (4):\n rev-parse: fix some options when executed from subpath of main tree\n t1500-rev-parse: add tests executed from sub path of the main worktree\n t2027-worktree-list: add and adjust tests related to git-rev-parse\n t1700-split-index: add test for rev-parse --shared-index-path\n\n builtin/rev-parse.c | 19 ++++++++++++++-----\n t/t1500-rev-parse.sh | 37 +++++++++++++++++++++++++++++++++++++\n t/t1700-split-index.sh | 17 +++++++++++++++++\n t/t2027-worktree-list.sh | 10 +++++++++-\n 4 files changed, 77 insertions(+), 6 deletions(-)\n\n-- \n2.8.0\n"},{"id":"284192","messageId":"1461361992-91918-2-git-send-email-rappazzo@gmail.com","threadId":"42115","inReplyTo":"1461361992-91918-1-git-send-email-rappazzo@gmail.com","subject":"[PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree","fromName":"Michael Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2016-04-22T21:53:09Z","receivedAt":"2016-04-22T21:53:09Z","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 `,\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, however, it incorrectly\nreturns a path which starts 'sub/path/.git'. Change this to return the\nproper relative path to the git directory.\n\nSigned-off-by: Michael Rappazzo \n---\n builtin/rev-parse.c | 19 ++++++++++++++-----\n 1 file changed, 14 insertions(+), 5 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}\n-- \n2.8.0\n"},{"id":"284196","messageId":"1461361992-91918-3-git-send-email-rappazzo@gmail.com","threadId":"42115","inReplyTo":"1461361992-91918-1-git-send-email-rappazzo@gmail.com","subject":"[PATCH v2 2/4] t1500-rev-parse: add tests executed from sub path of the main worktree","fromName":"Michael Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2016-04-22T21:53:10Z","receivedAt":"2016-04-22T21:53:10Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"Signed-off-by: Michael Rappazzo \n---\n t/t1500-rev-parse.sh | 37 +++++++++++++++++++++++++++++++++++++\n 1 file changed, 37 insertions(+)\n\ndiff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\nindex 48ee077..1e220f7 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,40 @@ 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+#cleanup from the above\n+cd ..\n+rm -r work\n+mv repo.git .git || exit 1\n+unset GIT_DIR\n+unset GIT_CONFIG\n+git config core.bare $original_core_bare\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_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+\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_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 &&\n+\tgit -C path/to/child rev-parse --git-path objects >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.8.0\n"},{"id":"284193","messageId":"1461361992-91918-4-git-send-email-rappazzo@gmail.com","threadId":"42115","inReplyTo":"1461361992-91918-1-git-send-email-rappazzo@gmail.com","subject":"[PATCH v2 3/4] t2027-worktree-list: add and adjust tests related to git-rev-parse","fromName":"Michael Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2016-04-22T21:53:11Z","receivedAt":"2016-04-22T21:53:11Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"Adjust the incorrect expectation for `rev-parse --git-common-dir`.\n\nAdd a test for `git rev-parse --git-path` executed from a linked\nworktree.\n\nSigned-off-by: Michael Rappazzo \n---\n t/t2027-worktree-list.sh | 10 +++++++++-\n 1 file changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh\nindex 1b1b65a..16eec6e 100755\n--- a/t/t2027-worktree-list.sh\n+++ b/t/t2027-worktree-list.sh\n@@ -14,10 +14,18 @@ test_expect_success 'rev-parse --git-common-dir on main worktree' '\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_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+\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":"284194","messageId":"1461361992-91918-5-git-send-email-rappazzo@gmail.com","threadId":"42115","inReplyTo":"1461361992-91918-1-git-send-email-rappazzo@gmail.com","subject":"[PATCH v2 4/4] t1700-split-index: add test for rev-parse --shared-index-path","fromName":"Michael Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2016-04-22T21:53:12Z","receivedAt":"2016-04-22T21:53:12Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"Signed-off-by: Michael Rappazzo \n---\n t/t1700-split-index.sh | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh\nindex 8aef49f..d2d9e02 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_success '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\n-- \n2.8.0\n"},{"id":"284923","messageId":"20160429142131.16282-1-szeder@ira.uka.de","threadId":"42115","inReplyTo":"1461361992-91918-2-git-send-email-rappazzo@gmail.com","subject":"Re: [PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-04-29T14:21:31Z","receivedAt":"2016-04-29T14:21:31Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"[Resend to list, sorry for the duplicates.]\n\n> Executing `git-rev-parse` with `--git-common-dir`, `--git-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, however, it incorrectly\n> returns a path which starts 'sub/path/.git'.\n\nThis is not completely true, because '--git-path ...' returns a\nrelative path starting with '.git':\n\n $ git -C t/ rev-parse --git-dir --git-path objects --git-common-dir\n /home/szeder/src/git/.git\n .git/objects\n t/.git\n\nIt's still wrong, of course.\n\n> Change this to return the\n> proper relative path to the git directory.\n\nI think returning absolute paths would be better. It is consistent\nwith the already properly working '--git-dir' option, which returns an\nabsolute path in this case. Furthermore, both '--git-path ...' and\n'--git-common-dir' already return absolute paths when run from a\nsubdirectory of the .git directory:\n\n $ git -C .git/refs rev-parse --git-dir --git-path objects --git-common-dir\n /home/szeder/src/git/.git\n /home/szeder/src/git/.git/objects\n /home/szeder/src/git/.git\n\n> Signed-off-by: Michael Rappazzo \n> ---\n> builtin/rev-parse.c | 19 ++++++++++++++-----\n> 1 file changed, 14 insertions(+), 5 deletions(-)\n\nThis patch doesn't add any new tests, while subsequent patches of the\nseries do nothing but add more tests. Splitting up your changes this\nway doesn't add any value, it only increases the number of commits. I\nthink either:\n\n - all those new tests could be added with this patch, or\n\n - if you want to add the test separately, then add them before\n this patch and mark them with 'test_expect_failure' to clearly\n demonstrate what the series is about to fix, and flip them to\n 'test_expect_success' in this patch.\n\n - An alternative way to split this series, following the \"Make\n separate commits for logically separate changes\" guideline, would\n be to fix and test these options in separate patches, i.e. fix and\n test '--git-path ...' in one patch, then fix and test\n '--git-common-dir' in the next, ...\n"},{"id":"284925","messageId":"20160429142206.16397-1-szeder@ira.uka.de","threadId":"42115","inReplyTo":"1461361992-91918-3-git-send-email-rappazzo@gmail.com","subject":"Re: [PATCH v2 2/4] t1500-rev-parse: add tests executed from sub path of the main worktree","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-04-29T14:22:06Z","receivedAt":"2016-04-29T14:22:06Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"[Resend to list, sorry for the duplicates.]\n\n> Signed-off-by: Michael Rappazzo \n> ---\n> t/t1500-rev-parse.sh | 37 +++++++++++++++++++++++++++++++++++++\n> 1 file changed, 37 insertions(+)\n> \n> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh\n> index 48ee077..1e220f7 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,40 @@ 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> +#cleanup from the above\n> +cd ..\n> +rm -r work\n> +mv repo.git .git || exit 1\n\nYou can't just 'exit 1' mid-script, because terminating the test script\nabruptly makes the test harness unhappy.\n\n> +unset GIT_DIR\n> +unset GIT_CONFIG\n\nBoth variables are set at this point, so calling plain 'unset' is OK.\nStill, I would suggest using 'sane_unset' instead, so the next person\nlooking at this test doesn't have to spend brain cycles on figuring\nout whether plain 'unset' is indeed safe or not.\n\n> +git config core.bare $original_core_bare\n\nThis whole '#cleanup from the above' block is just ugly. Not your\nfault, of course, but the consequence of how the preceeding tests were\nwritten in the past. I think it would be best if this series were\nscheduled on top of the 't1500 cleanup & modernization' patch I saw a\nfew days ago, then this block wouldn't be necessary at all.\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_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> +\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_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 &&\n> +\tgit -C path/to/child rev-parse --git-path objects >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n> -- \n> 2.8.0\n"},{"id":"284926","messageId":"20160429142248.16496-1-szeder@ira.uka.de","threadId":"42115","inReplyTo":"1461361992-91918-4-git-send-email-rappazzo@gmail.com","subject":"Re: [PATCH v2 3/4] t2027-worktree-list: add and adjust tests related to git-rev-parse","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-04-29T14:22:48Z","receivedAt":"2016-04-29T14:22:48Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"[Resend to list, sorry for the duplicates...]\n\n> Adjust the incorrect expectation for `rev-parse --git-common-dir`.\n> \n> Add a test for `git rev-parse --git-path` executed from a linked\n> worktree.\n> \n> Signed-off-by: Michael Rappazzo \n> ---\n> t/t2027-worktree-list.sh | 10 +++++++++-\n> 1 file changed, 9 insertions(+), 1 deletion(-)\n> \n> diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh\n> index 1b1b65a..16eec6e 100755\n> --- a/t/t2027-worktree-list.sh\n> +++ b/t/t2027-worktree-list.sh\n> @@ -14,10 +14,18 @@ test_expect_success 'rev-parse --git-common-dir on main worktree' '\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\nThis hunk must go into patch 1/4.\n\nThe full test suite should pass after every single commit. As patch\n1/4 fixes things, the changing behavior makes this test case fail,\nresulting in two commits in which 'make test' would fail.\n\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> +\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> -- \n> 2.8.0\n"},{"id":"285696","messageId":"CANoM8SWBzFiHGc3zAwndyx+GUkWQGDoaewVVQtH-06jazAn8uQ@mail.gmail.com","threadId":"42115","inReplyTo":"20160429135051.15492-1-szeder@ira.uka.de","subject":"Re: [PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree","fromName":"Mike Rappazzo","fromEmail":"rappazzo@gmail.com","sentAt":"2016-05-06T13:02:40Z","receivedAt":"2016-05-06T13:02:40Z","isPatch":true,"sender":{"key":"rappazzo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/525287?v=4"},"body":"On Fri, Apr 29, 2016 at 9:50 AM, SZEDER Gábor wrote:\n>> Executing `git-rev-parse` with `--git-common-dir`, `--git-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, however, it incorrectly\n>> returns a path which starts 'sub/path/.git'.\n>\n> This is not completely true, because '--git-path ...' returns a\n> relative path starting with '.git':\n>\n> $ git -C t/ rev-parse --git-dir --git-path objects --git-common-dir\n> /home/szeder/src/git/.git\n> .git/objects\n> t/.git\n>\n> It's still wrong, of course.\n\nI'll try to reword this to make it indicate that the value isn't\nalways incorrect.\n\n>\n>> Change this to return the\n>> proper relative path to the git directory.\n>\n> I think returning absolute paths would be better. It is consistent\n> with the already properly working '--git-dir' option, which returns an\n> absolute path in this case. Furthermore, both '--git-path ...' and\n> '--git-common-dir' already return absolute paths when run from a\n> subdirectory of the .git directory:\n>\n> $ git -C .git/refs rev-parse --git-dir --git-path objects --git-common-dir\n> /home/szeder/src/git/.git\n> /home/szeder/src/git/.git/objects\n> /home/szeder/src/git/.git\n>\n\nI agree with this, but at one point Junio suggested that it should\nreturn the relative path[1],\nso I went with that. Maybe I could RFC a separate patch that changes\nall of these to\nabsolute.\n\n>> Signed-off-by: Michael Rappazzo \n>> ---\n>> builtin/rev-parse.c | 19 ++++++++++++++-----\n>> 1 file changed, 14 insertions(+), 5 deletions(-)\n>\n> This patch doesn't add any new tests, while subsequent patches of the\n> series do nothing but add more tests. Splitting up your changes this\n> way doesn't add any value, it only increases the number of commits. I\n> think either:\n>\n> - all those new tests could be added with this patch, or\n>\n> - if you want to add the test separately, then add them before\n> this patch and mark them with 'test_expect_failure' to clearly\n> demonstrate what the series is about to fix, and flip them to\n> 'test_expect_success' in this patch.\n>\n> - An alternative way to split this series, following the \"Make\n> separate commits for logically separate changes\" guideline, would\n> be to fix and test these options in separate patches, i.e. fix and\n> test '--git-path ...' in one patch, then fix and test\n> '--git-common-dir' in the next, ...\n>\n>\n\nThanks, have a re-roll ready to go based on you input. I went with\noption 2 - add\ntests with expect failure and fix them with the next.\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/290061\n"},{"id":"285705","messageId":"20160506161312.Horde.7i8_sE5ISIqccneOIfinvCX@webmail.informatik.kit.edu","threadId":"42115","inReplyTo":"CANoM8SWBzFiHGc3zAwndyx+GUkWQGDoaewVVQtH-06jazAn8uQ@mail.gmail.com","subject":"Re: [PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2016-05-06T14:13:12Z","receivedAt":"2016-05-06T14:13:12Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nQuoting Mike Rappazzo :\n\n> On Fri, Apr 29, 2016 at 9:50 AM, SZEDER Gábor wrote:\n>>> Executing `git-rev-parse` with `--git-common-dir`, `--git-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, however, it incorrectly\n>>> returns a path which starts 'sub/path/.git'.\n>>\n>> This is not completely true, because '--git-path ...' returns a\n>> relative path starting with '.git':\n>>\n>> $ git -C t/ rev-parse --git-dir --git-path objects --git-common-dir\n>> /home/szeder/src/git/.git\n>> .git/objects\n>> t/.git\n>>\n>> It's still wrong, of course.\n>\n> I'll try to reword this to make it indicate that the value isn't\n> always incorrect.\n\nNot sure I understand your intention about rewording, in particular that\n\"isn't always incorrect\" part. Just to make sure there is no\nmisunderstanding let's have a look at the two broken cases:\n\n $ git -C t/ rev-parse --git-common-dir\n t/.git\n\nObviously wrong.\nThis is what you correctly described in the commit message as\n\"incorrectly returns a path which starts 'sub/path/.git'.\n\n $ git -C t/ rev-parse --git-path objects\n .git/objects\n\nWrong as well. It would be correct if we were at the top of the working\ntree, but we are not. It must be relative to the directory where '-C t/'\nbrought us.\nYour description in the commit message implies that '--git-path '\nis wrong in the same way as '--git-common-dir' is, i.e. in this case\nwould also return the relative path starting with the subdirectory.\nThat is apparently not the case.\n\nMy point in the previous email was that both are wrong when executed in\na subdir of the working tree, but they are wrong in different ways. And\nthey are always wrong when executed from a subdir of the working tree.\nI would have described the current wrong behavior as:\n\n When executed from a subdirectory of the working tree, however,\n '--git-common-dir' incorrectly returns a path which starts\n 'sub/path/.git', while '--git-path ' incorrectly returns a path\n relative to the top of the working tree.\n\n(Still hasn't looked at shared index...)\n\n>>> Change this to return the\n>>> proper relative path to the git directory.\n>>\n>> I think returning absolute paths would be better. It is consistent\n>> with the already properly working '--git-dir' option, which returns an\n>> absolute path in this case. Furthermore, both '--git-path ...' and\n>> '--git-common-dir' already return absolute paths when run from a\n>> subdirectory of the .git directory:\n>>\n>> $ git -C .git/refs rev-parse --git-dir --git-path objects --git-common-dir\n>> /home/szeder/src/git/.git\n>> /home/szeder/src/git/.git/objects\n>> /home/szeder/src/git/.git\n>>\n>\n> I agree with this, but at one point Junio suggested that it should\n> return the relative path[1],\n\nI wasn't aware of Junio's suggestion.\n\nIt shouldn't really matter in practice, because both the absolute and\nrelative paths will ultimately lead to the same place. However, I\nstill think that for consistency's sake absolute paths would be better\nwhen executed in a subdir of the working tree.\n"},{"id":"285755","messageId":"xmqq8tzn5d26.fsf@gitster.mtv.corp.google.com","threadId":"42115","inReplyTo":"20160506161312.Horde.7i8_sE5ISIqccneOIfinvCX@webmail.informatik.kit.edu","subject":"Re: [PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-06T17:08:49Z","receivedAt":"2016-05-06T17:08:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n>> I agree with this, but at one point Junio suggested that it should\n>> return the relative path[1],\n>\n> I wasn't aware of Junio's suggestion.\n\nMike justified his position to change everything to absolute, citing\nthat it is what \"rev-parse --show-toplevel\" does when run from a\nsubdirectory. I just gave a counter-example that rev-parse does not\nalways talk in absolute with \"rev-parse -C t --show-cdup\".\n\nIt hinted that his justification may not be strong enough to choose\nabsolute over relative, but wasn't meant to be a suggestion to\nchoose relative at all. At most, I was saying \"either is equally\nplausible\".\n\n> It shouldn't really matter in practice, because both the absolute and\n> relative paths will ultimately lead to the same place. However, I\n> still think that for consistency's sake absolute paths would be better\n> when executed in a subdir of the working tree.\n\nSure.\n\nOne thing I'd be worried about is that people may be using it to\ncompute paths and store them in $GIT_DIR/somewhere, and expecting\nthat they can later do a wholesale \"mv\" of directory hierarchy and\nrelative paths still work. I wouldn't be one of those who does the\n\"mv\" so I personally do not care, but I suspect some do.\n"},{"id":"286036","messageId":"CAPig+cSZ9PXr4Wogt2EJAJyapso8hZVucQ2yvJpCRK35a2j8jg@mail.gmail.com","threadId":"42115","inReplyTo":"20160506161312.Horde.7i8_sE5ISIqccneOIfinvCX@webmail.informatik.kit.edu","subject":"Re: [PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-10T07:49:28Z","receivedAt":"2016-05-10T07:49:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 6, 2016 at 10:13 AM, SZEDER Gábor wrote:\n> Quoting Mike Rappazzo :\n>> I'll try to reword this to make it indicate that the value isn't\n>> always incorrect.\n>\n> Not sure I understand your intention about rewording, in particular that\n> \"isn't always incorrect\" part. Just to make sure there is no\n> misunderstanding let's have a look at the two broken cases:\n>\n> $ git -C t/ rev-parse --git-common-dir\n> t/.git\n>\n> Obviously wrong.\n> This is what you correctly described in the commit message as\n> \"incorrectly returns a path which starts 'sub/path/.git'.\n>\n> $ git -C t/ rev-parse --git-path objects\n> .git/objects\n>\n> Wrong as well. It would be correct if we were at the top of the working\n> tree, but we are not. It must be relative to the directory where '-C t/'\n> brought us.\n> Your description in the commit message implies that '--git-path '\n> is wrong in the same way as '--git-common-dir' is, i.e. in this case\n> would also return the relative path starting with the subdirectory.\n> That is apparently not the case.\n>\n> My point in the previous email was that both are wrong when executed in\n> a subdir of the working tree, but they are wrong in different ways. And\n> they are always wrong when executed from a subdir of the working tree.\n\nThis explanation argues strongly in favor of placing each fix in a\nseparate patch[1] so that each commit message can be tailored to\nprecisely match the problem being fixed. Bundling the corresponding\ntests with the fix would be a bonus; no need for a lead-in patch\nintroducing the tests en masse, and no need for test_expect_failure.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/293001/\n"}]}