[PATCH] rev-parse --git-path: fix output when running in a subdirectory

[PATCH] rev-parse --git-path: fix output when running in a subdirectory

[Johannes Schindelin]

In addition to making git_path() aware of certain file names that need
to be handled differently e.g. when running in worktrees, the commit
557bd833bb (git_path(): be aware of file relocation in $GIT_DIR,
2014-11-30) also snuck in a new option for `git rev-parse`:
`--git-path`.

On the face of it, there is no obvious bug in that commit's diff: it
faithfully calls git_path() on the argument and prints it out, i.e. `git
rev-parse --git-path <filename>` has the same precise behavior as
calling `git_path("<filename>")` in C.

The problem lies deeper, much deeper. In hindsight (which is always
unfair), implementing the .git/ directory discovery in
`setup_git_directory()` by changing the working directory may have
allowed us to avoid passing around a struct that contains information
about the current repository, but it bought us many, many problems.

In this case, when being called in a subdirectory, `git rev-parse`
changes the working directory to the top-level directory before calling
`git_path()`. In the new working directory, the result is correct. But
in the working directory of the calling script, it is incorrect.

Example: when calling `git rev-parse --git-path HEAD` in, say, the
Documentation/ subdirectory of Git's own source code, the string
`.git/HEAD` is printed.

Side note: that bug is hidden when running in a subdirectory of a
worktree that was added by the `git worktree` command: in that case, the
(correct) absolute path of the `HEAD` file is printed.

In the interest of time, this patch does not go the "correct" route to
introduce a struct with repository information (and removing global
state in the process), instead this patch chooses to detect when the
command was called in a subdirectory and forces the result to be an
absolute path.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Published-As: https://github.com/dscho/git/releases/tag/git-path-in-subdir-v1
Fetch-It-Via: git fetch https://github.com/dscho/git git-path-in-subdir-v1

 builtin/rev-parse.c   | 6 +++++-
 t/t0060-path-utils.sh | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index ff13e59e1d..f9d5762bf2 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -597,9 +597,13 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 		}
 
 		if (!strcmp(arg, "--git-path")) {
+			const char *path;
 			if (!argv[i + 1])
 				die("--git-path requires an argument");
-			puts(git_path("%s", argv[i + 1]));
+			path = git_path("%s", argv[i + 1]);
+			if (prefix && !is_absolute_path(path))
+				path = real_path(path);
+			puts(path);
 			i++;
 			continue;
 		}
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 444b5a4df8..790584fcc5 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -271,6 +271,8 @@ relative_path "<null>"		"<empty>"	./
 relative_path "<null>"		"<null>"	./
 relative_path "<null>"		/foo/a/b	./
 
+test_git_path "mkdir sub && cd sub && test_when_finished cd .. &&" \
+	foo "$(pwd)/.git/foo"
 test_git_path A=B                info/grafts .git/info/grafts
 test_git_path GIT_GRAFT_FILE=foo info/grafts foo
 test_git_path GIT_GRAFT_FILE=foo info/////grafts foo

base-commit: 6e3a7b3398559305c7a239a42e447c21a8f39ff8
-- 
2.11.1.windows.1

Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> The problem lies deeper, much deeper...
> ... but it bought us many, many problems.

I think you are making a mountain out of molehill here.  
This looks like as an opposite problem of a bug that forgets to
prepend the prefix to relative pathnames given by the end user.

I do agree that some calling scripts may find it more convenient if
the output were relative to their current directory, and in that
sense, this is worth addressing.

However.

How long has "rev-parse --git-path" been around?  Had scripts in the
wild chance to learn to live with the "output is relative to the top
of the working tree" reality?  I think the answers are "since 2.5" and
"yes".

I do not think we can make this unconditionally without breaking
users.  We instead need something like a new "--git-path-relative"
option, similar in the spirit that output from "git diff" can be
made relative to the current directory with the "--relative" option.

Assuming that we are discussing the new behaviour that is
conditionally triggered, let's see the implementation.

> -			puts(git_path("%s", argv[i + 1]));
> +			path = git_path("%s", argv[i + 1]);
> +			if (prefix && !is_absolute_path(path))
> +				path = real_path(path);
> +			puts(path);

Duy, want to help me out here?  I am wondering if using a logic
similar to what is used by "cd t && git grep -e foo :/" to emit
paths as "../Documentation/CodingGuidelines" as relative to the
current working directory is more appropriate than forcing the
absolute path output here (and if so, it may be preferrable to use
the relative_path() helper to do so), or the paths to files in
$GIT_DIR are conceptually different enough from paths to files in
the working tree and it will be more robust to have the output as an
absolute path.

I am leaning toward the latter (i.e. the above use of real_path() is
simple and good), but I haven't thought things through and since we
have an area expert here in the thread...

Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory

[Duy Nguyen]

On Wed, Feb 8, 2017 at 7:17 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> In addition to making git_path() aware of certain file names that need
> to be handled differently e.g. when running in worktrees, the commit
> 557bd833bb (git_path(): be aware of file relocation in $GIT_DIR,
> 2014-11-30) also snuck in a new option for `git rev-parse`:
> `--git-path`.
>
> On the face of it, there is no obvious bug in that commit's diff: it
> faithfully calls git_path() on the argument and prints it out, i.e. `git
> rev-parse --git-path <filename>` has the same precise behavior as
> calling `git_path("<filename>")` in C.
>
> The problem lies deeper, much deeper. In hindsight (which is always
> unfair), implementing the .git/ directory discovery in
> `setup_git_directory()` by changing the working directory may have
> allowed us to avoid passing around a struct that contains information
> about the current repository, but it bought us many, many problems.

Relevant thread in the past [1] which fixes both --git-path and
--git-common-dir. I think the author dropped it somehow (or forgot
about it, I know I did). Sorry can't comment on that thread, or this
patch, yet.

[1] http://public-inbox.org/git/1464261556-89722-1-git-send-email-rappazzo@gmail.com/
-- 
Duy

Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory

[Mike Rappazzo]

