threads / patch / 45086

patchrev-parse --git-path: fix output when running in a subdirectory

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

## tl;dr

27 messages between Feb 8, 2017 and Feb 17, 2017. Diffs are folded; open one to read it.

replies: 26people: 5as markdown or json

Johannes Schindelin· Feb 8, 2017, 12:17 UTC · lore

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(-)
Show changes to 2 files +7 −1

builtin/rev-parse.c, t/t0060-path-utils.sh

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
Junio C Hamano· Feb 8, 2017, 18:47 UTC · re: Johannes Schindelin · lore

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

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.

Show 5 quoted lines
> -			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...

Johannes Schindelin· Feb 9, 2017, 21:05 UTC · re: Junio C Hamano · lore

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

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

Junio C Hamano· Feb 9, 2017, 21:50 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 20 quoted lines
> 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.

Jeff King· Feb 10, 2017, 04:21 UTC · re: Junio C Hamano · lore

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

On Thu, Feb 09, 2017 at 01:50:31PM -0800, Junio C Hamano wrote:
Show 13 quoted lines
> > 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
Duy Nguyen· Feb 9, 2017, 09:48 UTC · re: Johannes Schindelin · lore

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

On Wed, Feb 8, 2017 at 7:17 PM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

Show 16 quoted lines
> 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
Mike Rappazzo· Feb 9, 2017, 13:46 UTC · re: Duy Nguyen · lore

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

On Thu, Feb 9, 2017 at 4:48 AM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 23 quoted lines
> 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/
Johannes Schindelin· Feb 9, 2017, 21:11 UTC · re: Mike Rappazzo · lore

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

Hi Mike,
On Thu, 9 Feb 2017, Mike Rappazzo wrote:
Show 17 quoted lines
> 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

Junio C Hamano· Feb 9, 2017, 21:33 UTC · re: Duy Nguyen · lore

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

Duy Nguyen <pclouds@gmail.com> writes:
Show 6 quoted lines
> 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.
Johannes Schindelin· Feb 9, 2017, 22:11 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On Thu, 9 Feb 2017, Junio C Hamano wrote:
Show 21 quoted lines
> 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

Junio C Hamano· Feb 9, 2017, 22:54 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 7 quoted lines
>> 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.
Mike Rappazzo· Feb 10, 2017, 03:52 UTC · re: Junio C Hamano · lore

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

On Thu, Feb 9, 2017 at 5:54 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 6 quoted lines
>
> 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/
Johannes Schindelin· Feb 10, 2017, 15:44 UTC · re: Mike Rappazzo · lore

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

Hi Mike,
On Thu, 9 Feb 2017, Mike Rappazzo wrote:
Show 13 quoted lines
> 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

Johannes Schindelin· Feb 10, 2017, 15:33 UTC · re: Johannes Schindelin · lore

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

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
Johannes Schindelin· Feb 10, 2017, 15:33 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to 4 files +16 −9

builtin/rev-parse.c, t/t1500-rev-parse.sh, t/t1700-split-index.sh, t/t2027-worktree-list.sh

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
Junio C Hamano· Feb 10, 2017, 18:57 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 51 quoted lines
> 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.
Johannes Schindelin· Feb 17, 2017, 16:53 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On Fri, 10 Feb 2017, Junio C Hamano wrote:
Show 56 quoted lines
> 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...
Show 10 quoted lines
> 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

Johannes Schindelin· Feb 10, 2017, 15:33 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to 3 files +55 −2

t/t1500-rev-parse.sh, t/t1700-split-index.sh, t/t2027-worktree-list.sh

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
Junio C Hamano· Feb 10, 2017, 18:50 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 15 quoted lines
> 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.
Show 35 quoted lines
> 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.
Show 24 quoted lines
>  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".

Show 19 quoted lines
> 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.
Show 8 quoted lines
> +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.
Johannes Schindelin· Feb 17, 2017, 16:55 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On Fri, 10 Feb 2017, Junio C Hamano wrote:
Show 27 quoted lines
> 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.
Show 19 quoted lines
> 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

Junio C Hamano· Feb 10, 2017, 20:25 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 11 quoted lines
> 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.

Show 15 quoted lines
> +	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
Johannes Schindelin· Feb 17, 2017, 16:57 UTC · re: Junio C Hamano · lore

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

Hi Junio,
On Fri, 10 Feb 2017, Junio C Hamano wrote:
Show 18 quoted lines
> 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

Junio C Hamano· Feb 10, 2017, 18:59 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 16 quoted lines
> 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.

Johannes Schindelin· Feb 17, 2017, 16:58 UTC · re: Johannes Schindelin · lore

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

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
Johannes Schindelin· Feb 17, 2017, 16:59 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to 3 files +54 −2

t/t1500-rev-parse.sh, t/t1700-split-index.sh, t/t2027-worktree-list.sh

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
Johannes Schindelin· Feb 17, 2017, 16:59 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to 4 files +16 −9

builtin/rev-parse.c, t/t1500-rev-parse.sh, t/t1700-split-index.sh, t/t2027-worktree-list.sh

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
Junio C Hamano· Feb 17, 2017, 18:25 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 15 quoted lines
> 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.

← back to recent threads