threads / patch / 42115

v2, 4 partsrev-parse: fix some options when executed from subpath of main tree

Subject: [PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree

## tl;dr

12 messages between Apr 22, 2016 and May 10, 2016. Diffs are folded; open one to read it.

replies: 11people: 4as markdown or json

Michael Rappazzo· Apr 22, 2016, 21:53 UTC · lore

[PATCH v2 0/4] rev-parse: adjust results when they should be relative

Differences from v1[1]:
 - Simplify implementation based on review comments
 - Included similar changes in rev-parse for --git-path and --shared-index-path
 - Added tests and separated them into individual commits
[1] http://thread.gmane.org/gmane.comp.version-control.git/290669
Michael Rappazzo (4):
  rev-parse: fix some options when executed from subpath of main tree
  t1500-rev-parse: add tests executed from sub path of the main worktree
  t2027-worktree-list: add and adjust tests related to git-rev-parse
  t1700-split-index: add test for rev-parse --shared-index-path
 builtin/rev-parse.c      | 19 ++++++++++++++-----
 t/t1500-rev-parse.sh     | 37 +++++++++++++++++++++++++++++++++++++
 t/t1700-split-index.sh   | 17 +++++++++++++++++
 t/t2027-worktree-list.sh | 10 +++++++++-
 4 files changed, 77 insertions(+), 6 deletions(-)
-- 
2.8.0
Michael Rappazzo· Apr 22, 2016, 21:53 UTC · re: Michael Rappazzo · lore

Executing `git-rev-parse` with `--git-common-dir`, `--git-path <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, however, it incorrectly returns a path which starts 'sub/path/.git'. Change this to return the proper relative path to the git directory.

Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
---
 builtin/rev-parse.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)
Show changes to builtin/rev-parse.c +14 −5
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index c961b74..1da2e10 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -564,10 +564,13 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 		}
 
 		if (!strcmp(arg, "--git-path")) {
+			struct strbuf sb = STRBUF_INIT;
 			if (!argv[i + 1])
 				die("--git-path requires an argument");
-			puts(git_path("%s", argv[i + 1]));
-			i++;
+
+			puts(relative_path(xstrfmt("%s/%s", get_git_dir(), argv[++i]),
+				prefix, &sb));
+			strbuf_release(&sb);
 			continue;
 		}
 		if (as_is) {
@@ -787,8 +790,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 +815,12 @@ 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;
+
+					puts(relative_path(
+						xstrfmt("%s/sharedindex.%s", get_git_dir(), sha1_to_hex(sha1)),
+						prefix, &sb));
+					strbuf_release(&sb);
 				}
 				continue;
 			}

-- 
2.8.0
SZEDER Gábor· Apr 29, 2016, 14:21 UTC · re: Michael Rappazzo · lore

Re: [PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree

[Resend to list, sorry for the duplicates.]
Show 6 quoted lines
> Executing `git-rev-parse` with `--git-common-dir`, `--git-path <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, however, it incorrectly
> returns a path which starts 'sub/path/.git'.

This is not completely true, because '--git-path ...' returns a relative path starting with '.git':

  $ git -C t/ rev-parse --git-dir --git-path objects --git-common-dir
  /home/szeder/src/git/.git
  .git/objects
  t/.git
It's still wrong, of course.
> Change this to return the
> proper relative path to the git directory.

I think returning absolute paths would be better. It is consistent with the already properly working '--git-dir' option, which returns an absolute path in this case. Furthermore, both '--git-path ...' and '--git-common-dir' already return absolute paths when run from a subdirectory of the .git directory:

  $ git -C .git/refs rev-parse --git-dir --git-path objects --git-common-dir
  /home/szeder/src/git/.git
  /home/szeder/src/git/.git/objects
  /home/szeder/src/git/.git
> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---
>  builtin/rev-parse.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)

This patch doesn't add any new tests, while subsequent patches of the series do nothing but add more tests. Splitting up your changes this way doesn't add any value, it only increases the number of commits. I think either:

  - all those new tests could be added with this patch, or
  - if you want to add the test separately, then add them before
    this patch and mark them with 'test_expect_failure' to clearly
    demonstrate what the series is about to fix, and flip them to
    'test_expect_success' in this patch.
  - An alternative way to split this series, following the "Make
    separate commits for logically separate changes" guideline, would
    be to fix and test these options in separate patches, i.e. fix and
    test '--git-path ...' in one patch, then fix and test
    '--git-common-dir' in the next, ...
Michael Rappazzo· Apr 22, 2016, 21:53 UTC · re: Michael Rappazzo · lore