On Thu, Feb 9, 2017 at 4:48 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Wed, Feb 8, 2017 at 7:17 PM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
>> In addition to making git_path() aware of certain file names that need
>> to be handled differently e.g. when running in worktrees, the commit
>> 557bd833bb (git_path(): be aware of file relocation in $GIT_DIR,
>> 2014-11-30) also snuck in a new option for `git rev-parse`:
>> `--git-path`.
>>
>> On the face of it, there is no obvious bug in that commit's diff: it
>> faithfully calls git_path() on the argument and prints it out, i.e. `git
>> rev-parse --git-path <filename>` has the same precise behavior as
>> calling `git_path("<filename>")` in C.
>>
>> The problem lies deeper, much deeper. In hindsight (which is always
>> unfair), implementing the .git/ directory discovery in
>> `setup_git_directory()` by changing the working directory may have
>> allowed us to avoid passing around a struct that contains information
>> about the current repository, but it bought us many, many problems.
>
> Relevant thread in the past [1] which fixes both --git-path and
> --git-common-dir. I think the author dropped it somehow (or forgot
> about it, I know I did). Sorry can't comment on that thread, or this
> patch, yet.

I didn't exactly forget it (I have it sitting in a branch), I wasn't
sure what else was needed (from a v5 I guess), so it has stagnated.

There was also another patch [1] at the time done by SZEDER Gábor
trying to speed up the completion scripts by adding `git rev-parse
--absolute-git-dir` option to deal with this case as well.

>
> [1] http://public-inbox.org/git/1464261556-89722-1-git-send-email-rappazzo@gmail.com/
> --
> Duy

[1] http://public-inbox.org/git/20170203024829.8071-16-szeder.dev@gmail.com/

Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory

[Johannes Schindelin]

Hi Junio,

On Wed, 8 Feb 2017, Junio C Hamano wrote:

> How long has "rev-parse --git-path" been around?  Had scripts in the
> wild chance to learn to live with the "output is relative to the top of
> the working tree" reality?  I think the answers are "since 2.5" and
> "yes".

Correct. And the third question is: How did the scripts work around this
feature?

The answer is obvious: by switching back to `$(git rev-parse
--git-dir)/filename`.

This is literally on what I spent the better part of Wednesday.

There is just no way you can "fix" this otherwise. As an occasional shell
scripter, you may be tempted to use `$(git rev-parse --show-cdup)$(git
rev-parse --git-path filename)`, but of course that breaks in worktrees
and if you do not use worktrees you would not even know that your
workaround introduced another bug.

The current handling of --git-path is just plain wrong. The fact that I am
the first person to report this here merely shows how much it is used in
the wild.

Ciao,
Johannes

Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1603 bytes --]

Hi Mike,

On Thu, 9 Feb 2017, Mike Rappazzo wrote:

> On Thu, Feb 9, 2017 at 4:48 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>
> > Relevant thread in the past [1] which fixes both --git-path and
> > --git-common-dir. I think the author dropped it somehow (or forgot
> > about it, I know I did). Sorry can't comment on that thread, or this
> > patch, yet.
> 
> I didn't exactly forget it (I have it sitting in a branch), I wasn't
> sure what else was needed (from a v5 I guess), so it has stagnated.
> 
> There was also another patch [1] at the time done by SZEDER Gábor trying
> to speed up the completion scripts by adding `git rev-parse
> --absolute-git-dir` option to deal with this case as well.
> 
> > [1] http://public-inbox.org/git/1464261556-89722-1-git-send-email-rappazzo@gmail.com/
> 
> [1] http://public-inbox.org/git/20170203024829.8071-16-szeder.dev@gmail.com/

Ah, so I was not the only person reporting this bug, but I am seemingly
having as much luck getting a fix in.

I had a quick look at your v4:
http://public-inbox.org/git/1464261556-89722-3-git-send-email-rappazzo@gmail.com/

It seems you replaced the git_path() by a combination of git_dir() and
relative_path(), but that would break the use case where git_path()
handles certain arguments specially, e.g. "objects" which knows that the
.git/objects/ path can be overridden via the environment.

I tried very hard to keep that working in my patch, essentially by
emulating what git_path() does already when being called in a worktree's
subdirectory: make the path absolute.

Ciao,
Johannes

Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> Relevant thread in the past [1] which fixes both --git-path and
> --git-common-dir. I think the author dropped it somehow (or forgot
> about it, I know I did). Sorry can't comment on that thread, or this
> patch, yet.
>
> [1] http://public-inbox.org/git/1464261556-89722-1-git-send-email-rappazzo@gmail.com/

