[RFC PATCH 0/2] fixing git pull from symlinked directory

[RFC PATCH 0/2] fixing git pull from symlinked directory

[Marcel M. Cary]

In a past message I described a problem when pulling in a
directory that I arrived at by "cd"-ing to a symlink.

http://article.gmane.org/gmane.comp.version-control.git/98223

I've created a unit test to illustrate the problem and
compare it to "git pull" without the symlink and "git push"
with the symlink, which both work.  That's the first patch
in this series.

I think the root cause here is that a shell's "cd" and C's
chdir() behave slightly different with symlinks.  See the unit
test comments for more details.

I can make the unit test pass if I make bash's "cd" behave
like chdir() by adding "-P", as in the second patch of this
series, but I think that has portability issues.  Any tips
for addressing that?  Maybe instead call realpath() on the
result of "git rev-parse --show-cdup" in C before printing
it?  That's a substantial change as it would print
/full/path/to/work-dir/ instead of just "../".

Marcel M. Cary (2):
  Add failing test for "git pull" in symlinked directory
  Support shell scripts that run from symlinks into a git working dir

 git-sh-setup.sh         |    2 +-
 t/t5521-pull-symlink.sh |   67 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+), 1 deletions(-)
 create mode 100644 t/t5521-pull-symlink.sh

[RFC PATCH 1/2] Add failing test for "git pull" in symlinked directory

[Marcel M. Cary]

* Illustrate the scenario of interest and show how it breaks
* Show a contrasting working "git pull" without the symlink
* Show a contrasting working "git push" with the symlink

Signed-off-by: Marcel M. Cary <marcel@oak.homeunix.org>
---
 t/t5521-pull-symlink.sh |   67 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 67 insertions(+), 0 deletions(-)

diff --git a/t/t5521-pull-symlink.sh b/t/t5521-pull-symlink.sh
new file mode 100644
index 0000000..683784d
--- /dev/null
+++ b/t/t5521-pull-symlink.sh
@@ -0,0 +1,67 @@
+#!/bin/sh
+
+test_description='pulling from symlinked subdir'
+
+. ./test-lib.sh
+
+D=`pwd`
+
+# The scenario we are building:
+#
+#   trash\ directory/
+#     clone-repo/
+#       subdir/
+#         bar
+#     subdir-link -> clone-repo/subdir/
+#
+# The working directory is subdir-link.
+#
+test_expect_success setup '
+
+    mkdir subdir &&
+    touch subdir/bar &&
+    git add subdir/bar &&
+    git commit -m empty &&
+    git clone . clone-repo &&
+    # demonstrate that things work without the symlink
+    test_debug "cd clone-repo/subdir/ && git pull; cd ../.." &&
+    ln -s clone-repo/subdir/ subdir-link &&
+    cd subdir-link/ &&
+    test_debug "set +x"
+'
+
+# From subdir-link, pulling should work as it does from
+# clone-repo/subdir/.
+#
+# Instead, the error pull gave was:
+#
+#   fatal: 'origin': unable to chdir or not a git archive
+#   fatal: The remote end hung up unexpectedly
+#
+# bacause git would find the .git/config for the trash\ directory
+# repo, not for the clone-repo repo.  The trash\ directory repo
+# had no entry for origin.  Git found the wrong .git because
+# git rev-parse --show-cdup printed a path relative to
+# clone-repo/subdir/, not subdir-link/.  Git rev-parse --show-cdup
+# used the correct .git, but when the git pull shell script did
+# "cd `git rev-parse --show-cdup`", it ended up in the wrong
+# directory.  Shell "cd" works a little different from chdir() in C.
+# Bash's "cd -P" works like chdir() in C.
+#
+test_expect_failure 'pulling from symlinked subdir' '
+
+    git pull
+'
+
+# Prove that the remote end really is a repo, and other commands
+# work fine in this context.
+#
+test_debug "
+    test_expect_success 'pushing from symlinked subdir' '
+
+        git push
+    '
+"
+cd "$D"
+
+test_done
-- 
1.6.0.3

[RFC PATCH 2/2] Support shell scripts that run from symlinks into a git working dir

[Marcel M. Cary]

Use "cd -P" instead of just "cd" when switching to the top level
of the git working directory.  When working from a symlink
into GIT_WORK_TREE, the shell function cd_to_toplevel will now
change to GIT_WORK_TREE rather than the parent of the symlink,
which may not even be the root of a git working directory.

Unfortunately this solution looks non-portable.

