# [PATCH v4 0/2] rev-parse: fix some options when executed from subpath of main tree 5 messages from 2016-05-26 to 2016-05-27. Participants: Michael Rappazzo, Mike Hommey, Junio C Hamano. Thread: https://gitlist.dev/t/42455 ## Michael Rappazzo, 2016-05-26 11:19 Subject: [PATCH v4 0/2] rev-parse: fix some options when executed from subpath of main tree Message-ID: <1464261556-89722-1-git-send-email-rappazzo@gmail.com> URL: https://gitlist.dev/e/1464261556-89722-1-git-send-email-rappazzo%40gmail.com ``` Changes since v3 [1]: - Rebased onto 'pu' which includes the cleanup of t1500 by Eric Sunshine - Fixed a memory leak due to misusing xstrfmt() [1] http://thread.gmane.org/gmane.comp.version-control.git/293778 Michael Rappazzo (2): rev-parse tests: add tests executed from a subdirectory rev-parse: fix some options when executed from subpath of main tree builtin/rev-parse.c | 22 +++++++++++++++++----- t/t1500-rev-parse.sh | 28 ++++++++++++++++++++++++++++ t/t1700-split-index.sh | 17 +++++++++++++++++ t/t2027-worktree-list.sh | 10 +++++++++- 4 files changed, 71 insertions(+), 6 deletions(-) -- 2.8.0 ``` ## Michael Rappazzo, 2016-05-26 11:19 Subject: [PATCH v4 1/2] rev-parse tests: add tests executed from a subdirectory Message-ID: <1464261556-89722-2-git-send-email-rappazzo@gmail.com> URL: https://gitlist.dev/e/1464261556-89722-2-git-send-email-rappazzo%40gmail.com In-Reply-To: <1464261556-89722-1-git-send-email-rappazzo@gmail.com> ``` t2027-worktree-list has an incorrect expectation for --git-common-dir which has been adjusted and marked to expect failure. Some of the tests added have been marked to expect failure. These demonstrate a problem with the way that some options to git rev-parse behave when executed from a subdirectory of the main worktree. Signed-off-by: Michael Rappazzo --- t/t1500-rev-parse.sh | 28 ++++++++++++++++++++++++++++ t/t1700-split-index.sh | 17 +++++++++++++++++ t/t2027-worktree-list.sh | 12 ++++++++++-- 3 files changed, 55 insertions(+), 2 deletions(-) diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh index 038e24c..f39f783 100755 --- a/t/t1500-rev-parse.sh +++ b/t/t1500-rev-parse.sh @@ -87,4 +87,32 @@ test_rev_parse -C work -g ../repo.git -b t 'GIT_DIR=../repo.git, core.bare = tru test_rev_parse -C work -g ../repo.git -b u 'GIT_DIR=../repo.git, core.bare undefined' false false true '' +test_expect_success 'git-common-dir from worktree root' ' + echo .git >expect && + git rev-parse --git-common-dir >actual && + test_cmp expect actual +' + +test_expect_failure 'git-common-dir inside sub-dir' ' + mkdir -p path/to/child && + test_when_finished "rm -rf path" && + echo "$(git -C path/to/child rev-parse --show-cdup).git" >expect && + git -C path/to/child rev-parse --git-common-dir >actual && + test_cmp expect actual +' + +test_expect_success 'git-path from worktree root' ' + echo .git/objects >expect && + git rev-parse --git-path objects >actual && + test_cmp expect actual +' + +test_expect_failure 'git-path inside sub-dir' ' + mkdir -p path/to/child && + test_when_finished "rm -rf path" && + echo "$(git -C path/to/child rev-parse --show-cdup).git/objects" >expect && + git -C path/to/child rev-parse --git-path objects >actual && + test_cmp expect actual +' + test_done diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh index 8aef49f..8ca21bd 100755 --- a/t/t1700-split-index.sh +++ b/t/t1700-split-index.sh @@ -200,4 +200,21 @@ EOF test_cmp expect actual ' +test_expect_failure 'rev-parse --shared-index-path' ' + rm -rf .git && + test_create_repo . && + git update-index --split-index && + ls -t .git/sharedindex* | tail -n 1 >expect && + git rev-parse --shared-index-path >actual && + test_cmp expect actual && + mkdir work && + test_when_finished "rm -rf work" && + ( + cd work && + ls -t ../.git/sharedindex* | tail -n 1 >expect && + git rev-parse --shared-index-path >actual && + test_cmp expect actual + ) +' + test_done diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh index 1b1b65a..53cc5d3 100755 --- a/t/t2027-worktree-list.sh +++ b/t/t2027-worktree-list.sh @@ -8,16 +8,24 @@ test_expect_success 'setup' ' test_commit init ' -test_expect_success 'rev-parse --git-common-dir on main worktree' ' +test_expect_failure 'rev-parse --git-common-dir on main worktree' ' git rev-parse --git-common-dir >actual && echo .git >expected && test_cmp expected actual && mkdir sub && git -C sub rev-parse --git-common-dir >actual2 && - echo sub/.git >expected2 && + echo ../.git >expected2 && test_cmp expected2 actual2 ' +test_expect_failure 'rev-parse --git-path objects linked worktree' ' + echo "$(git rev-parse --show-toplevel)/.git/worktrees/linked-tree/objects" >expect && + test_when_finished "rm -rf linked-tree && git worktree prune" && + git worktree add --detach linked-tree master && + git -C linked-tree rev-parse --git-path objects >actual && + test_cmp expect actual +' + test_expect_success '"list" all worktrees from main' ' echo "$(git rev-parse --show-toplevel) $(git rev-parse --short HEAD) [$(git symbolic-ref --short HEAD)]" >expect && test_when_finished "rm -rf here && git worktree prune" && -- 2.8.0 ``` ## Michael Rappazzo, 2016-05-26 11:19 Subject: [PATCH v4 2/2] rev-parse: fix some options when executed from subpath of main tree Message-ID: <1464261556-89722-3-git-send-email-rappazzo@gmail.com> URL: https://gitlist.dev/e/1464261556-89722-3-git-send-email-rappazzo%40gmail.com In-Reply-To: <1464261556-89722-1-git-send-email-rappazzo@gmail.com> ``` Executing `git-rev-parse` with `--git-common-dir`, `--git-path `, or `--shared-index-path` from the root of the main worktree results in a relative path to the git dir. When executed from a subdirectory of the main tree, it can incorrectly return a path which starts 'sub/path/.git'. Change this to return the proper relative path to the git directory. Related tests marked to expect failure are updated to expect success Signed-off-by: Michael Rappazzo --- builtin/rev-parse.c | 22 +++++++++++++++++----- t/t1500-rev-parse.sh | 4 ++-- t/t1700-split-index.sh | 2 +- t/t2027-worktree-list.sh | 4 ++-- 4 files changed, 22 insertions(+), 10 deletions(-) diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c index c961b74..40579e0 100644 --- a/builtin/rev-parse.c +++ b/builtin/rev-parse.c @@ -564,10 +564,15 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix) } if (!strcmp(arg, "--git-path")) { + struct strbuf sb = STRBUF_INIT; + char *path; if (!argv[i + 1]) die("--git-path requires an argument"); - puts(git_path("%s", argv[i + 1])); - i++; + + path = xstrfmt("%s/%s", get_git_dir(), argv[++i]); + puts(relative_path(path, prefix, &sb)); + strbuf_release(&sb); + free(path); continue; } if (as_is) { @@ -787,8 +792,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix) continue; } if (!strcmp(arg, "--git-common-dir")) { - const char *pfx = prefix ? prefix : ""; - puts(prefix_filename(pfx, strlen(pfx), get_git_common_dir())); + struct strbuf sb = STRBUF_INIT; + puts(relative_path(get_git_common_dir(), prefix, &sb)); + strbuf_release(&sb); continue; } if (!strcmp(arg, "--is-inside-git-dir")) { @@ -811,7 +817,13 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix) die(_("Could not read the index")); if (the_index.split_index) { const unsigned char *sha1 = the_index.split_index->base_sha1; - puts(git_path("sharedindex.%s", sha1_to_hex(sha1))); + struct strbuf sb = STRBUF_INIT; + char *path; + + path = xstrfmt("%s/sharedindex.%s", get_git_dir(), sha1_to_hex(sha1)); + puts(relative_path(path, prefix, &sb)); + strbuf_release(&sb); + free(path); } continue; } diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh index f39f783..d74f09a 100755 --- a/t/t1500-rev-parse.sh +++ b/t/t1500-rev-parse.sh @@ -93,7 +93,7 @@ test_expect_success 'git-common-dir from worktree root' ' test_cmp expect actual ' -test_expect_failure 'git-common-dir inside sub-dir' ' +test_expect_success 'git-common-dir inside sub-dir' ' mkdir -p path/to/child && test_when_finished "rm -rf path" && echo "$(git -C path/to/child rev-parse --show-cdup).git" >expect && @@ -107,7 +107,7 @@ test_expect_success 'git-path from worktree root' ' test_cmp expect actual ' -test_expect_failure 'git-path inside sub-dir' ' +test_expect_success 'git-path inside sub-dir' ' mkdir -p path/to/child && test_when_finished "rm -rf path" && echo "$(git -C path/to/child rev-parse --show-cdup).git/objects" >expect && diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh index 8ca21bd..d2d9e02 100755 --- a/t/t1700-split-index.sh +++ b/t/t1700-split-index.sh @@ -200,7 +200,7 @@ EOF test_cmp expect actual ' -test_expect_failure 'rev-parse --shared-index-path' ' +test_expect_success 'rev-parse --shared-index-path' ' rm -rf .git && test_create_repo . && git update-index --split-index && diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh index 53cc5d3..16eec6e 100755 --- a/t/t2027-worktree-list.sh +++ b/t/t2027-worktree-list.sh @@ -8,7 +8,7 @@ test_expect_success 'setup' ' test_commit init ' -test_expect_failure 'rev-parse --git-common-dir on main worktree' ' +test_expect_success 'rev-parse --git-common-dir on main worktree' ' git rev-parse --git-common-dir >actual && echo .git >expected && test_cmp expected actual && @@ -18,7 +18,7 @@ test_expect_failure 'rev-parse --git-common-dir on main worktree' ' test_cmp expected2 actual2 ' -test_expect_failure 'rev-parse --git-path objects linked worktree' ' +test_expect_success 'rev-parse --git-path objects linked worktree' ' echo "$(git rev-parse --show-toplevel)/.git/worktrees/linked-tree/objects" >expect && test_when_finished "rm -rf linked-tree && git worktree prune" && git worktree add --detach linked-tree master && -- 2.8.0 ``` ## Mike Hommey, 2016-05-26 13:26 Subject: Re: [PATCH v4 2/2] rev-parse: fix some options when executed from subpath of main tree Message-ID: <20160526132611.GA2736@glandium.org> URL: https://gitlist.dev/e/20160526132611.GA2736%40glandium.org In-Reply-To: <1464261556-89722-3-git-send-email-rappazzo@gmail.com> ``` On Thu, May 26, 2016 at 07:19:16AM -0400, Michael Rappazzo wrote: > Executing `git-rev-parse` with `--git-common-dir`, `--git-path `, > or `--shared-index-path` from the root of the main worktree results in > a relative path to the git dir. > > When executed from a subdirectory of the main tree, it can incorrectly > return a path which starts 'sub/path/.git'. Change this to return the > proper relative path to the git directory. > > Related tests marked to expect failure are updated to expect success As mentioned previously (but late in the thread), I don't get why this one case of --git-common-dir should not return the same thing as --git-dir, which is an absolute directory. Especially when there is other flag telling you whether you are in the main or another worktree, so comparing the output for --git-dir and --git-common-dir is the easiest way to do so, but then you have to normalize them on their own because git returns different values pointing to the same directory. Mike ``` ## Junio C Hamano, 2016-05-27 18:50 Subject: Re: [PATCH v4 2/2] rev-parse: fix some options when executed from subpath of main tree Message-ID: URL: https://gitlist.dev/e/xmqqa8jbqqro.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20160526132611.GA2736@glandium.org> ``` Mike Hommey writes: > On Thu, May 26, 2016 at 07:19:16AM -0400, Michael Rappazzo wrote: >> Executing `git-rev-parse` with `--git-common-dir`, `--git-path `, >> or `--shared-index-path` from the root of the main worktree results in >> a relative path to the git dir. >> >> When executed from a subdirectory of the main tree, it can incorrectly >> return a path which starts 'sub/path/.git'. Change this to return the >> proper relative path to the git directory. >> >> Related tests marked to expect failure are updated to expect success > > As mentioned previously (but late in the thread), I don't get why this > one case of --git-common-dir should not return the same thing as > --git-dir, which is an absolute directory. Especially when there is > other flag telling you whether you are in the main or another worktree, > so comparing the output for --git-dir and --git-common-dir is the > easiest way to do so, but then you have to normalize them on their own > because git returns different values pointing to the same directory. Sounds like a sensible line of thought. A possible/plausible counter-argument from Michael's side that would be equally sensible might run along the lines of: An expected use of "git rev-parse --commit-dir" is to store the output in $GIT_DIR/$X so that the layout the worktree machinery expects can be set up by scripted Porcelains without using "git worktree". Making the value stored in $GIT_DIR/$X relative to $Y would help for such and such reasons. While making it easier to build a competing UI like that is a sensible goal, I do not think of what that $X or $Y are, and I do not think of what that "such and such reasons" are, either. And the cost of having to compare absolute --git-dir output with relative --git-common-dir (i.e. the justification for Mike's proposal to make --git-common-dir absolute) and the cost of having to turn absolute output from --git-common-dir to a path relative to $Y (i.e. the justification of making it relative in the hypothetical counter-argument) would be about the same, so it does not sound very compelling after all. ```