Thanks for a pointer.  I see Mike responded to this message (I
haven't had a chance to read and think about it yet), so I trust
that you three can figure out if these are the same issues and what
the final solution in the longer term should be.  

I have no strong opinion for or against a "longer term" solution
that makes "rev-parse --git-path" behave differently from how it
behaves today, but I am not yet convinced that we can reach that
longer term goal without a transition period, as I suspect there are
existing users that know and came to expect how it behaves, based on
its today's behaviour.  Other than that I do not have suggestion on
this topic at the moment.

Thanks.

Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Wed, 8 Feb 2017, Junio C Hamano wrote:
>
>> How long has "rev-parse --git-path" been around?  Had scripts in the
>> wild chance to learn to live with the "output is relative to the top of
>> the working tree" reality?  I think the answers are "since 2.5" and
>> "yes".
>
> Correct. And the third question is: How did the scripts work around this
> feature?
>
> The answer is obvious: by switching back to `$(git rev-parse
> --git-dir)/filename`.
>
> This is literally on what I spent the better part of Wednesday.
>
> There is just no way you can "fix" this otherwise. As an occasional shell
> scripter, you may be tempted to use `$(git rev-parse --show-cdup)$(git
> rev-parse --git-path filename)`, but of course that breaks in worktrees
> and if you do not use worktrees you would not even know that your
> workaround introduced another bug.

In case it is not clear, I understand all of the above.  

I was just worried about the people who do *NOT* use worktrees and
did the obvious "concatenate --cdup with --git-path" and thought
their script were working happily and well.  By prepending the path
to the (real) location of the .git in the updated --git-path output
ourselves, they will complain, our update broke their script.

If we introduced the fix gently, by (1) warn when "--git-path" is
used but give the current output anyway, while adding the "fixed"
one as another new option, and then (2) remove "--git-path" after
several releases, they will not have to complain, even though they
will see warning until they stop using "--git-path".

There may be gentler alternative ways to transition, and I do not
worry about the specifics of them too much.  I just think we cannot
do this in a single step without harming existing users.

Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory

[Johannes Schindelin]

Hi Junio,

On Thu, 9 Feb 2017, Junio C Hamano wrote:

> Duy Nguyen <pclouds@gmail.com> writes:
> 
> > Relevant thread in the past [1] which fixes both --git-path and
> > --git-common-dir. I think the author dropped it somehow (or forgot
> > about it, I know I did). Sorry can't comment on that thread, or this
> > patch, yet.
> >
> > [1] http://public-inbox.org/git/1464261556-89722-1-git-send-email-rappazzo@gmail.com/
> 
> Thanks for a pointer.  I see Mike responded to this message (I
> haven't had a chance to read and think about it yet), so I trust
> that you three can figure out if these are the same issues and what
> the final solution in the longer term should be.  
> 
> I have no strong opinion for or against a "longer term" solution
> that makes "rev-parse --git-path" behave differently from how it
> behaves today, but I am not yet convinced that we can reach that
> longer term goal without a transition period, as I suspect there are
> existing users that know and came to expect how it behaves, based on
> its today's behaviour.  Other than that I do not have suggestion on
> this topic at the moment.

Given that

- the output is incorrect, not some output that could maybe be improved,

- warnings in a script execution are most likely to be missed,

- --git-path gives incorrect output in subdirectories, except inside
  worktrees, therefore scripts relying on the current behavior are highly
  likely to misbehave in worktrees anyway,

- leaving this bug unfixed even when we know about it for 3 major releases
  reflects really badly on Git as a project, and

- the longer we wait to fix this bug, the more developers will simply stay
  away from --git-path (of course, only *after* they were bitten by the
  bug, like I was),

it should be safe to assume that a transitional period is more likely to
do more harm to our users than bring benefit.

Ciao,
Johannes

Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> I have no strong opinion for or against a "longer term" solution
>> that makes "rev-parse --git-path" behave differently from how it
>> behaves today, but I am not yet convinced that we can reach that
>> longer term goal without a transition period, as I suspect there are
>> existing users that know and came to expect how it behaves, based on
>> its today's behaviour.  Other than that I do not have suggestion on
>> this topic at the moment.

I think I was simply being silly (not merely "overcautious", but
just "silly") here.

There is no reason for people to use "--git-path" if they are not
preparing to work with secondary worktrees, because the whole point
of the feature is so that cases where "$(rev-parse --git-dir)/path"
does a wrong thing (e.g. end up referring to the main worktree thing
when you need to refer to your own, or vice versa).

> Given that
> ...
> it should be safe to assume that a transitional period is more likely to
> do more harm to our users than bring benefit.

In short, "--git-path as currently exposed to the end-users is
utterly broken and cannot have been used for anything sensible".  If
that is the case, let's just change that with an entry in the
release notes that states so (iow, there is no need for even a
backward compatibility notice, we just have an entry that says "this
was totally broken in such and such way, and now it is fixed to
behave this way").

That leaves what the right single-step behaviour change should be.
As I recall Duy said something about --common-dir and other things
Mike's earlier change also covered, I'd prefer to leave it to three
of you to figure out what the final patch should be.

Thanks.

Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory

[Mike Rappazzo]

On Thu, Feb 9, 2017 at 5:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> That leaves what the right single-step behaviour change should be.
> As I recall Duy said something about --common-dir and other things
> Mike's earlier change also covered, I'd prefer to leave it to three
> of you to figure out what the final patch should be.
>

The tests which I covered in my previous patch [1] addressed the
places where we identified similar problems.  We should try to include
some form of those tests.  As far as implementation goes in rev-parse,
the version in this thread is probably better that what I had, but it
would need to also be applied to --git-common-dir and
--shared-index-path.


[1] http://public-inbox.org/git/1464261556-89722-2-git-send-email-rappazzo@gmail.com/

Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory

[Jeff King]

On Thu, Feb 09, 2017 at 01:50:31PM -0800, Junio C Hamano wrote:

> > There is just no way you can "fix" this otherwise. As an occasional shell
> > scripter, you may be tempted to use `$(git rev-parse --show-cdup)$(git
> > rev-parse --git-path filename)`, but of course that breaks in worktrees
> > and if you do not use worktrees you would not even know that your
> > workaround introduced another bug.
> 
> In case it is not clear, I understand all of the above.  
> 
> I was just worried about the people who do *NOT* use worktrees and
> did the obvious "concatenate --cdup with --git-path" and thought
> their script were working happily and well.  By prepending the path
> to the (real) location of the .git in the updated --git-path output
> ourselves, they will complain, our update broke their script.

That concatenating approach is broken in other ways, too. For example,
if you have $GIT_DIR set to an absolute path, then --git-path will use
that.  I don't think we have ever promised that the output of --git-path
(or --git-dir) would ever be absolute or relative (in fact, the
--git-dir documentation implies that you may get either).

So yes, there could be somebody who is doing this concatenation
workaround, but only ever calls their script in a certain way that never
triggers the absolute-path variant. They're happy now, and may not be
after Dscho's patch. But I really think they are relying on a bogus
assumption, and their scripts are already buggy.

-Peff

[PATCH v2 0/2] Fix bugs in rev-parse's output when run in a subdirectory

[Johannes Schindelin]

The bug that bit me (hard!) and that triggered not only a long series of
curses but also my writing a patch and sending it to the list was that
`git rev-parse --git-path HEAD` would give *incorrect* output when run
in a subdirectory of a regular checkout, but *correct* output when run
in a subdirectory of an associated *worktree*.

I had tested the script in question quite a bit, but in a worktree. And
in production, it quietly did exactly the wrong thing.

Relative to v1 of the then-single patch, this changed:

- the patch now covers also --git-common-dir and --shared-index-path

- the output is now a relative path when git_dir is relative

- I plucked the tests from Mike's patch series and dropped my ad-hoc
  test from t0060.

- While at it, I fixed Mike's test that assumed that the objects/
  directory lives in .git/worktrees/<name>/objects/.


Johannes Schindelin (1):
  rev-parse: fix several options when running in a subdirectory

Michael Rappazzo (1):
  rev-parse tests: add tests executed from a subdirectory

 builtin/rev-parse.c      | 15 +++++++++++----
 t/t1500-rev-parse.sh     | 28 ++++++++++++++++++++++++++++
 t/t1700-split-index.sh   | 17 +++++++++++++++++
 t/t2027-worktree-list.sh | 10 +++++++++-
 4 files changed, 65 insertions(+), 5 deletions(-)


base-commit: 6e3a7b3398559305c7a239a42e447c21a8f39ff8
Published-As: https://github.com/dscho/git/releases/tag/git-path-in-subdir-v2
Fetch-It-Via: git fetch https://github.com/dscho/git git-path-in-subdir-v2

Interdiff vs v1:

 diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
 index f9d5762bf2b..84af2802f6f 100644
 --- a/builtin/rev-parse.c
 +++ b/builtin/rev-parse.c
 @@ -545,6 +545,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
  	unsigned int flags = 0;
  	const char *name = NULL;
  	struct object_context unused;
 +	struct strbuf buf = STRBUF_INIT;
  
  	if (argc > 1 && !strcmp("--parseopt", argv[1]))
  		return cmd_parseopt(argc - 1, argv + 1, prefix);
 @@ -597,13 +598,11 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
  		}
  
  		if (!strcmp(arg, "--git-path")) {
 -			const char *path;
  			if (!argv[i + 1])
  				die("--git-path requires an argument");
 -			path = git_path("%s", argv[i + 1]);
 -			if (prefix && !is_absolute_path(path))
 -				path = real_path(path);
 -			puts(path);
 +			strbuf_reset(&buf);
 +			puts(relative_path(git_path("%s", argv[i + 1]),
 +					   prefix, &buf));
  			i++;
  			continue;
  		}
 @@ -825,8 +824,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()));
 +				strbuf_reset(&buf);
 +				puts(relative_path(get_git_common_dir(),
 +						   prefix, &buf));
  				continue;
  			}
  			if (!strcmp(arg, "--is-inside-git-dir")) {
 @@ -849,7 +849,9 @@ 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)));
 +					const char *path = git_path("sharedindex.%s", sha1_to_hex(sha1));
 +					strbuf_reset(&buf);
 +					puts(relative_path(path, prefix, &buf));
  				}
  				continue;
  			}
 @@ -910,5 +912,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
  		die_no_single_rev(quiet);
  	} else
  		show_default();
 +	strbuf_release(&buf);
  	return 0;
  }
 diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
 index 790584fcc54..444b5a4df80 100755
 --- a/t/t0060-path-utils.sh
 +++ b/t/t0060-path-utils.sh
 @@ -271,8 +271,6 @@ relative_path "<null>"		"<empty>"	./
  relative_path "<null>"		"<null>"	./
  relative_path "<null>"		/foo/a/b	./
  
 -test_git_path "mkdir sub && cd sub && test_when_finished cd .. &&" \
 -	foo "$(pwd)/.git/foo"
  test_git_path A=B                info/grafts .git/info/grafts
  test_git_path GIT_GRAFT_FILE=foo info/grafts foo
  test_git_path GIT_GRAFT_FILE=foo info/////grafts foo
 diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
 index 038e24c4014..d74f09ad93e 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_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
 diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
 index 292a0720fcc..446ff34f966 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
 diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
 index 465eeeacd3d..848da5f3684 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/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.11.1.windows.1