Signed-off-by: Marcel M. Cary <marcel@oak.homeunix.org>
---
 git-sh-setup.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index dbdf209..4006150 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -85,7 +85,7 @@ cd_to_toplevel () {
 	cdup=$(git rev-parse --show-cdup)
 	if test ! -z "$cdup"
 	then
-		cd "$cdup" || {
+		cd -P "$cdup" || {
 			echo >&2 "Cannot chdir to $cdup, the toplevel of the working tree"
 			exit 1
 		}
-- 
1.6.0.3

[PATCH] rev-parse: Fix shell scripts whose cwd is a symlink into a git work-dir

[Marcel M. Cary]

* Change "git rev-parse --show-cdup" to print a full path instead of
  a series of "../" when it prints anything
* Added a special case to support some existing scripts that rely
  on --show-cdup's prior behavior of printing a blank line when in
  the work-tree root
* Add some tests for "git rev-parse --show-cdup" in existing scenarios
  and add a symlinked scenario that failed before this fix
* Add a test for "git pull" in a symlinked directory that failed
  before this fix, plus constrasting already working scenarios

Signed-off-by: Marcel M. Cary <marcel@oak.homeunix.org>
---

I suggested use of realpath() but then realized that git seems to
chdir to the work-dir before processing the "--show-cdup" option,
so getcwd() is a simpler option.  

By changing rev-parse instead of cd_to_toplevel, the fix will
help shell scripts that call rev-parse directly as well.  Hopefully
the change from "../" to /full/path/to/work-dir will not be too
disruptive -- no tests fail at least.

 builtin-rev-parse.c     |   25 ++++++++++++-----
 t/t1501-worktree.sh     |   53 ++++++++++++++++++++++++++-----------
 t/t5521-pull-symlink.sh |   67 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 122 insertions(+), 23 deletions(-)
 create mode 100755 t/t5521-pull-symlink.sh

diff --git a/builtin-rev-parse.c b/builtin-rev-parse.c
index 81d5a6f..9cf5f82 100644
--- a/builtin-rev-parse.c
+++ b/builtin-rev-parse.c
@@ -536,14 +536,25 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 						printf("%s\n", work_tree);
 					continue;
 				}
-				while (pfx) {
-					pfx = strchr(pfx, '/');
-					if (pfx) {
-						pfx++;
-						printf("../");
-					}
+				/*
+				 * An empty line tells some scripts they are at
+				 * the work dir's root.  For example,
+				 * rebase --interactive.
+				 */
+				if (!prefix) {
+					putchar('\n');
+					continue;
 				}
-				putchar('\n');
+				/*
+				 * A full path is less ambiguous than ../ when
+				 * the shell arrived at it's cwd via a symlink.
+				 * Otherwise the shell's "cd" may choose the
+				 * symbolic parent.
+				 */
+				static char cwd[PATH_MAX];
+				if (!getcwd(cwd, PATH_MAX))
+					die("unable to get current working directory");
+				printf("%s\n", cwd);
 				continue;
 			}
 			if (!strcmp(arg, "--git-dir")) {
diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
index f6a6f83..7e83c81 100755
--- a/t/t1501-worktree.sh
+++ b/t/t1501-worktree.sh
@@ -22,15 +22,35 @@ test_rev_parse() {
 	shift
 	[ $# -eq 0 ] && return
 
+	test_expect_success "$name: cdup" \
+	"test '$1' = \"\$(git rev-parse --show-cdup)\""
+	shift
+	[ $# -eq 0 ] && return
+
 	test_expect_success "$name: prefix" \
 	"test '$1' = \"\$(git rev-parse --show-prefix)\""
 	shift
 	[ $# -eq 0 ] && return
 }
 
+D="$(pwd)"
+
 EMPTY_TREE=$(git write-tree)
-mkdir -p work/sub/dir || exit 1
-mv .git repo.git || exit 1
+mkdir -p repo/sub/dir || exit 1
+ln -s repo/sub/dir lnk || exit 1
+mv .git repo/ || exit 1
+work="$(pwd)/repo"
+cd lnk
+
+say "worktree is parent of symlink"
+test_rev_parse 'symlink'      false false true "$work" sub/dir/
+cd "$D"
+
+
+mv repo/.git repo.git || exit 1
+mv repo work || exit 1
+rm lnk || exit 1
+work="$(pwd)/work"
 
 say "core.worktree = relative path"
 GIT_DIR=repo.git
@@ -38,26 +58,26 @@ GIT_CONFIG="$(pwd)"/$GIT_DIR/config
 export GIT_DIR GIT_CONFIG
 unset GIT_WORK_TREE
 git config core.worktree ../work
-test_rev_parse 'outside'      false false false
+test_rev_parse 'outside'      false false false "$work"
 cd work || exit 1
 GIT_DIR=../repo.git
 GIT_CONFIG="$(pwd)"/$GIT_DIR/config
-test_rev_parse 'inside'       false false true ''
+test_rev_parse 'inside'       false false true '' ''
 cd sub/dir || exit 1
 GIT_DIR=../../../repo.git
 GIT_CONFIG="$(pwd)"/$GIT_DIR/config
-test_rev_parse 'subdirectory' false false true sub/dir/
+test_rev_parse 'subdirectory' false false true "$work" sub/dir/
 cd ../../.. || exit 1
 
 say "core.worktree = absolute path"
 GIT_DIR=$(pwd)/repo.git
 GIT_CONFIG=$GIT_DIR/config
 git config core.worktree "$(pwd)/work"
-test_rev_parse 'outside'      false false false
+test_rev_parse 'outside'      false false false "$work"
 cd work || exit 1
-test_rev_parse 'inside'       false false true ''
+test_rev_parse 'inside'       false false true '' ''
 cd sub/dir || exit 1
-test_rev_parse 'subdirectory' false false true sub/dir/
+test_rev_parse 'subdirectory' false false true "$work" sub/dir/
 cd ../../.. || exit 1
 
 say "GIT_WORK_TREE=relative path (override core.worktree)"
@@ -66,30 +86,31 @@ GIT_CONFIG=$GIT_DIR/config
 git config core.worktree non-existent
 GIT_WORK_TREE=work
 export GIT_WORK_TREE
-test_rev_parse 'outside'      false false false
+test_rev_parse 'outside'      false false false "$work"
 cd work || exit 1
 GIT_WORK_TREE=.
-test_rev_parse 'inside'       false false true ''
+test_rev_parse 'inside'       false false true '' ''
 cd sub/dir || exit 1
 GIT_WORK_TREE=../..
-test_rev_parse 'subdirectory' false false true sub/dir/
+test_rev_parse 'subdirectory' false false true "$work" sub/dir/
 cd ../../.. || exit 1
 
 mv work repo.git/work
+work="$(pwd)/repo.git/work"
 
 say "GIT_WORK_TREE=absolute path, work tree below git dir"
 GIT_DIR=$(pwd)/repo.git
 GIT_CONFIG=$GIT_DIR/config
 GIT_WORK_TREE=$(pwd)/repo.git/work
-test_rev_parse 'outside'              false false false
+test_rev_parse 'outside'              false false false "$work"
 cd repo.git || exit 1
-test_rev_parse 'in repo.git'              false true  false
+test_rev_parse 'in repo.git'              false true  false "$work"
 cd objects || exit 1
-test_rev_parse 'in repo.git/objects'      false true  false
+test_rev_parse 'in repo.git/objects'      false true  false "$work"
 cd ../work || exit 1
-test_rev_parse 'in repo.git/work'         false true true ''
+test_rev_parse 'in repo.git/work'         false true true '' ''
 cd sub/dir || exit 1
-test_rev_parse 'in repo.git/sub/dir' false true true sub/dir/
+test_rev_parse 'in repo.git/sub/dir' false true true "$work" sub/dir/
 cd ../../../.. || exit 1
 
 test_expect_success 'repo finds its work tree' '
diff --git a/t/t5521-pull-symlink.sh b/t/t5521-pull-symlink.sh
new file mode 100755
index 0000000..f18fec7
--- /dev/null
+++ b/t/t5521-pull-symlink.sh
@@ -0,0 +1,67 @@
+#!/bin/sh
+
+test_description='pulling from symlinked subdir'
+
+. ./test-lib.sh
+
+D=`pwd`
+
+# The scenario we are building:
+#
+#   trash\ directory/
+#     clone-repo/
+#       subdir/
+#         bar
+#     subdir-link -> clone-repo/subdir/
+#
+# The working directory is subdir-link.
+#
+test_expect_success setup '
+
+    mkdir subdir &&
+    touch subdir/bar &&
+    git add subdir/bar &&
+    git commit -m empty &&
+    git clone . clone-repo &&
+    # demonstrate that things work without the symlink
+    test_debug "cd clone-repo/subdir/ && git pull; cd ../.." &&
+    ln -s clone-repo/subdir/ subdir-link &&
+    cd subdir-link/ &&
+    test_debug "set +x"
+'
+
+# From subdir-link, pulling should work as it does from
+# clone-repo/subdir/.
+#
+# Instead, the error pull gave was:
+#
+#   fatal: 'origin': unable to chdir or not a git archive
+#   fatal: The remote end hung up unexpectedly
+#
+# because git would find the .git/config for the "trash directory"
+# repo, not for the clone-repo repo.  The "trash directory" repo
+# had no entry for origin.  Git found the wrong .git because
+# git rev-parse --show-cdup printed a path relative to
+# clone-repo/subdir/, not subdir-link/.  Git rev-parse --show-cdup
+# used the correct .git, but when the git pull shell script did
+# "cd `git rev-parse --show-cdup`", it ended up in the wrong
+# directory.  Shell "cd" works a little different from chdir() in C.
+# Bash's "cd -P" works like chdir() in C.
+#
+test_expect_success 'pulling from symlinked subdir' '
+
+    git pull
+'
+
+# Prove that the remote end really is a repo, and other commands
+# work fine in this context.
+#
+test_debug "
+    test_expect_success 'pushing from symlinked subdir' '
+
+        git push
+    '
+"
+cd "$D"
+
+test_done
-- 
1.6.0.3

Re: [PATCH] rev-parse: Fix shell scripts whose cwd is a symlink into a git work-dir

[Jakub Narebski]

"Marcel M. Cary" <marcel@oak.homeunix.org> writes:

> * Change "git rev-parse --show-cdup" to print a full path instead of
>   a series of "../" when it prints anything

But that is contrary to the _name_ of option. It is --show-cdup, as
in "show cd up". And I think your change will break a few scripts.

I think you should use "git rev-parse --work-tree" for full path
to working directory:

    --show-cdup
        When the command is invoked from a subdirectory, show the path
        of the top-level directory relative to the current directory
        (typically a sequence of "../", or an empty string).

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH] rev-parse: Fix shell scripts whose cwd is a symlink into a git work-dir

[Andreas Ericsson]

Jakub Narebski wrote:
> "Marcel M. Cary" <marcel@oak.homeunix.org> writes:
> 
>> * Change "git rev-parse --show-cdup" to print a full path instead of
>>   a series of "../" when it prints anything
> 
> But that is contrary to the _name_ of option. It is --show-cdup, as
> in "show cd up". And I think your change will break a few scripts.
> 
> I think you should use "git rev-parse --work-tree" for full path
> to working directory:
> 
>     --show-cdup
>         When the command is invoked from a subdirectory, show the path
>         of the top-level directory relative to the current directory
>         (typically a sequence of "../", or an empty string).
> 

AFAIR, it was introduced to make test-builds of really large projects in
really deep directories with a ton of symlinks leading to the path work a
lot faster.

The thing to remember about git and its UI is that it was evolved from
users' actual needs. Very, very little of what is in the UI can be reworked
without breaking something for someone, so it's (almost) always better to
add a new option. For this, I'd suggest "--show-absolute-worktree-path" if
that's what it does. Since it's an option primarily targeting scripts, I'm
not too worried that it's half a mile long.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] rev-parse: Fix shell scripts whose cwd is a symlink into a git work-dir

[Marcel M. Cary]

> AFAIR, it was introduced to make test-builds of really large projects in
> really deep directories with a ton of symlinks leading to the path work a
> lot faster.

Andreas,

I see value in keeping Git very fast.  That is, after all, why I chose
Git over Mercurial.  Do you know where that discussion was, if was in
the archives?  I found these reasons to avoid absolute paths in the git
archives:

* paths with more components are slower to work with (in the context of
  add and diff, which deal with many many paths)
* absolute paths may exceed PATH_MAX while relative ones didn't
* getcwd() will fail if parent directories are not executable, or on
  some platforms, if parent directories are not readable

My impression is that the performance issue is probably not significant
for cd_to_toplevel since it's not in a tight inner loop, and dito for
other potential callers of --show-cdup.  The PATH_MAX seems to be a
restriction elsewhere in the code already.

Even if there were a scenario that put --show-cdup in a tight loop, I
wonder whether current implementation provides much performance benefit,
at least when bash is the calling language: bash seems to make the
relative path absolute anyway inside the "cd" builtin.

The commit (5f94c730) that introduces that code doesn't mention
performance.  It compares to:

  git rev-parse --show-prefix | sed -e 's|[^/][^/]*|..|g'


I also noticed that this failure case with "--show-cdup" in a symlinked
directory has come up more than once before.
http://marc.info/?l=git&m=122452534912000&w=2
http://marc.info/?l=git&m=121613416212958&w=2
https://kerneltrap.org/mailarchive/git/2007/4/25/244653/thread

Marcel

Re: [PATCH] rev-parse: Fix shell scripts whose cwd is a symlink into a git work-dir

[Andreas Ericsson]

Marcel M. Cary wrote:
>> AFAIR, it was introduced to make test-builds of really large projects in
>> really deep directories with a ton of symlinks leading to the path work a
>> lot faster.
> 
> Andreas,
> 
> I see value in keeping Git very fast.  That is, after all, why I chose
> Git over Mercurial.  Do you know where that discussion was, if was in
> the archives?  I found these reasons to avoid absolute paths in the git
> archives:
> 
> * paths with more components are slower to work with (in the context of
>   add and diff, which deal with many many paths)
> * absolute paths may exceed PATH_MAX while relative ones didn't
> * getcwd() will fail if parent directories are not executable, or on
>   some platforms, if parent directories are not readable
> 
> My impression is that the performance issue is probably not significant
> for cd_to_toplevel since it's not in a tight inner loop, and dito for
> other potential callers of --show-cdup.  The PATH_MAX seems to be a
> restriction elsewhere in the code already.
> 

The performance issue does not come from cd_to_toplevel itself, but from
its callers. That is, if scripts start to use absolute paths from *other*
tight loops, that's when we hit a problem.

> Even if there were a scenario that put --show-cdup in a tight loop, I
> wonder whether current implementation provides much performance benefit,
> at least when bash is the calling language: bash seems to make the
> relative path absolute anyway inside the "cd" builtin.
> 
> The commit (5f94c730) that introduces that code doesn't mention
> performance.  It compares to:
> 
>   git rev-parse --show-prefix | sed -e 's|[^/][^/]*|..|g'
> 
> 
> I also noticed that this failure case with "--show-cdup" in a symlinked
> directory has come up more than once before.
> http://marc.info/?l=git&m=122452534912000&w=2
> http://marc.info/?l=git&m=121613416212958&w=2
> https://kerneltrap.org/mailarchive/git/2007/4/25/244653/thread
> 

I can imagine. However, --show-cdup has a different use too. It's nifty
for printing relative paths from commands running inside a subdirectory
of the repository. If you need the absolute path to the root of the repo,
I'd suggest you add "--show-absolute-path" instead.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH] rev-parse: Fix shell scripts whose cwd is a symlink into a git work-dir

[Marcel M. Cary]

> But that is contrary to the _name_ of option. It is --show-cdup, as
> in "show cd up". And I think your change will break a few scripts.
> 
> I think you should use "git rev-parse --work-tree" for full path
> to working directory:
> 
>     --show-cdup
>         When the command is invoked from a subdirectory, show the path
>         of the top-level directory relative to the current directory
>         (typically a sequence of "../", or an empty string).

Jakub,

Yes, I agree, there is a risk in breaking scripts that rely on the "../"
format.  That's the "substantial change" I was alluding.  Here's how I
arrived at that choice.  I considered these fixes:

(a) add some shell code to cd_to_toplevel to find the canonical pwd and
    interpret --show-cdup output from there

(b) make a new option (--work-tree would be a good name) to print the
    canonical work tree path, and leave --show-cdup as it is.  Then
    change cd_to_toplevel and/or git pull to use the new option

(c) change --show-cdup to print the canonical work tree path, even
    though it's not entirely consistent with the name of the option

The main reason I avoided (a), even though that "cd" is what violated my
expecations, is because I didn't want to have to re-implement code to
check whether each path component is a symlink.  (Now I see that "cd
`/bin/pwd` might be a more concise fix.)

The reason I avoided (b) is because, to make all of git work for me, I
expected to have to change several calling scripts.  (Now that I look, I
see only three calls to --show-cdup in the git codebase to change.)
Even so, third-party scripts that I might want to use in the future
would not immediately be changed.

Option (c) keeps the change small and isolated and makes it effective
everywhere.  The documentation, while perhaps in need of update given my
patch, doesn't promise to always return zero or more "../".  Also,
there's a branch branch of the --show-cdup code (that I can't seem to
exercise) that the result of get_git_work_tree(), which might not be
zero or more "../".


Would you suggest pursuing option (a)?  I wonder whether there are
languages other than shell that might suffer from the same problem of
keeping an internal PWD variable of some sort, or perhaps there are
shell scripts out there that call --show-cdup directly instead of
calling cd_to_toplevel.

Do you think it would be less likely to break existing scripts if I
restrict the (c) behavior to when getenv("PWD") doesn't match the
starting getcwd()?  (I'm not sure yet whether that's a reliable way to
detect the symlink scenario, but it seems to work with bash.)

Marcel

Re: [PATCH] rev-parse: Fix shell scripts whose cwd is a symlink into a git work-dir

[Johannes Sixt]

Marcel M. Cary schrieb:
> * Change "git rev-parse --show-cdup" to print a full path instead of
>   a series of "../" when it prints anything

http://thread.gmane.org/gmane.comp.version-control.git/88557/focus=88562

I don't see that you bring in any new arguments.

-- Hannes

Re: [PATCH] rev-parse: Fix shell scripts whose cwd is a symlink into a git work-dir

[Marcel M. Cary]

Johannes Sixt wrote:
> Marcel M. Cary schrieb:
>> * Change "git rev-parse --show-cdup" to print a full path instead of
>>   a series of "../" when it prints anything
> 
> http://thread.gmane.org/gmane.comp.version-control.git/88557/focus=88562
> 
> I don't see that you bring in any new arguments.

To be clear, as mentioned here:

http://thread.gmane.org/gmane.comp.version-control.git/88557/focus=88573

I'm not talking about a situation where the symlink is in the working
tree pointing outwards.  I'm talking about a symlink outside pointing
in.  And as mentioned later in that thread, the --work-tree workaround
doesn't actually work.


One new thing I have to add is that the reason --show-cdup prints a
correct path but pull fails is because it's the *shell* who
misinterprets the path.  So telling git rev-parse where the work-tree is
helps nothing.  It already knows.

http://thread.gmane.org/gmane.comp.version-control.git/88557/focus=88581

So far I've seen no response to the idea, which Yves mentions, about
trying to restrict the absolute path behavior to times when bash would
interpret the "../" incorrectly.

Nor have I seen a response to the idea of correcting the shell's
behavior in cd_to_toplevel, for example by adding a "cd `pwd`", and I
don't really understand the scenario where this would be a performance
concern; I think I haven't found a particular discussion that several
people have referenced.  Perhaps I should prepare a patch for that so I
can verify that it works as I expect and so we have something more
concrete to discuss?

Any tips on how to follow the reference
7vk5sly3h9.fsf@assigned-by-dhcp.cox.net in the first url above?  It
looks to be about performance.  Message-Id seems to not be indexed for
searching.

Marcel

Re: [PATCH] rev-parse: Fix shell scripts whose cwd is a symlink into a git work-dir

[Johannes Sixt]

Marcel M. Cary schrieb:
> Any tips on how to follow the reference
> 7vk5sly3h9.fsf@assigned-by-dhcp.cox.net in the first url above?  It
> looks to be about performance.  Message-Id seems to not be indexed for
> searching.

http://mid.gmane.org/7vk5sly3h9.fsf@assigned-by-dhcp.cox.net

-- Hannes

Re: [PATCH] rev-parse: Fix shell scripts whose cwd is a symlink into a git work-dir

[Junio C Hamano]

"Marcel M. Cary" <marcel@oak.homeunix.org> writes:

> Nor have I seen a response to the idea of correcting the shell's
> behavior in cd_to_toplevel, for example by adding a "cd `pwd`",...

I think you probably could fool the shell by unsetting PWD or something
silly like that (or "cd -P" which may not be supported by non POSIX shells
but I suspect the ones we care about do support it).

Doing a 'cd "$(pwd)"' inside cd_to_toplevel would be a less objectionable
and arguably more portable workaround for the case where you have a
symlink pointing into somewhere in your work tree.

[PATCH] git-sh-setup: Fix scripts whose PWD is a symlink into a git work-dir

[Marcel M. Cary]

* Before interpretting an upward path (../) in cd_to_toplevel,
  cd to a path without symlinks given by /bin/pwd
* Add tests for cd_to_toplevel and "git pull" in a symlinked
  directory that failed before this fix, plus constrasting
  scenarios that already worked

Signed-off-by: Marcel M. Cary <marcel@oak.homeunix.org>
---

I hope this patch will address concerns both about changes
to existing APIs and speed of the new behavior.

A few notes on implementation choices:

I used /bin/pwd because of this precedent for choosing it over
"cd -P" for compatibility.
http://article.gmane.org/gmane.comp.version-control.git/46918

If cd_to_toplevel had concatenated $(/bin/pwd) with $cdup to
avoid the separate "cd", it would require checking for $cdup
being an absolute path.  I wasn't sure how to check that in
a way that is both portable and clearly faster than "cd",
so cd_to_toplevel runs "cd" twice.  I'm assuming that
running an external command like expr or grep is slower than
just doing the "cd".

cd_to_toplevel doesn't check $PWD to see whether to do the
first cd, because some shells allegedly don't update it
reliably.

Since cd_to_toplevel doesn't know whether it's at a
symlinked PWD or not, I wrote it to treat the 
"cd $(/bin/pwd)" as mandatory, even when it might not
actually be.  So on systems without /bin/pwd, it will fail
even when there are no symlinks.  I thought that was better
than inconsistent behavior depending on whether /bin/pwd is
available.

The extra "cd" will be skipped when the script is already at
the top of the working tree.


 git-sh-setup.sh           |   11 +++++++
 t/t2300-cd-to-toplevel.sh |   37 +++++++++++++++++++++++++
 t/t5521-pull-symlink.sh   |   67 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 115 insertions(+), 0 deletions(-)
 create mode 100755 t/t2300-cd-to-toplevel.sh
 create mode 100755 t/t5521-pull-symlink.sh

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index dbdf209..377700b 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -85,6 +85,17 @@ cd_to_toplevel () {
 	cdup=$(git rev-parse --show-cdup)
 	if test ! -z "$cdup"
 	then
+		# Interpret $cdup relative to the physical, not logical, cwd.
+		# Probably /bin/pwd is more portable than passing -P to cd or pwd.
+		phys="$(/bin/pwd)" || {
+			echo >&2 "Cannot determine the physical path to the current dir"
+			exit 1
+		}
+		cd "$phys" || {
+			echo >&2 "Cannot chdir to the physical path to current dir: $phys"
+			exit 1
+		}
+
 		cd "$cdup" || {
 			echo >&2 "Cannot chdir to $cdup, the toplevel of the working tree"
 			exit 1
diff --git a/t/t2300-cd-to-toplevel.sh b/t/t2300-cd-to-toplevel.sh
new file mode 100755
index 0000000..293dc35
--- /dev/null
+++ b/t/t2300-cd-to-toplevel.sh
@@ -0,0 +1,37 @@
+#!/bin/sh
+
+test_description='cd_to_toplevel'
+
+. ./test-lib.sh
+
+test_cd_to_toplevel () {
+	test_expect_success "$2" '
+		(
+			cd '"'$1'"' &&
+			. git-sh-setup &&
+			cd_to_toplevel &&
+			[ "$(pwd -P)" = "$TOPLEVEL" ]
+		)
+	'
+}
+
+TOPLEVEL="$(pwd -P)/repo"
+mkdir -p repo/sub/dir
+mv .git repo/
+SUBDIRECTORY_OK=1
+
+test_cd_to_toplevel repo 'at physical root'
+
+test_cd_to_toplevel repo/sub/dir 'at physical subdir'
+
+ln -s repo symrepo
+test_cd_to_toplevel symrepo 'at symbolic root'
+
+ln -s repo/sub/dir subdir-link
+test_cd_to_toplevel subdir-link 'at symbolic subdir'
+
+cd repo
+ln -s sub/dir internal-link
+test_cd_to_toplevel internal-link 'at internal symbolic subdir'
+
+test_done
diff --git a/t/t5521-pull-symlink.sh b/t/t5521-pull-symlink.sh
new file mode 100755
index 0000000..f18fec7
--- /dev/null
+++ b/t/t5521-pull-symlink.sh
@@ -0,0 +1,67 @@
+#!/bin/sh
+
+test_description='pulling from symlinked subdir'
+
+. ./test-lib.sh
+
+D=`pwd`
+
+# The scenario we are building:
+#
+#   trash\ directory/
+#     clone-repo/
+#       subdir/
+#         bar
+#     subdir-link -> clone-repo/subdir/
+#
+# The working directory is subdir-link.
+#
+test_expect_success setup '
+
+    mkdir subdir &&
+    touch subdir/bar &&
+    git add subdir/bar &&
+    git commit -m empty &&
+    git clone . clone-repo &&
+    # demonstrate that things work without the symlink
+    test_debug "cd clone-repo/subdir/ && git pull; cd ../.." &&
+    ln -s clone-repo/subdir/ subdir-link &&
+    cd subdir-link/ &&
+    test_debug "set +x"
+'
+
+# From subdir-link, pulling should work as it does from
+# clone-repo/subdir/.
+#
+# Instead, the error pull gave was:
+#
+#   fatal: 'origin': unable to chdir or not a git archive
+#   fatal: The remote end hung up unexpectedly
+#
+# because git would find the .git/config for the "trash directory"
+# repo, not for the clone-repo repo.  The "trash directory" repo
+# had no entry for origin.  Git found the wrong .git because
+# git rev-parse --show-cdup printed a path relative to
+# clone-repo/subdir/, not subdir-link/.  Git rev-parse --show-cdup
+# used the correct .git, but when the git pull shell script did
+# "cd `git rev-parse --show-cdup`", it ended up in the wrong
+# directory.  Shell "cd" works a little different from chdir() in C.
+# Bash's "cd -P" works like chdir() in C.
+#
+test_expect_success 'pulling from symlinked subdir' '
+
+    git pull
+'
+
+# Prove that the remote end really is a repo, and other commands
+# work fine in this context.
+#
+test_debug "
+    test_expect_success 'pushing from symlinked subdir' '
+
+        git push
+    '
+"
+cd "$D"
+
+test_done
-- 
1.6.0.3

Re: [PATCH] git-sh-setup: Fix scripts whose PWD is a symlink into a git work-dir

[Junio C Hamano]

"Marcel M. Cary" <marcel@oak.homeunix.org> writes:

> If cd_to_toplevel had concatenated $(/bin/pwd) with $cdup to
> avoid the separate "cd", it would require checking for $cdup
> being an absolute path.  I wasn't sure how to check that in
> a way that is both portable and clearly faster than "cd",

    case "$v" in
    /*) : handle absolute path ;;
    *) : everything else ;;
    esac

In all shells that support "case..esac", it is built-in.

Having said that, I think it would probably be better to bite the bullet
and start using "cd -P" soon after 1.6.1 goes final, and at the same time
existing places that use "cd `pwd`" as a workaround if there are some.

[PATCH v2] git-sh-setup: Fix scripts whose PWD is a symlink into a git work-dir

[Marcel M. Cary]

* When interpretting a relative upward (../) path in cd_to_toplevel,
  prepend the cwd without symlinks, given by /bin/pwd
* Add tests for cd_to_toplevel and "git pull" in a symlinked
  directory that failed before this fix, plus constrasting
  scenarios that already worked

Signed-off-by: Marcel M. Cary <marcel@oak.homeunix.org>
---

Now /bin/pwd is only used when $cdup both is relative
and contains a ".." component, which I think is most of
the time that git didn't start out in the top-level
directory.

I can't seem to exercise the case that show-cdup prints
something other than "../" or "", even when setting
GIT_WORK_TREE in or out of the work tree.  Is it premature
to anticipate that show-cdup might print an arbitrary path
sometime in the future, given the 
"if (!is_inside_work_tree())" branch of show-cdup?  Or maybe
it does now and I just don't see the use case.

Also, the additional invocation of "cd" is now removed.


 git-sh-setup.sh           |   23 ++++++++++++++-
 t/t2300-cd-to-toplevel.sh |   37 +++++++++++++++++++++++++
 t/t5521-pull-symlink.sh   |   67 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 125 insertions(+), 2 deletions(-)
 create mode 100755 t/t2300-cd-to-toplevel.sh
 create mode 100755 t/t5521-pull-symlink.sh

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index dbdf209..f07d96b 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -85,8 +85,27 @@ cd_to_toplevel () {
 	cdup=$(git rev-parse --show-cdup)
 	if test ! -z "$cdup"
 	then
-		cd "$cdup" || {
-			echo >&2 "Cannot chdir to $cdup, the toplevel of the working tree"
+		case "$cdup" in
+		/*)
+			# Not quite the same as if we did "cd -P '$cdup'" when
+			# $cdup contains ".." after symlink path components.
+			# Don't fix that case at least until Git switches to
+			# "cd -P" across the board.
+			phys="$cdup"
+			;;
+		..|../*|*/..|*/../*)
+			# Interpret $cdup relative to the physical, not logical, cwd.
+			# Probably /bin/pwd is more portable than passing -P to cd or pwd.
+			phys="$(/bin/pwd)/$cdup"
+			;;
+		*)
+			# There's no "..", so no need to make things absolute.
+			phys="$cdup"
+			;;
+		esac
+
+		cd "$phys" || {
+			echo >&2 "Cannot chdir to $phys, the toplevel of the working tree"
 			exit 1
 		}
 	fi
diff --git a/t/t2300-cd-to-toplevel.sh b/t/t2300-cd-to-toplevel.sh
new file mode 100755
index 0000000..293dc35
--- /dev/null
+++ b/t/t2300-cd-to-toplevel.sh
@@ -0,0 +1,37 @@
+#!/bin/sh
+
+test_description='cd_to_toplevel'
+
+. ./test-lib.sh
+
+test_cd_to_toplevel () {
+	test_expect_success "$2" '
+		(
+			cd '"'$1'"' &&
+			. git-sh-setup &&
+			cd_to_toplevel &&
+			[ "$(pwd -P)" = "$TOPLEVEL" ]
+		)
+	'
+}
+
+TOPLEVEL="$(pwd -P)/repo"
+mkdir -p repo/sub/dir
+mv .git repo/
+SUBDIRECTORY_OK=1
+
+test_cd_to_toplevel repo 'at physical root'
+
+test_cd_to_toplevel repo/sub/dir 'at physical subdir'
+
+ln -s repo symrepo
+test_cd_to_toplevel symrepo 'at symbolic root'
+
+ln -s repo/sub/dir subdir-link
+test_cd_to_toplevel subdir-link 'at symbolic subdir'
+
+cd repo
+ln -s sub/dir internal-link
+test_cd_to_toplevel internal-link 'at internal symbolic subdir'
+
+test_done
diff --git a/t/t5521-pull-symlink.sh b/t/t5521-pull-symlink.sh
new file mode 100755
index 0000000..f18fec7
--- /dev/null
+++ b/t/t5521-pull-symlink.sh
@@ -0,0 +1,67 @@
+#!/bin/sh
+
+test_description='pulling from symlinked subdir'
+
+. ./test-lib.sh
+
+D=`pwd`
+
+# The scenario we are building:
+#
+#   trash\ directory/
+#     clone-repo/
+#       subdir/
+#         bar
+#     subdir-link -> clone-repo/subdir/
+#
+# The working directory is subdir-link.
+#
+test_expect_success setup '
+
+    mkdir subdir &&
+    touch subdir/bar &&
+    git add subdir/bar &&
+    git commit -m empty &&
+    git clone . clone-repo &&
+    # demonstrate that things work without the symlink
+    test_debug "cd clone-repo/subdir/ && git pull; cd ../.." &&
+    ln -s clone-repo/subdir/ subdir-link &&
+    cd subdir-link/ &&
+    test_debug "set +x"
+'
+
+# From subdir-link, pulling should work as it does from
+# clone-repo/subdir/.
+#
+# Instead, the error pull gave was:
+#
+#   fatal: 'origin': unable to chdir or not a git archive
+#   fatal: The remote end hung up unexpectedly
+#
+# because git would find the .git/config for the "trash directory"
+# repo, not for the clone-repo repo.  The "trash directory" repo
+# had no entry for origin.  Git found the wrong .git because
+# git rev-parse --show-cdup printed a path relative to
+# clone-repo/subdir/, not subdir-link/.  Git rev-parse --show-cdup
+# used the correct .git, but when the git pull shell script did
+# "cd `git rev-parse --show-cdup`", it ended up in the wrong
+# directory.  Shell "cd" works a little different from chdir() in C.
+# Bash's "cd -P" works like chdir() in C.
+#
+test_expect_success 'pulling from symlinked subdir' '
+
+    git pull
+'
+
+# Prove that the remote end really is a repo, and other commands
+# work fine in this context.
+#
+test_debug "
+    test_expect_success 'pushing from symlinked subdir' '
+
+        git push
+    '
+"
+cd "$D"
+
+test_done
-- 
1.6.0.3

Re: [PATCH v2] git-sh-setup: Fix scripts whose PWD is a symlink into a git work-dir

[Junio C Hamano]

"Marcel M. Cary" <marcel@oak.homeunix.org> writes:

> * When interpretting a relative upward (../) path in cd_to_toplevel,
>   prepend the cwd without symlinks, given by /bin/pwd
> * Add tests for cd_to_toplevel and "git pull" in a symlinked
>   directory that failed before this fix, plus constrasting
>   scenarios that already worked

These are descriptions of changes (and good ones at that, but
"constrasting?").

It however is a good idea to describe the problem the patch tries to solve
*before* going into details of what you did.  "If A is B, operation C
tries to incorrectly access directory D; it should use directory E.  This
breakage is because F is confused by G..."

Yes, the "Subject:" already hints about the "If A is B" part, and the
second bullet point uses the word "failed" to hint that there was a
breakage, but that will not be sufficient description to recall the
analysis you did of the problem, when you have read the commit log message
6 months from now what the breakage was about.

In order to justify the change against "Doctor if A is B, it hurts ---
don't do it then" rebuttals, it further may make sense to defend why it is
sometimes useful to be able to satisify the precondition that triggers the
existing problem.  That would come before the problem description to
prepare readers with the context of the patch.

> diff --git a/t/t2300-cd-to-toplevel.sh b/t/t2300-cd-to-toplevel.sh
> new file mode 100755
> index 0000000..293dc35
> --- /dev/null
> +++ b/t/t2300-cd-to-toplevel.sh
> @@ -0,0 +1,37 @@
> +#!/bin/sh
> +
> +test_description='cd_to_toplevel'
> +
> +. ./test-lib.sh
> +
> +test_cd_to_toplevel () {
> +	test_expect_success "$2" '
> +		(
> +			cd '"'$1'"' &&
> +			. git-sh-setup &&
> +			cd_to_toplevel &&
> +			[ "$(pwd -P)" = "$TOPLEVEL" ]
> +		)
> +	'
> +}
> +
> +TOPLEVEL="$(pwd -P)/repo"

Hmm.  Does it make sense to assume everybody's pwd can take -P when the
primary change this patch introduces carefully avoids assuming the
availability of -P for "cd"?

> +ln -s repo symrepo
> +test_cd_to_toplevel symrepo 'at symbolic root'
> +
> +ln -s repo/sub/dir subdir-link
> +test_cd_to_toplevel subdir-link 'at symbolic subdir'
> +
> +cd repo
> +ln -s sub/dir internal-link
> +test_cd_to_toplevel internal-link 'at internal symbolic subdir'

To be very honest, although it is good that you made them work, I am still
not getting why the latter two scenarios are worth supporting.  The first
one I am Ok with, though.

> diff --git a/t/t5521-pull-symlink.sh b/t/t5521-pull-symlink.sh
> new file mode 100755
> index 0000000..f18fec7
> --- /dev/null
> +++ b/t/t5521-pull-symlink.sh
> @@ -0,0 +1,67 @@
> +#!/bin/sh
> +
> +test_description='pulling from symlinked subdir'
> +
> +. ./test-lib.sh
> +
> +D=`pwd`
> +
> +# The scenario we are building:
> +#
> +#   trash\ directory/
> +#     clone-repo/
> +#       subdir/
> +#         bar
> +#     subdir-link -> clone-repo/subdir/
> +#
> +# The working directory is subdir-link.
> +#

It is great to see the scenario explained like this.  It makes it easier
to follow what the tests are trying to do.

> +test_expect_success setup '
> +
> +    mkdir subdir &&
> +    touch subdir/bar &&
> +    git add subdir/bar &&
> +    git commit -m empty &&
> +    git clone . clone-repo &&
> +    # demonstrate that things work without the symlink
> +    test_debug "cd clone-repo/subdir/ && git pull; cd ../.." &&
> +    ln -s clone-repo/subdir/ subdir-link &&
> +    cd subdir-link/ &&
> +    test_debug "set +x"
> +'
> +
> +# From subdir-link, pulling should work as it does from
> +# clone-repo/subdir/.
> +#
> +# Instead, the error pull gave was:
> +#
> +#   fatal: 'origin': unable to chdir or not a git archive
> +#   fatal: The remote end hung up unexpectedly
> +#
> +# because git would find the .git/config for the "trash directory"
> +# repo, not for the clone-repo repo.  The "trash directory" repo
> +# had no entry for origin.  Git found the wrong .git because
> +# git rev-parse --show-cdup printed a path relative to
> +# clone-repo/subdir/, not subdir-link/.  Git rev-parse --show-cdup
> +# used the correct .git, but when the git pull shell script did
> +# "cd `git rev-parse --show-cdup`", it ended up in the wrong
> +# directory.  Shell "cd" works a little different from chdir() in C.
> +# Bash's "cd -P" works like chdir() in C.

This is a very good analysis.  s/Bash's "cd -P"/"cd -P" in POSIX shells/,
though.

> +#
> +test_expect_success 'pulling from symlinked subdir' '
> +
> +    git pull
> +'

I'd prefer to see each test_expect_success be able to fail independently,
which would mean (1) when you chdir around, do so in a subshell, and (2)
each test_expect_success assumes it begins in the same directory.

In the case of these tests, I think it is just the matter of moving the
last two lines from the previous test to the beginning of this test and
enclosing this test in (), right?

> +
> +# Prove that the remote end really is a repo, and other commands
> +# work fine in this context.
> +#
> +test_debug "
> +    test_expect_success 'pushing from symlinked subdir' '
> +
> +        git push
> +    '
> +"

Why should this be hidden inside test_debug?

[PATCH v3] git-sh-setup: Fix scripts whose PWD is a symlink into a git work-dir

[Marcel M. Cary]

I want directories of my working tree to be linked to from various
paths on my filesystem where third-party components expect them, both
in development and production environments.  A build system's install
step could solve this, but we develop scripts and web pages that don't
need to be built.  Git's submodule system could solve this, but we
tend to develop, branch, and test those directories all in unison, so
one big repository feels more natural.  We prefer to edit and commit
on the symlinked paths, not the canonical ones, and in that setting,
"git pull" fails to find the top-level directory of the working tree
and the .git directory in it.

"git pull" fails because POSIX shells have a notion of current working
directory that is different from getcwd().  The shell stores this path
in PWD.  As a result, "cd ../" in a shell script can be interpretted
differently in a shell than chdir("../") in a C program.  The shell
interprets "../" by essentially stripping the last textual path
component from PWD, whereas C chdir() follows the ".." link in the
current directory on the filesystem.  When PWD is a symlink, these are
different destinations.  As a result, Git's C commands find the
correct top-level working tree, and shell scripts do not.

Changes:

* When interpretting a relative upward (../) path in cd_to_toplevel,
  prepend the cwd without symlinks, given by /bin/pwd
* Add tests for cd_to_toplevel and "git pull" in a symlinked
  directory that failed before this fix, plus contrasting scenarios
  that already worked

Signed-off-by: Marcel M. Cary <marcel@oak.homeunix.org>
---

Hopefully the new commit message adequately describes the problem and
motivation for solving it without getting too long-winded or bogged down
in personal details.

I also removed the "pwd -P" from the unit test.  I originally worried
that it was little like testing an algorithm by running it twice and
checking the results against eachother.  But I guess having the unit
tests run on unusual platforms is more important.

The pull-symlink test cases should now fail independently.  I also
moved the setup out of the test_expect_success block for symetry and
since I see that other tests don't check for setup failure.  There's
no "set -e" or anything to catch a setup failure other than the
eventual test cases push/pull not working.  So is it good form in this
situation to not check the setup steps for success?  Or would it make
sense to put them in their own 'setup' test case?  Or would it be better
to just "exit 1" if they fail?

> > +ln -s repo symrepo
> > +test_cd_to_toplevel symrepo 'at symbolic root'
> > +
> > +ln -s repo/sub/dir subdir-link
> > +test_cd_to_toplevel subdir-link 'at symbolic subdir'
> > +
> > +cd repo
> > +ln -s sub/dir internal-link
> > +test_cd_to_toplevel internal-link 'at internal symbolic subdir'
> 
> To be very honest, although it is good that you made them work, I am still
> not getting why the latter two scenarios are worth supporting.  The first
> one I am Ok with, though.

The middle scenario is the one I want most.  I hope the first
paragraph of the commit message sheds more light on the reason.  

The third is really just there for completeness (although there are
other cases I didn't include...).  Since Git supports operation below
the top-level, and it supports symlinks, it seems useful to test the
cooperation of those features.  I wouldn't miss it much if you thought
it was not interesting enough.

> > +
> > +# Prove that the remote end really is a repo, and other commands
> > +# work fine in this context.
> > +#
> > +test_debug "
> > +    test_expect_success 'pushing from symlinked subdir' '
> > +
> > +        git push
> > +    '
> > +"
> 
> Why should this be hidden inside test_debug?

I'm not particularly trying to test "git push" or "git pull" in
general here.  That's also why the other "git pull" was in a
test_debug.  I thought it was really only useful to someone trying to
understand the contents of the test file.  There are other files that
cover push and pull.  Do you think these test cases should run all the
time here?


 git-sh-setup.sh           |   23 +++++++++++++-
 t/t2300-cd-to-toplevel.sh |   37 +++++++++++++++++++++++
 t/t5521-pull-symlink.sh   |   73 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 131 insertions(+), 2 deletions(-)
 create mode 100755 t/t2300-cd-to-toplevel.sh
 create mode 100755 t/t5521-pull-symlink.sh

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index dbdf209..f07d96b 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -85,8 +85,27 @@ cd_to_toplevel () {
 	cdup=$(git rev-parse --show-cdup)
 	if test ! -z "$cdup"
 	then
-		cd "$cdup" || {
-			echo >&2 "Cannot chdir to $cdup, the toplevel of the working tree"
+		case "$cdup" in
+		/*)
+			# Not quite the same as if we did "cd -P '$cdup'" when
+			# $cdup contains ".." after symlink path components.
+			# Don't fix that case at least until Git switches to
+			# "cd -P" across the board.
+			phys="$cdup"
+			;;
+		..|../*|*/..|*/../*)
+			# Interpret $cdup relative to the physical, not logical, cwd.
+			# Probably /bin/pwd is more portable than passing -P to cd or pwd.
+			phys="$(/bin/pwd)/$cdup"
+			;;
+		*)
+			# There's no "..", so no need to make things absolute.
+			phys="$cdup"
+			;;
+		esac
+
+		cd "$phys" || {
+			echo >&2 "Cannot chdir to $phys, the toplevel of the working tree"
 			exit 1
 		}
 	fi
diff --git a/t/t2300-cd-to-toplevel.sh b/t/t2300-cd-to-toplevel.sh
new file mode 100755
index 0000000..05854b4
--- /dev/null
+++ b/t/t2300-cd-to-toplevel.sh
@@ -0,0 +1,37 @@
+#!/bin/sh
+
+test_description='cd_to_toplevel'
+
+. ./test-lib.sh
+
+test_cd_to_toplevel () {
+	test_expect_success "$2" '
+		(
+			cd '"'$1'"' &&
+			. git-sh-setup &&
+			cd_to_toplevel &&
+			[ "$(pwd -P)" = "$TOPLEVEL" ]
+		)
+	'
+}
+
+TOPLEVEL="$(/bin/pwd)/repo"
+mkdir -p repo/sub/dir
+mv .git repo/
+SUBDIRECTORY_OK=1
+
+test_cd_to_toplevel repo 'at physical root'
+
+test_cd_to_toplevel repo/sub/dir 'at physical subdir'
+
+ln -s repo symrepo
+test_cd_to_toplevel symrepo 'at symbolic root'
+
+ln -s repo/sub/dir subdir-link
+test_cd_to_toplevel subdir-link 'at symbolic subdir'
+
+cd repo
+ln -s sub/dir internal-link
+test_cd_to_toplevel internal-link 'at internal symbolic subdir'
+
+test_done
diff --git a/t/t5521-pull-symlink.sh b/t/t5521-pull-symlink.sh
new file mode 100755
index 0000000..8869262
--- /dev/null
+++ b/t/t5521-pull-symlink.sh
@@ -0,0 +1,73 @@
+#!/bin/sh
+
+test_description='pulling from symlinked subdir'
+
+. ./test-lib.sh
+
+# The scenario we are building:
+#
+#   trash\ directory/
+#     clone-repo/
+#       subdir/
+#         bar
+#     subdir-link -> clone-repo/subdir/
+#
+# The working directory is subdir-link.
+
+mkdir subdir
+touch subdir/bar
+git add subdir/bar
+git commit -m empty
+git clone . clone-repo
+ln -s clone-repo/subdir/ subdir-link
+
+
+# Demonstrate that things work if we just avoid the symlink
+#
+test_debug "
+	test_expect_success 'pulling from real subdir' '
+		(
+			cd clone-repo/subdir/ &&
+			git pull
+		)
+	'
+"
+
+# From subdir-link, pulling should work as it does from
+# clone-repo/subdir/.
+#
+# Instead, the error pull gave was:
+#
+#   fatal: 'origin': unable to chdir or not a git archive
+#   fatal: The remote end hung up unexpectedly
+#
+# because git would find the .git/config for the "trash directory"
+# repo, not for the clone-repo repo.  The "trash directory" repo
+# had no entry for origin.  Git found the wrong .git because
+# git rev-parse --show-cdup printed a path relative to
+# clone-repo/subdir/, not subdir-link/.  Git rev-parse --show-cdup
+# used the correct .git, but when the git pull shell script did
+# "cd `git rev-parse --show-cdup`", it ended up in the wrong
+# directory.  A POSIX shell's "cd" works a little differently
+# than chdir() in C; "cd -P" is much closer to chdir().
+#
+test_expect_success 'pulling from symlinked subdir' '
+	(
+		cd subdir-link/ &&
+		git pull
+	)
+'
+
+# Prove that the remote end really is a repo, and other commands
+# work fine in this context.  It's just that "git pull" breaks.
+#
+test_debug "
+	test_expect_success 'pushing from symlinked subdir' '
+		(
+			cd subdir-link/ &&
+			git push
+		)
+	'
+"
+
+test_done
-- 
1.6.0.3

Re: [PATCH v3] git-sh-setup: Fix scripts whose PWD is a symlink into a git work-dir

[Junio C Hamano]

"Marcel M. Cary" <marcel@oak.homeunix.org> writes:

> I also removed the "pwd -P" from the unit test.

Hmm, really...?

>> > +# Prove that the remote end really is a repo, and other commands
>> > +# work fine in this context.
>> > +#
>> > +test_debug "
>> > +    test_expect_success 'pushing from symlinked subdir' '
>> > +
>> > +        git push
>> > +    '
>> > +"
>> 
>> Why should this be hidden inside test_debug?
>
> I'm not particularly trying to test "git push" or "git pull" in general
> here.  That's also why the other "git pull" was in a test_debug.  I
> thought it was really only useful to someone trying to understand the
> contents of the test file.  There are other files that cover push and
> pull.  Do you think these test cases should run all the time here?

I'd say so.  Your supporting argument could be "See, push works just fine
with this layout, but pull doesn't because it is a shell script that can
be fooled, and this change is to fix the inconsistencies between them."
Having these test enabled would be a good way to do so.  Then it becomes
irrelevant if "jump into the middle of a directory hierarchy sideways via
symlink" is worth supporting or not ;-)

But whether it is inside test_debug or not, the test should check not just
the exit status from 'git push' but also check what happened to the
receiving repository at least to make sure it is pushing to the location
you are expecting it to.

> diff --git a/t/t2300-cd-to-toplevel.sh b/t/t2300-cd-to-toplevel.sh
> new file mode 100755
> index 0000000..05854b4
> --- /dev/null
> +++ b/t/t2300-cd-to-toplevel.sh
> @@ -0,0 +1,37 @@
> +#!/bin/sh
> +
> +test_description='cd_to_toplevel'
> +
> +. ./test-lib.sh
> +
> +test_cd_to_toplevel () {
> +	test_expect_success "$2" '
> +		(
> +			cd '"'$1'"' &&
> +			. git-sh-setup &&
> +			cd_to_toplevel &&
> +			[ "$(pwd -P)" = "$TOPLEVEL" ]
> +		)
> +	'
> +}

The quoting of $1 here is a bit tricky, but I think it is good enough for
directory names used in tests that use this function.

[PATCH v3] git-sh-setup: Fix scripts whose PWD is a symlink into a git work-dir

[Marcel M. Cary]

I want directories of my working tree to be linked to from various
paths on my filesystem where third-party components expect them, both
in development and production environments.  A build system's install
step could solve this, but I develop scripts and web pages that don't
need to be built.  Git's submodule system could solve this, but we
tend to develop, branch, and test those directories all in unison, so
one big repository feels more natural.  We prefer to edit and commit
on the symlinked paths, not the canonical ones, and in that setting,
"git pull" fails to find the top-level directory of the repository
while other commands work fine.

"git pull" fails because POSIX shells have a notion of current working
directory that is different from getcwd().  The shell stores this path
in PWD.  As a result, "cd ../" can be interpreted differently in a
shell script than chdir("../") in a C program.  The shell interprets
"../" by essentially stripping the last textual path component from
PWD, whereas C chdir() follows the ".." link in the current directory
on the filesystem.  When PWD is a symlink, these are different
destinations.  As a result, Git's C commands find the correct
top-level working tree, and shell scripts do not.

Changes:

* When interpreting a relative upward (../) path in cd_to_toplevel,
  prepend the cwd without symlinks, given by /bin/pwd
* Add tests for cd_to_toplevel and "git pull" in a symlinked
  directory that failed before this fix, plus contrasting scenarios
  that already worked

Signed-off-by: Marcel M. Cary <marcel@oak.homeunix.org>
---

> > I also removed the "pwd -P" from the unit test.
>
> Hmm, really...?

Ouch.  Removing just one occurrence won't help much, will it.

> > Do you think these test cases should run all the time here?
>
> I'd say so.  Your supporting argument could be "See, push works just
> fine with this layout, but pull doesn't because it is a shell script
> that can be fooled, and this change is to fix the inconsistencies
> between them."

Ok, removed those cases from test_debug and emphasized in the first
paragraph of the commit message that other commands support this kind
of "sideways jumping."

> But whether it is inside test_debug or not, the test should check
> not just the exit status from 'git push' but also check what
> happened to the receiving repository at least to make sure it is
> pushing to the location you are expecting it to.

Ok, I did this by adding an additional file each time and checking the
same path in the other repository.


 git-sh-setup.sh           |   23 ++++++++++++-
 t/t2300-cd-to-toplevel.sh |   37 +++++++++++++++++++++
 t/t5521-pull-symlink.sh   |   78 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 136 insertions(+), 2 deletions(-)
 create mode 100755 t/t2300-cd-to-toplevel.sh
 create mode 100755 t/t5521-pull-symlink.sh

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index dbdf209..f07d96b 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -85,8 +85,27 @@ cd_to_toplevel () {
 	cdup=$(git rev-parse --show-cdup)
 	if test ! -z "$cdup"
 	then
-		cd "$cdup" || {
-			echo >&2 "Cannot chdir to $cdup, the toplevel of the working tree"
+		case "$cdup" in
+		/*)
+			# Not quite the same as if we did "cd -P '$cdup'" when
+			# $cdup contains ".." after symlink path components.
+			# Don't fix that case at least until Git switches to
+			# "cd -P" across the board.
+			phys="$cdup"
+			;;
+		..|../*|*/..|*/../*)
+			# Interpret $cdup relative to the physical, not logical, cwd.
+			# Probably /bin/pwd is more portable than passing -P to cd or pwd.
+			phys="$(/bin/pwd)/$cdup"
+			;;
+		*)
+			# There's no "..", so no need to make things absolute.
+			phys="$cdup"
+			;;
+		esac
+
+		cd "$phys" || {
+			echo >&2 "Cannot chdir to $phys, the toplevel of the working tree"
 			exit 1
 		}
 	fi
diff --git a/t/t2300-cd-to-toplevel.sh b/t/t2300-cd-to-toplevel.sh
new file mode 100755
index 0000000..beddb4e
--- /dev/null
+++ b/t/t2300-cd-to-toplevel.sh
@@ -0,0 +1,37 @@
+#!/bin/sh
+
+test_description='cd_to_toplevel'
+
+. ./test-lib.sh
+
+test_cd_to_toplevel () {
+	test_expect_success "$2" '
+		(
+			cd '"'$1'"' &&
+			. git-sh-setup &&
+			cd_to_toplevel &&
+			[ "$(/bin/pwd)" = "$TOPLEVEL" ]
+		)
+	'
+}
+
+TOPLEVEL="$(/bin/pwd)/repo"
+mkdir -p repo/sub/dir
+mv .git repo/
+SUBDIRECTORY_OK=1
+
+test_cd_to_toplevel repo 'at physical root'
+
+test_cd_to_toplevel repo/sub/dir 'at physical subdir'
+
+ln -s repo symrepo
+test_cd_to_toplevel symrepo 'at symbolic root'
+
+ln -s repo/sub/dir subdir-link
+test_cd_to_toplevel subdir-link 'at symbolic subdir'
+
+cd repo
+ln -s sub/dir internal-link
+test_cd_to_toplevel internal-link 'at internal symbolic subdir'
+
+test_done
diff --git a/t/t5521-pull-symlink.sh b/t/t5521-pull-symlink.sh
new file mode 100755
index 0000000..5672b51
--- /dev/null
+++ b/t/t5521-pull-symlink.sh
@@ -0,0 +1,78 @@
+#!/bin/sh
+
+test_description='pulling from symlinked subdir'
+
+. ./test-lib.sh
+
+# The scenario we are building:
+#
+#   trash\ directory/
+#     clone-repo/
+#       subdir/
+#         bar
+#     subdir-link -> clone-repo/subdir/
+#
+# The working directory is subdir-link.
+
+mkdir subdir
+echo file >subdir/file
+git add subdir/file
+git commit -q -m file
+git clone -q . clone-repo
+ln -s clone-repo/subdir/ subdir-link
+
+
+# Demonstrate that things work if we just avoid the symlink
+#
+test_expect_success 'pulling from real subdir' '
+	(
+		echo real >subdir/file &&
+		git commit -m real subdir/file &&
+		cd clone-repo/subdir/ &&
+		git pull &&
+		test real = $(cat file)
+	)
+'
+
+# From subdir-link, pulling should work as it does from
+# clone-repo/subdir/.
+#
+# Instead, the error pull gave was:
+#
+#   fatal: 'origin': unable to chdir or not a git archive
+#   fatal: The remote end hung up unexpectedly
+#
+# because git would find the .git/config for the "trash directory"
+# repo, not for the clone-repo repo.  The "trash directory" repo
+# had no entry for origin.  Git found the wrong .git because
+# git rev-parse --show-cdup printed a path relative to
+# clone-repo/subdir/, not subdir-link/.  Git rev-parse --show-cdup
+# used the correct .git, but when the git pull shell script did
+# "cd `git rev-parse --show-cdup`", it ended up in the wrong
+# directory.  A POSIX shell's "cd" works a little differently
+# than chdir() in C; "cd -P" is much closer to chdir().
+#
+test_expect_success 'pulling from symlinked subdir' '
+	(
+		echo link >subdir/file &&
+		git commit -m link subdir/file &&
+		cd subdir-link/ &&
+		git pull &&
+		test link = $(cat file)
+	)
+'
+
+# Prove that the remote end really is a repo, and other commands
+# work fine in this context.  It's just that "git pull" breaks.
+#
+test_expect_success 'pushing from symlinked subdir' '
+	(
+		cd subdir-link/ &&
+		echo push >file &&
+		git commit -m push ./file &&
+		git push
+	) &&
+	test push = $(git show HEAD:subdir/file)
+'
+
+test_done
-- 
1.6.0.3

Re: [PATCH v3] <-- really v4

[Marcel M. Cary]

I guess I'm really on v4, sorry.

Marcel

[RFC PATCH] git-sh-setup: Use "cd" option, not /bin/pwd, for symlinked work tree

[Marcel M. Cary]

In cd_to_toplevel, instead of 'cd $(unset PWD; /bin/pwd)/$path'
use 'cd -P $path'.  The "-P" option yields a desirable similarity to
C chdir.

While the "-P" option may be slightly less commonly supported than
/bin/pwd, it is more concise, better tested, and less error prone.
I've already added the 'unset PWD' to fix the /bin/pwd solution on
BSD; there may be more edge cases out there.

This still passes all the same test cases in t5521-pull-symlink.sh and
t2300-cd-to-toplevel.sh, even before updating them to use 'pwd -P'.
---

> ... I think it would probably be better to bite the bullet
> and start using "cd -P" soon after 1.6.1 goes final, ...

Here's a post-1.6.1 way to make Git shell scripts work like C programs
when in symlinked directories.

> ... and at the same time
> existing places that use "cd `pwd`" as a workaround if there are some.

I haven't found other places that use the "cd `/bin/pwd`" workaround.

I also wasn't able to think of a test case that fails under the previous
way and works under this new way either.

Any opinions on whether this is worthwhile?  It seems more standardized
if less supported, odd as that seems to me.

Marcel

 git-sh-setup.sh           |   29 ++++++++---------------------
 t/t2300-cd-to-toplevel.sh |    4 ++--
 2 files changed, 10 insertions(+), 23 deletions(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 2142308..8382339 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -85,27 +85,14 @@ cd_to_toplevel () {
 	cdup=$(git rev-parse --show-cdup)
 	if test ! -z "$cdup"
 	then
-		case "$cdup" in
-		/*)
-			# Not quite the same as if we did "cd -P '$cdup'" when
-			# $cdup contains ".." after symlink path components.
-			# Don't fix that case at least until Git switches to
-			# "cd -P" across the board.
-			phys="$cdup"
-			;;
-		..|../*|*/..|*/../*)
-			# Interpret $cdup relative to the physical, not logical, cwd.
-			# Probably /bin/pwd is more portable than passing -P to cd or pwd.
-			phys="$(unset PWD; /bin/pwd)/$cdup"
-			;;
-		*)
-			# There's no "..", so no need to make things absolute.
-			phys="$cdup"
-			;;
-		esac
-
-		cd "$phys" || {
-			echo >&2 "Cannot chdir to $phys, the toplevel of the working tree"
+		# The "-P" option says to follow "physical" directory
+		# structure instead of following symbolic links.  When cdup is
+		# "../", this means following the ".." entry in the current
+		# directory instead textually removing a symlink path element
+		# from the PWD shell variable.  The "-P" behavior is more
+		# consistent with the C-style chdir used by most of Git.
+		cd -P "$cdup" || {
+			echo >&2 "Cannot chdir to $cdup, the toplevel of the working tree"
 			exit 1
 		}
 	fi
diff --git a/t/t2300-cd-to-toplevel.sh b/t/t2300-cd-to-toplevel.sh
index e42cbfe..293dc35 100755
--- a/t/t2300-cd-to-toplevel.sh
+++ b/t/t2300-cd-to-toplevel.sh
@@ -10,12 +10,12 @@ test_cd_to_toplevel () {
 			cd '"'$1'"' &&
 			. git-sh-setup &&
 			cd_to_toplevel &&
-			[ "$(unset PWD; /bin/pwd)" = "$TOPLEVEL" ]
+			[ "$(pwd -P)" = "$TOPLEVEL" ]
 		)
 	'
 }
 
-TOPLEVEL="$(unset PWD; /bin/pwd)/repo"
+TOPLEVEL="$(pwd -P)/repo"
 mkdir -p repo/sub/dir
 mv .git repo/
 SUBDIRECTORY_OK=1
-- 
1.6.1

Re: [RFC PATCH] git-sh-setup: Use "cd" option, not /bin/pwd, for symlinked work tree

[Johannes Schindelin]

Hi,

On Fri, 6 Feb 2009, Marcel M. Cary wrote:

> While the "-P" option may be slightly less commonly supported than
> /bin/pwd,

Does this not suggest that your patch should at least fall back to using 
/bin/pwd when it was detected that "cd -P" does not work?

Ciao,
Dscho

Re: [RFC PATCH] git-sh-setup: Use "cd" option, not /bin/pwd, for symlinked work tree

[Marcel M. Cary]

Johannes Schindelin wrote:
> On Fri, 6 Feb 2009, Marcel M. Cary wrote:
>> While the "-P" option may be slightly less commonly supported than
>> /bin/pwd,
>
> Does this not suggest that your patch should at least fall back to
> using /bin/pwd when it was detected that "cd -P" does not work?

Having the "cd -P" strategy fall back to /bin/pwd negates most of the
value I saw in using the simpler strategy.

I haven't found cases where "cd -P" is more correct.  Are there other
reasons to bother with "cd -P" at all?  Maybe performance: "cd -P"
would save a fork, which seems to make it ~10x faster.  Dropping buffer
caches doesn't seem to widen or narrow the gap, so I don't think the
filesystem access is much different, performance-wise.  But I don't
expect this "cd" to be a performance bottleneck; most scripts that do
something repetitive can just start off in the work tree root to avoid
the issue.

Falling back to /bin/pwd would help compatibility if it were easy to
detect when "cd -P" failed.  But since its failure is hypothetical for
me at this point -- I don't know of an environment where it fails -- I'm
not sure whether to expect it to fail with non-zero exit status or by
silently ignoring the "-P".  And to handle the cases of silently
ignoring the "-P" I'd guess cd_to_toplevel would have to run /bin/pwd
just to check that it ended up in the right place, which seems
counterproductive to me.  Do you think it would be reasonable to just
assume "cd -P" will exit non-zero if "cd" doesn't understand "-P", send
its stderr to /dev/null, and try again using /bin/pwd?

Marcel

Re: [RFC PATCH] git-sh-setup: Use "cd" option, not /bin/pwd, for symlinked work tree

[Johannes Schindelin]

Hi,

On Sun, 8 Feb 2009, Marcel M. Cary wrote:

> Johannes Schindelin wrote:
> > On Fri, 6 Feb 2009, Marcel M. Cary wrote:
> >> While the "-P" option may be slightly less commonly supported than
> >> /bin/pwd,
> >
> > Does this not suggest that your patch should at least fall back to
> > using /bin/pwd when it was detected that "cd -P" does not work?
> 
> Having the "cd -P" strategy fall back to /bin/pwd negates most of the
> value I saw in using the simpler strategy.
> 
> I haven't found cases where "cd -P" is more correct.

Actually, it was not clear for me how much you researched the portability 
of "cd -P".

As long as it is not proven that your patch keeps working setups working, 
I think you'll have to put in a bit more effort, research it, and then put 
the discussion into the commit message.

Ciao,
Dscho

Re: [RFC PATCH] git-sh-setup: Use "cd" option, not /bin/pwd, for symlinked work tree

[Marcel M. Cary]

Johannes Schindelin wrote:
> On Sun, 8 Feb 2009, Marcel M. Cary wrote:
>> Johannes Schindelin wrote:
>>> On Fri, 6 Feb 2009, Marcel M. Cary wrote:
>>>> While the "-P" option may be slightly less commonly supported than
>>>> /bin/pwd,
>>> Does this not suggest that your patch should at least fall back to
>>> using /bin/pwd when it was detected that "cd -P" does not work?
>> Having the "cd -P" strategy fall back to /bin/pwd negates most of the
>> value I saw in using the simpler strategy.
>>
>> I haven't found cases where "cd -P" is more correct.
> 
> Actually, it was not clear for me how much you researched the portability 
> of "cd -P".

I have not.  I've seen only that it's POSIX, is on BSD and Linux, and
was suggested by Junio.

> As long as it is not proven that your patch keeps working setups working, 
> I think you'll have to put in a bit more effort, research it, and then put 
> the discussion into the commit message.

Actually, since I haven't heard any continued interest in following up
with the suggestion to use "cd -P", I don't see much benefit myself, and
there is concern about it not being compatible enough, I'm content to
just table this.

I agree that keeping working setups working is important, and it seems
like a major project to research portability of "cd -P" on a list of
platforms that I'm guessing I'd have to collect myself.

Marcel

Re: [RFC PATCH] git-sh-setup: Use "cd" option, not /bin/pwd, for symlinked work tree

[Jeff King]

On Wed, Feb 11, 2009 at 06:44:41AM -0800, Marcel M. Cary wrote:

> > Actually, it was not clear for me how much you researched the portability 
> > of "cd -P".
> 
> I have not.  I've seen only that it's POSIX, is on BSD and Linux, and
> was suggested by Junio.

Even Solaris /usr/xpg4/bin/sh has it. Their /bin/sh does not, but that
is not a surprise: that shell is useless and already unsupported by git.

I don't know about other obscure platforms (wasn't there some guy
running git on antique SCO machines or something?).

I think it is nice to shoot for "more portable" in general, and I don't
particularly care one way or the other about this feature. But I think
we are somewhat hampered by having no clue what the supported set of
platforms is. I'm pretty sure we support at least:

  - various recent Linux distributions
  - FreeBSD 6.x (maybe as far back as 4.x)
  - OS X
  - NetBSD and OpenBSD, but no idea which versions
  - Solaris >= 2.8
  - AIX 5.3

and I suspect most of those have somebody building them regularly enough
that breakages are caught. I have no idea what people are using beyond
that, and how quickly they might catch a portability breakage.

-Peff