[PATCH v2 2/4] t1500-rev-parse: add tests executed from sub path of the main worktree

Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
---
 t/t1500-rev-parse.sh | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)
Show changes to t/t1500-rev-parse.sh +37 −0
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 48ee077..1e220f7 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -36,6 +36,7 @@ test_rev_parse() {
 # label is-bare is-inside-git is-inside-work prefix git-dir
 
 ROOT=$(pwd)
+original_core_bare=$(git config core.bare)
 
 test_rev_parse toplevel false false true '' .git
 
@@ -84,4 +85,40 @@ test_rev_parse 'GIT_DIR=../repo.git, core.bare = true' true false false ''
 git config --unset core.bare
 test_rev_parse 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
 
+#cleanup from the above
+cd ..
+rm -r work
+mv repo.git .git || exit 1
+unset GIT_DIR
+unset GIT_CONFIG
+git config core.bare $original_core_bare
+
+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_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 &&
+	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_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 &&
+	git -C path/to/child rev-parse --git-path objects >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
2.8.0
SZEDER Gábor· Apr 29, 2016, 14:22 UTC · re: Michael Rappazzo · lore

Re: [PATCH v2 2/4] t1500-rev-parse: add tests executed from sub path of the main worktree

[Resend to list, sorry for the duplicates.]
Show 25 quoted lines
> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---
>  t/t1500-rev-parse.sh | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> index 48ee077..1e220f7 100755
> --- a/t/t1500-rev-parse.sh
> +++ b/t/t1500-rev-parse.sh
> @@ -36,6 +36,7 @@ test_rev_parse() {
>  # label is-bare is-inside-git is-inside-work prefix git-dir
>  
>  ROOT=$(pwd)
> +original_core_bare=$(git config core.bare)
>  
>  test_rev_parse toplevel false false true '' .git
>  
> @@ -84,4 +85,40 @@ test_rev_parse 'GIT_DIR=../repo.git, core.bare = true' true false false ''
>  git config --unset core.bare
>  test_rev_parse 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
>  
> +#cleanup from the above
> +cd ..
> +rm -r work
> +mv repo.git .git || exit 1

You can't just 'exit 1' mid-script, because terminating the test script abruptly makes the test harness unhappy.

> +unset GIT_DIR
> +unset GIT_CONFIG

Both variables are set at this point, so calling plain 'unset' is OK. Still, I would suggest using 'sane_unset' instead, so the next person looking at this test doesn't have to spend brain cycles on figuring out whether plain 'unset' is indeed safe or not.

> +git config core.bare $original_core_bare

This whole '#cleanup from the above' block is just ugly. Not your fault, of course, but the consequence of how the preceeding tests were written in the past. I think it would be best if this series were scheduled on top of the 't1500 cleanup & modernization' patch I saw a few days ago, then this block wouldn't be necessary at all.

Show 31 quoted lines
> +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_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 &&
> +	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_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 &&
> +	git -C path/to/child rev-parse --git-path objects >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_done
> -- 
> 2.8.0
Michael Rappazzo· Apr 22, 2016, 21:53 UTC · re: Michael Rappazzo · lore

[PATCH v2 3/4] t2027-worktree-list: add and adjust tests related to git-rev-parse

Adjust the incorrect expectation for `rev-parse --git-common-dir`.

Add a test for `git rev-parse --git-path` executed from a linked worktree.

Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
---
 t/t2027-worktree-list.sh | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
Show changes to t/t2027-worktree-list.sh +9 −1
diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index 1b1b65a..16eec6e 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh
@@ -14,10 +14,18 @@ test_expect_success 'rev-parse --git-common-dir on main worktree' '
 	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_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 &&
+	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
SZEDER Gábor· Apr 29, 2016, 14:22 UTC · re: Michael Rappazzo · lore

Re: [PATCH v2 3/4] t2027-worktree-list: add and adjust tests related to git-rev-parse

[Resend to list, sorry for the duplicates...]
Show 22 quoted lines
> Adjust the incorrect expectation for `rev-parse --git-common-dir`.
> 
> Add a test for `git rev-parse --git-path` executed from a linked
> worktree.
> 
> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---
>  t/t2027-worktree-list.sh | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
> index 1b1b65a..16eec6e 100755
> --- a/t/t2027-worktree-list.sh
> +++ b/t/t2027-worktree-list.sh
> @@ -14,10 +14,18 @@ test_expect_success 'rev-parse --git-common-dir on main worktree' '
>  	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
>  '
This hunk must go into patch 1/4.

The full test suite should pass after every single commit. As patch 1/4 fixes things, the changing behavior makes this test case fail, resulting in two commits in which 'make test' would fail.

Show 13 quoted lines
> +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 &&
> +	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· Apr 22, 2016, 21:53 UTC · re: Michael Rappazzo · lore

[PATCH v2 4/4] t1700-split-index: add test for rev-parse --shared-index-path

Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
---
 t/t1700-split-index.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
Show changes to t/t1700-split-index.sh +17 −0
diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
index 8aef49f..d2d9e02 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -200,4 +200,21 @@ EOF
 	test_cmp expect actual
 '
 
+test_expect_success '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

-- 
2.8.0
Mike Rappazzo· May 6, 2016, 13:02 UTC · lore

Re: [PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree

On Fri, Apr 29, 2016 at 9:50 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
Show 16 quoted lines
>> Executing `git-rev-parse` with `--git-common-dir`, `--git-path <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, however, it incorrectly
>> returns a path which starts 'sub/path/.git'.
>
> This is not completely true, because '--git-path ...' returns a
> relative path starting with '.git':
>
>   $ git -C t/ rev-parse --git-dir --git-path objects --git-common-dir
>   /home/szeder/src/git/.git
>   .git/objects
>   t/.git
>
> It's still wrong, of course.

I'll try to reword this to make it indicate that the value isn't always incorrect.

Show 15 quoted lines
>
>> Change this to return the
>> proper relative path to the git directory.
>
> I think returning absolute paths would be better.  It is consistent
> with the already properly working '--git-dir' option, which returns an
> absolute path in this case.  Furthermore, both '--git-path ...' and
> '--git-common-dir' already return absolute paths when run from a
> subdirectory of the .git directory:
>
>   $ git -C .git/refs rev-parse --git-dir --git-path objects --git-common-dir
>   /home/szeder/src/git/.git
>   /home/szeder/src/git/.git/objects
>   /home/szeder/src/git/.git
>

I agree with this, but at one point Junio suggested that it should return the relative path[1], so I went with that. Maybe I could RFC a separate patch that changes all of these to absolute.

Show 24 quoted lines
>> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
>> ---
>>  builtin/rev-parse.c | 19 ++++++++++++++-----
>>  1 file changed, 14 insertions(+), 5 deletions(-)
>
> This patch doesn't add any new tests, while subsequent patches of the
> series do nothing but add more tests.  Splitting up your changes this
> way doesn't add any value, it only increases the number of commits.  I
> think either:
>
>   - all those new tests could be added with this patch, or
>
>   - if you want to add the test separately, then add them before
>     this patch and mark them with 'test_expect_failure' to clearly
>     demonstrate what the series is about to fix, and flip them to
>     'test_expect_success' in this patch.
>
>   - An alternative way to split this series, following the "Make
>     separate commits for logically separate changes" guideline, would
>     be to fix and test these options in separate patches, i.e. fix and
>     test '--git-path ...' in one patch, then fix and test
>     '--git-common-dir' in the next, ...
>
>

Thanks, have a re-roll ready to go based on you input. I went with option 2 - add tests with expect failure and fix them with the next.

[1] http://article.gmane.org/gmane.comp.version-control.git/290061
SZEDER Gábor· May 6, 2016, 14:13 UTC · re: Mike Rappazzo · lore

Re: [PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree

Quoting Mike Rappazzo <rappazzo@gmail.com>:
Show 20 quoted lines
> On Fri, Apr 29, 2016 at 9:50 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>>> Executing `git-rev-parse` with `--git-common-dir`, `--git-path <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, however, it incorrectly
>>> returns a path which starts 'sub/path/.git'.
>>
>> This is not completely true, because '--git-path ...' returns a
>> relative path starting with '.git':
>>
>>  $ git -C t/ rev-parse --git-dir --git-path objects --git-common-dir
>>  /home/szeder/src/git/.git
>>  .git/objects
>>  t/.git
>>
>> It's still wrong, of course.
>
> I'll try to reword this to make it indicate that the value isn't
> always incorrect.

Not sure I understand your intention about rewording, in particular that "isn't always incorrect" part. Just to make sure there is no misunderstanding let's have a look at the two broken cases:

    $ git -C t/ rev-parse --git-common-dir
    t/.git

Obviously wrong. This is what you correctly described in the commit message as "incorrectly returns a path which starts 'sub/path/.git'.

    $ git -C t/ rev-parse --git-path objects
    .git/objects

Wrong as well. It would be correct if we were at the top of the working tree, but we are not. It must be relative to the directory where '-C t/' brought us. Your description in the commit message implies that '--git-path <path>' is wrong in the same way as '--git-common-dir' is, i.e. in this case would also return the relative path starting with the subdirectory. That is apparently not the case.

My point in the previous email was that both are wrong when executed in a subdir of the working tree, but they are wrong in different ways. And they are always wrong when executed from a subdir of the working tree. I would have described the current wrong behavior as:

    When executed from a subdirectory of the working tree, however,
    '--git-common-dir' incorrectly returns a path which starts
    'sub/path/.git', while '--git-path <path>' incorrectly returns a path
    relative to the top of the working tree.
(Still hasn't looked at shared index...)
Show 17 quoted lines
>>> Change this to return the
>>> proper relative path to the git directory.
>>
>> I think returning absolute paths would be better.  It is consistent
>> with the already properly working '--git-dir' option, which returns an
>> absolute path in this case.  Furthermore, both '--git-path ...' and
>> '--git-common-dir' already return absolute paths when run from a
>> subdirectory of the .git directory:
>>
>>  $ git -C .git/refs rev-parse --git-dir --git-path objects --git-common-dir
>>  /home/szeder/src/git/.git
>>  /home/szeder/src/git/.git/objects
>>  /home/szeder/src/git/.git
>>
>
> I agree with this, but at one point Junio suggested that it should
> return the relative path[1],
I wasn't aware of Junio's suggestion.

It shouldn't really matter in practice, because both the absolute and relative paths will ultimately lead to the same place. However, I still think that for consistency's sake absolute paths would be better when executed in a subdir of the working tree.

Junio C Hamano· May 6, 2016, 17:08 UTC · re: SZEDER Gábor · lore

Re: [PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree

SZEDER Gábor <szeder@ira.uka.de> writes:
>> I agree with this, but at one point Junio suggested that it should
>> return the relative path[1],
>
> I wasn't aware of Junio's suggestion.

Mike justified his position to change everything to absolute, citing that it is what "rev-parse --show-toplevel" does when run from a subdirectory. I just gave a counter-example that rev-parse does not always talk in absolute with "rev-parse -C t --show-cdup".

It hinted that his justification may not be strong enough to choose absolute over relative, but wasn't meant to be a suggestion to choose relative at all. At most, I was saying "either is equally plausible".

> It shouldn't really matter in practice, because both the absolute and
> relative paths will ultimately lead to the same place.  However, I
> still think that for consistency's sake absolute paths would be better
> when executed in a subdir of the working tree.
Sure.

One thing I'd be worried about is that people may be using it to compute paths and store them in $GIT_DIR/somewhere, and expecting that they can later do a wholesale "mv" of directory hierarchy and relative paths still work. I wouldn't be one of those who does the "mv" so I personally do not care, but I suspect some do.

Eric Sunshine· May 10, 2016, 07:49 UTC · re: SZEDER Gábor · lore

Re: [PATCH v2 1/4] rev-parse: fix some options when executed from subpath of main tree

On Fri, May 6, 2016 at 10:13 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
Show 29 quoted lines
> Quoting Mike Rappazzo <rappazzo@gmail.com>:
>> I'll try to reword this to make it indicate that the value isn't
>> always incorrect.
>
> Not sure I understand your intention about rewording, in particular that
> "isn't always incorrect" part.  Just to make sure there is no
> misunderstanding let's have a look at the two broken cases:
>
>    $ git -C t/ rev-parse --git-common-dir
>    t/.git
>
> Obviously wrong.
> This is what you correctly described in the commit message as
> "incorrectly returns a path which starts 'sub/path/.git'.
>
>    $ git -C t/ rev-parse --git-path objects
>    .git/objects
>
> Wrong as well.  It would be correct if we were at the top of the working
> tree, but we are not.  It must be relative to the directory where '-C t/'
> brought us.
> Your description in the commit message implies that '--git-path <path>'
> is wrong in the same way as '--git-common-dir' is, i.e. in this case
> would also return the relative path starting with the subdirectory.
> That is apparently not the case.
>
> My point in the previous email was that both are wrong when executed in
> a subdir of the working tree, but they are wrong in different ways.  And
> they are always wrong when executed from a subdir of the working tree.

This explanation argues strongly in favor of placing each fix in a separate patch[1] so that each commit message can be tailored to precisely match the problem being fixed. Bundling the corresponding tests with the fix would be a bonus; no need for a lead-in patch introducing the tests en masse, and no need for test_expect_failure.

[1]: http://article.gmane.org/gmane.comp.version-control.git/293001/

← back to recent threads