[PATCH v2 1/2] rev-parse tests: add tests executed from a subdirectory

[Johannes Schindelin]

From: Michael Rappazzo <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.

[jes: fixed incorrect assumption that objects/ lives in the
worktree-specific git-dir (it lives in the common dir instead).]

Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 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 038e24c4014..f39f783f2db 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 292a0720fcc..1d6e27a09d8 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 465eeeacd3d..c1a072348e7 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/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.11.1.windows.1

[PATCH v2 2/2] rev-parse: fix several options when running in a subdirectory

[Johannes Schindelin]

In addition to making git_path() aware of certain file names that need
to be handled differently e.g. when running in worktrees, the commit
557bd833bb (git_path(): be aware of file relocation in $GIT_DIR,
2014-11-30) also snuck in a new option for `git rev-parse`:
`--git-path`.

On the face of it, there is no obvious bug in that commit's diff: it
faithfully calls git_path() on the argument and prints it out, i.e. `git
rev-parse --git-path <filename>` has the same precise behavior as
calling `git_path("<filename>")` in C.

The problem lies deeper, much deeper. In hindsight (which is always
unfair), implementing the .git/ directory discovery in
`setup_git_directory()` by changing the working directory may have
allowed us to avoid passing around a struct that contains information
about the current repository, but it bought us many, many problems.

In this case, when being called in a subdirectory, `git rev-parse`
changes the working directory to the top-level directory before calling
`git_path()`. In the new working directory, the result is correct. But
in the working directory of the calling script, it is incorrect.

Example: when calling `git rev-parse --git-path HEAD` in, say, the
Documentation/ subdirectory of Git's own source code, the string
`.git/HEAD` is printed.

Side note: that bug is hidden when running in a subdirectory of a
worktree that was added by the `git worktree` command: in that case, the
(correct) absolute path of the `HEAD` file is printed.

In the interest of time, this patch does not go the "correct" route to
introduce a struct with repository information (and removing global
state in the process), instead this patch chooses to detect when the
command was called in a subdirectory and forces the result to be an
absolute path.

While at it, we are also fixing the output of --git-common-dir and
--shared-index-path.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/rev-parse.c      | 15 +++++++++++----
 t/t1500-rev-parse.sh     |  4 ++--
 t/t1700-split-index.sh   |  2 +-
 t/t2027-worktree-list.sh |  4 ++--
 4 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index ff13e59e1db..84af2802f6f 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -545,6 +545,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 	unsigned int flags = 0;
 	const char *name = NULL;
 	struct object_context unused;
+	struct strbuf buf = STRBUF_INIT;
 
 	if (argc > 1 && !strcmp("--parseopt", argv[1]))
 		return cmd_parseopt(argc - 1, argv + 1, prefix);
@@ -599,7 +600,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 		if (!strcmp(arg, "--git-path")) {
 			if (!argv[i + 1])
 				die("--git-path requires an argument");
-			puts(git_path("%s", argv[i + 1]));
+			strbuf_reset(&buf);
+			puts(relative_path(git_path("%s", argv[i + 1]),
+					   prefix, &buf));
 			i++;
 			continue;
 		}
@@ -821,8 +824,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()));
+				strbuf_reset(&buf);
+				puts(relative_path(get_git_common_dir(),
+						   prefix, &buf));
 				continue;
 			}
 			if (!strcmp(arg, "--is-inside-git-dir")) {
@@ -845,7 +849,9 @@ 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)));
+					const char *path = git_path("sharedindex.%s", sha1_to_hex(sha1));
+					strbuf_reset(&buf);
+					puts(relative_path(path, prefix, &buf));
 				}
 				continue;
 			}
@@ -906,5 +912,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 		die_no_single_rev(quiet);
 	} else
 		show_default();
+	strbuf_release(&buf);
 	return 0;
 }
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index f39f783f2db..d74f09ad93e 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 1d6e27a09d8..446ff34f966 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 c1a072348e7..848da5f3684 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/objects" >expect &&
 	test_when_finished "rm -rf linked-tree && git worktree prune" &&
 	git worktree add --detach linked-tree master &&
-- 
2.11.1.windows.1

Re: [PATCH] rev-parse --git-path: fix output when running in a subdirectory

[Johannes Schindelin]

Hi Mike,

On Thu, 9 Feb 2017, Mike Rappazzo wrote:

> On Thu, Feb 9, 2017 at 5:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >
> > That leaves what the right single-step behaviour change should be.  As
> > I recall Duy said something about --common-dir and other things Mike's
> > earlier change also covered, I'd prefer to leave it to three of you to
> > figure out what the final patch should be.
> >
> 
> The tests which I covered in my previous patch [1] addressed the places
> where we identified similar problems.  We should try to include some
> form of those tests.  As far as implementation goes in rev-parse, the
> version in this thread is probably better that what I had, but it would
> need to also be applied to --git-common-dir and --shared-index-path.

Thank you so much for pointing out that git-common-dir and
shared-index-path have the same problem.

I looked a little further, and it seems that the show_file() function may
have the exact same problem... but then, it only prefixes filenames if the
--prefix=<prefix> option has been passed, and it could be argued that
those prefixed filenames are *not* meant to be relative to the cwd but to
the top-level directory.

Anways, v2 was just sent out, and with Peff's acknowledgement that this
fixes a real bug and that hypothetical scripts relying on the buggy
behavior were broken beyond repair even without worktrees anyway, I am
hopeful that we'll get somewhere.

Ciao,
Johannes

Re: [PATCH v2 1/2] rev-parse tests: add tests executed from a subdirectory

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> From: Michael Rappazzo <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.
>
> [jes: fixed incorrect assumption that objects/ lives in the
> worktree-specific git-dir (it lives in the common dir instead).]
>
> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

Just one iffy part; otherwise looks good.

> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> index 038e24c4014..f39f783f2db 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
> +'

All of these look sensible.

>  test_done
> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> index 292a0720fcc..1d6e27a09d8 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
> +	)

This looks iffy.  If we expect multiple sharedindex* files, the
first output from "ls -t" may or may not match the real one in use
(multiple things do happen within a single second or whatever your
filesystem's time granularity is).  Two "ls -t" run in this test
would (hopefully) give stable results, but I suspect that the chance
the first line in the output matches what --shared-index-path reports
is 50% if we expect to have two sharedindex* files.

On the other hand, if we do not expect multiple sharedindex* files,
use of "ls" piped to "tail" is simply misleading.

If this test can be written in such a way that there is only one
such file that match the pattern, it would be the cleanest to
understand and explain.  As there is only a single invocation of
"update-index --split-index" immediately after a new repository is
created, I suspect that the expectation to see only one sharedindex*
file already holds (because its name is unpredictable, we still need
to catch it with wildcard), and if that is the case, we can just
lose "-t" and pipe to tail, i.e. "ls .git/sharedindex* >expect".

> diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
> index 465eeeacd3d..c1a072348e7 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
>  '

OK, this swaps a wrong expectation with a more usable one.

> +test_expect_failure 'rev-parse --git-path objects linked worktree' '
> +	echo "$(git rev-parse --show-toplevel)/.git/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
> +'
> +

Looking good.

Thanks.

Re: [PATCH v2 2/2] rev-parse: fix several options when running in a subdirectory

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index ff13e59e1db..84af2802f6f 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -545,6 +545,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>  	unsigned int flags = 0;
>  	const char *name = NULL;
>  	struct object_context unused;
> +	struct strbuf buf = STRBUF_INIT;
>  
>  	if (argc > 1 && !strcmp("--parseopt", argv[1]))
>  		return cmd_parseopt(argc - 1, argv + 1, prefix);
> @@ -599,7 +600,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>  		if (!strcmp(arg, "--git-path")) {
>  			if (!argv[i + 1])
>  				die("--git-path requires an argument");
> -			puts(git_path("%s", argv[i + 1]));
> +			strbuf_reset(&buf);
> +			puts(relative_path(git_path("%s", argv[i + 1]),
> +					   prefix, &buf));
>  			i++;
>  			continue;
>  		}
> @@ -821,8 +824,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()));
> +				strbuf_reset(&buf);
> +				puts(relative_path(get_git_common_dir(),
> +						   prefix, &buf));
>  				continue;
>  			}
>  			if (!strcmp(arg, "--is-inside-git-dir")) {
> @@ -845,7 +849,9 @@ 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)));
> +					const char *path = git_path("sharedindex.%s", sha1_to_hex(sha1));
> +					strbuf_reset(&buf);
> +					puts(relative_path(path, prefix, &buf));
>  				}
>  				continue;
>  			}
> @@ -906,5 +912,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
>  		die_no_single_rev(quiet);
>  	} else
>  		show_default();
> +	strbuf_release(&buf);

This uses "reset then use" pattern for repeated use of strbuf, and
causes the string last held in the strbuf to leak on early return,
which can be mitigated by using "use then reset" pattern.  I.e.

			if (!strcmp(arg, "--git-common-dir")) {
				puts(relative_path(get_git_common_dir(),
						   prefix, &buf));
				strbuf_reset(&buf);
				continue;
			}

I'd think.  You'd still want to release it at the end anyway for
good code hygiene, though.

Other than that, looks good to me.

Thanks.

Re: [PATCH v2 0/2] Fix bugs in rev-parse's output when run in a subdirectory

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> Johannes Schindelin (1):
>   rev-parse: fix several options when running in a subdirectory
>
> Michael Rappazzo (1):
>   rev-parse tests: add tests executed from a subdirectory
>
>  builtin/rev-parse.c      | 15 +++++++++++----
>  t/t1500-rev-parse.sh     | 28 ++++++++++++++++++++++++++++
>  t/t1700-split-index.sh   | 17 +++++++++++++++++
>  t/t2027-worktree-list.sh | 10 +++++++++-
>  4 files changed, 65 insertions(+), 5 deletions(-)
>
>
> base-commit: 6e3a7b3398559305c7a239a42e447c21a8f39ff8
> Published-As: https://github.com/dscho/git/releases/tag/git-path-in-subdir-v2
> Fetch-It-Via: git fetch https://github.com/dscho/git git-path-in-subdir-v2

Will queue as js/git-path-in-subdir topic forked at 2.12-rc0.

I still think the log message in 2/2 is making a mountain out of
molehill and showing a skewed perception on pros-and-cons in a
design decision, but I won't repeat my review.  I saw a few
correctness issues and pointed them out on the patches.

Re: [PATCH v2 1/2] rev-parse tests: add tests executed from a subdirectory

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> index 292a0720fcc..1d6e27a09d8 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 . &&

Another thing that I notice only after merging this and other topics
to 'pu' was that this piece needs to always come at the end of the
script because of this removal.  It would make the test more robust
to create a test repository for this test and work inside it.

> +	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

Re: [PATCH v2 2/2] rev-parse: fix several options when running in a subdirectory

[Johannes Schindelin]

Hi Junio,

On Fri, 10 Feb 2017, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> > index ff13e59e1db..84af2802f6f 100644
> > --- a/builtin/rev-parse.c
> > +++ b/builtin/rev-parse.c
> > @@ -545,6 +545,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> >  	unsigned int flags = 0;
> >  	const char *name = NULL;
> >  	struct object_context unused;
> > +	struct strbuf buf = STRBUF_INIT;
> >  
> >  	if (argc > 1 && !strcmp("--parseopt", argv[1]))
> >  		return cmd_parseopt(argc - 1, argv + 1, prefix);
> > @@ -599,7 +600,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> >  		if (!strcmp(arg, "--git-path")) {
> >  			if (!argv[i + 1])
> >  				die("--git-path requires an argument");
> > -			puts(git_path("%s", argv[i + 1]));
> > +			strbuf_reset(&buf);
> > +			puts(relative_path(git_path("%s", argv[i + 1]),
> > +					   prefix, &buf));
> >  			i++;
> >  			continue;
> >  		}
> > @@ -821,8 +824,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()));
> > +				strbuf_reset(&buf);
> > +				puts(relative_path(get_git_common_dir(),
> > +						   prefix, &buf));
> >  				continue;
> >  			}
> >  			if (!strcmp(arg, "--is-inside-git-dir")) {
> > @@ -845,7 +849,9 @@ 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)));
> > +					const char *path = git_path("sharedindex.%s", sha1_to_hex(sha1));
> > +					strbuf_reset(&buf);
> > +					puts(relative_path(path, prefix, &buf));
> >  				}
> >  				continue;
> >  			}
> > @@ -906,5 +912,6 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
> >  		die_no_single_rev(quiet);
> >  	} else
> >  		show_default();
> > +	strbuf_release(&buf);
> 
> This uses "reset then use" pattern for repeated use of strbuf, and
> causes the string last held in the strbuf to leak on early return,

... which cannot happen due to the lack of an early return...

> which can be mitigated by using "use then reset" pattern.  I.e.
> 
> 			if (!strcmp(arg, "--git-common-dir")) {
> 				puts(relative_path(get_git_common_dir(),
> 						   prefix, &buf));
> 				strbuf_reset(&buf);
> 				continue;
> 			}
> 
> I'd think.

This would not release the memory, though:

	#define strbuf_reset(sb)  strbuf_setlen(sb, 0)

and

	static inline void strbuf_setlen(struct strbuf *sb, size_t len)
	{
		if (len > (sb->alloc ? sb->alloc - 1 : 0))
			die("BUG: strbuf_setlen() beyond buffer");
		sb->len = len;
		sb->buf[len] = '\0';
	}

There is not a single free() statement there.

So the "use then reset" scheme would leak *just the same*.

> You'd still want to release it at the end anyway for good code hygiene,
> though.

Which I do.

Technically, this is not even necessary because all of the cmd_*()
functions are immediately followed by a call to exit(). Wasn't that the
genius idea in the early Git days, that we could simply get away with
sloppy memory management because the program exit()s shortly afterwards,
anyway? ;-)

In any case, I adjusted the commit message to clarify why the "reset then
use" scheme is correct here.

Ciao,
Johannes

Re: [PATCH v2 1/2] rev-parse tests: add tests executed from a subdirectory

[Johannes Schindelin]

Hi Junio,

On Fri, 10 Feb 2017, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> > index 292a0720fcc..1d6e27a09d8 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
> > +	)
> 
> This looks iffy.

Indeed.

> If we expect multiple sharedindex* files, the first output from "ls -t"
> may or may not match the real one in use (multiple things do happen
> within a single second or whatever your filesystem's time granularity
> is).  Two "ls -t" run in this test would (hopefully) give stable
> results, but I suspect that the chance the first line in the output
> matches what --shared-index-path reports is 50% if we expect to have two
> sharedindex* files.
> 
> On the other hand, if we do not expect multiple sharedindex* files,
> use of "ls" piped to "tail" is simply misleading.
> 
> If this test can be written in such a way that there is only one
> such file that match the pattern, it would be the cleanest to
> understand and explain.  As there is only a single invocation of
> "update-index --split-index" immediately after a new repository is
> created, I suspect that the expectation to see only one sharedindex*
> file already holds (because its name is unpredictable, we still need
> to catch it with wildcard), and if that is the case, we can just
> lose "-t" and pipe to tail, i.e. "ls .git/sharedindex* >expect".

Indeed. We can expect only one sharedindex file to be present, and we do
not even have to call out to `ls` but can get away with calling `echo`
(which is a builtin in Bash).

Ciao,
Johannes

Re: [PATCH v2 1/2] rev-parse tests: add tests executed from a subdirectory

[Johannes Schindelin]

Hi Junio,

On Fri, 10 Feb 2017, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> > index 292a0720fcc..1d6e27a09d8 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 . &&
> 
> Another thing that I notice only after merging this and other topics
> to 'pu' was that this piece needs to always come at the end of the
> script because of this removal.  It would make the test more robust
> to create a test repository for this test and work inside it.

Yes, this is indeed unnecessary and even undesirable.

I waited a couple of days to give the original author a chance to fix
this. As I want a fix really badly (because I am really embarrassed of
this bug), I adjusted t1700 appropriately and will send out v3 in a
second.

Ciao,
Johannes

[PATCH v3 0/2] Fix bugs in rev-parse's output when run in a subdirectory

[Johannes Schindelin]

The bug that bit me (hard!) and that triggered not only a long series of
curses but also my writing a patch and sending it to the list was that
`git rev-parse --git-path HEAD` would give *incorrect* output when run
in a subdirectory of a regular checkout, but *correct* output when run
in a subdirectory of an associated *worktree*.

I had tested the script in question quite a bit, but in a worktree. And
in production, it quietly did exactly the wrong thing.

Changes relative to v2:

- the "iffy" test in t1700 was made "uniffy"

- clarified in the commit message of 2/2 why we can get away with the
  "reset then use" pattern


Johannes Schindelin (1):
  rev-parse: fix several options when running in a subdirectory

Michael Rappazzo (1):
  rev-parse tests: add tests executed from a subdirectory

 builtin/rev-parse.c      | 15 +++++++++++----
 t/t1500-rev-parse.sh     | 28 ++++++++++++++++++++++++++++
 t/t1700-split-index.sh   | 16 ++++++++++++++++
 t/t2027-worktree-list.sh | 10 +++++++++-
 4 files changed, 64 insertions(+), 5 deletions(-)


base-commit: 076c05393a047247ea723896289b48d6549ed7d0
Published-As: https://github.com/dscho/git/releases/tag/git-path-in-subdir-v3
Fetch-It-Via: git fetch https://github.com/dscho/git git-path-in-subdir-v3

Interdiff vs v2:

 diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
 index 84af2802f6f..2cfd8d2aae4 100644
 --- a/builtin/rev-parse.c
 +++ b/builtin/rev-parse.c
 @@ -903,6 +903,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
  			continue;
  		verify_filename(prefix, arg, 1);
  	}
 +	strbuf_release(&buf);
  	if (verify) {
  		if (revs_count == 1) {
  			show_rev(type, sha1, name);
 @@ -912,6 +913,5 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
  		die_no_single_rev(quiet);
  	} else
  		show_default();
 -	strbuf_release(&buf);
  	return 0;
  }
 diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
 index 446ff34f966..6096f2c6309 100755
 --- a/t/t1700-split-index.sh
 +++ b/t/t1700-split-index.sh
 @@ -201,17 +201,16 @@ EOF
  '
  
  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" &&
 +	test_create_repo split-index &&
  	(
 -		cd work &&
 -		ls -t ../.git/sharedindex* | tail -n 1 >expect &&
 +		cd split-index &&
 +		git update-index --split-index &&
 +		echo .git/sharedindex* >expect &&
 +		git rev-parse --shared-index-path >actual &&
 +		test_cmp expect actual &&
 +		mkdir subdirectory &&
 +		cd subdirectory &&
 +		echo ../.git/sharedindex* >expect &&
  		git rev-parse --shared-index-path >actual &&
  		test_cmp expect actual
  	)

-- 
2.11.1.windows.1.2.g87ad093.dirty

[PATCH v3 1/2] rev-parse tests: add tests executed from a subdirectory

[Johannes Schindelin]

From: Michael Rappazzo <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.

[jes: fixed incorrect assumption that objects/ lives in the
worktree-specific git-dir (it lives in the common dir instead). Also
adjusted t1700 so that the test case does not *need* to be the last
one in that script.]

Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t1500-rev-parse.sh     | 28 ++++++++++++++++++++++++++++
 t/t1700-split-index.sh   | 16 ++++++++++++++++
 t/t2027-worktree-list.sh | 12 ++++++++++--
 3 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 038e24c4014..f39f783f2db 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 292a0720fcc..b754865a618 100755
--- a/t/t1700-split-index.sh
+++ b/t/t1700-split-index.sh
@@ -200,4 +200,20 @@ EOF
 	test_cmp expect actual
 '
 
+test_expect_failure 'rev-parse --shared-index-path' '
+	test_create_repo split-index &&
+	(
+		cd split-index &&
+		git update-index --split-index &&
+		echo .git/sharedindex* >expect &&
+		git rev-parse --shared-index-path >actual &&
+		test_cmp expect actual &&
+		mkdir subdirectory &&
+		cd subdirectory &&
+		echo ../.git/sharedindex* >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 465eeeacd3d..c1a072348e7 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/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.11.1.windows.1.2.g87ad093.dirty

[PATCH v3 2/2] rev-parse: fix several options when running in a subdirectory

[Johannes Schindelin]

In addition to making git_path() aware of certain file names that need
to be handled differently e.g. when running in worktrees, the commit
557bd833bb (git_path(): be aware of file relocation in $GIT_DIR,
2014-11-30) also snuck in a new option for `git rev-parse`:
`--git-path`.

On the face of it, there is no obvious bug in that commit's diff: it
faithfully calls git_path() on the argument and prints it out, i.e. `git
rev-parse --git-path <filename>` has the same precise behavior as
calling `git_path("<filename>")` in C.

The problem lies deeper, much deeper. In hindsight (which is always
unfair), implementing the .git/ directory discovery in
`setup_git_directory()` by changing the working directory may have
allowed us to avoid passing around a struct that contains information
about the current repository, but it bought us many, many problems.

In this case, when being called in a subdirectory, `git rev-parse`
changes the working directory to the top-level directory before calling
`git_path()`. In the new working directory, the result is correct. But
in the working directory of the calling script, it is incorrect.

Example: when calling `git rev-parse --git-path HEAD` in, say, the
Documentation/ subdirectory of Git's own source code, the string
`.git/HEAD` is printed.

Side note: that bug is hidden when running in a subdirectory of a
worktree that was added by the `git worktree` command: in that case, the
(correct) absolute path of the `HEAD` file is printed.

In the interest of time, this patch does not go the "correct" route to
introduce a struct with repository information (and removing global
state in the process), instead this patch chooses to detect when the
command was called in a subdirectory and forces the result to be an
absolute path.

While at it, we are also fixing the output of --git-common-dir and
--shared-index-path.

Lastly, please note that we reuse the same strbuf for all of the
relative_path() calls; this avoids frequent allocation (and duplicated
code), and it does not risk memory leaks, for two reasons: 1) the
cmd_rev_parse() function does not return anywhere between the use of
the new strbuf instance and its final release, and 2) git-rev-parse is
one of these "one-shot" programs in Git, i.e. it exits after running
for a very short time, meaning that all allocated memory is released
with the exit() call anyway.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/rev-parse.c      | 15 +++++++++++----
 t/t1500-rev-parse.sh     |  4 ++--
 t/t1700-split-index.sh   |  2 +-
 t/t2027-worktree-list.sh |  4 ++--
 4 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index ff13e59e1db..2cfd8d2aae4 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -545,6 +545,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 	unsigned int flags = 0;
 	const char *name = NULL;
 	struct object_context unused;
+	struct strbuf buf = STRBUF_INIT;
 
 	if (argc > 1 && !strcmp("--parseopt", argv[1]))
 		return cmd_parseopt(argc - 1, argv + 1, prefix);
@@ -599,7 +600,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 		if (!strcmp(arg, "--git-path")) {
 			if (!argv[i + 1])
 				die("--git-path requires an argument");
-			puts(git_path("%s", argv[i + 1]));
+			strbuf_reset(&buf);
+			puts(relative_path(git_path("%s", argv[i + 1]),
+					   prefix, &buf));
 			i++;
 			continue;
 		}
@@ -821,8 +824,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()));
+				strbuf_reset(&buf);
+				puts(relative_path(get_git_common_dir(),
+						   prefix, &buf));
 				continue;
 			}
 			if (!strcmp(arg, "--is-inside-git-dir")) {
@@ -845,7 +849,9 @@ 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)));
+					const char *path = git_path("sharedindex.%s", sha1_to_hex(sha1));
+					strbuf_reset(&buf);
+					puts(relative_path(path, prefix, &buf));
 				}
 				continue;
 			}
@@ -897,6 +903,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 			continue;
 		verify_filename(prefix, arg, 1);
 	}
+	strbuf_release(&buf);
 	if (verify) {
 		if (revs_count == 1) {
 			show_rev(type, sha1, name);
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index f39f783f2db..d74f09ad93e 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 b754865a618..6096f2c6309 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' '
 	test_create_repo split-index &&
 	(
 		cd split-index &&
diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index c1a072348e7..848da5f3684 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/objects" >expect &&
 	test_when_finished "rm -rf linked-tree && git worktree prune" &&
 	git worktree add --detach linked-tree master &&
-- 
2.11.1.windows.1.2.g87ad093.dirty

Re: [PATCH v3 0/2] Fix bugs in rev-parse's output when run in a subdirectory

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> The bug that bit me (hard!) and that triggered not only a long series of
> curses but also my writing a patch and sending it to the list was that
> `git rev-parse --git-path HEAD` would give *incorrect* output when run
> in a subdirectory of a regular checkout, but *correct* output when run
> in a subdirectory of an associated *worktree*.
>
> I had tested the script in question quite a bit, but in a worktree. And
> in production, it quietly did exactly the wrong thing.
>
> Changes relative to v2:
>
> - the "iffy" test in t1700 was made "uniffy"
>
> - clarified in the commit message of 2/2 why we can get away with the
>   "reset then use" pattern

It is no longer relevant between "reset then use" and "use then
reset", I think, because you did something much better, which is to
move strbuf_release() up so that it comes before the possible early
returns.

Both patches look good.  Let's queue this and move it to 'next'
shortly.  Personally, I think it is OK to fast-track this to
'master' before the final, but just like any other bugs, we've lived
with the bug for some time, and it is not a big deal if we have to
live with it for a bit longer.

Thanks.