On Mac OS X and possibly BSDs, /bin/pwd reads PWD from the environment if available and shows the logical path by default rather than the physical one. Unset PWD before running /bin/pwd in both cd_to_toplevel and its test. Still use the external /bin/pwd because in my Bash on Linux, the builtin pwd prints the same result whether or not PWD is set. --- Brian Gernhardt wrote: > I didn't pay attention to this at the time, but I just tried to build a > new version of git and noticed this... > >> + ..|../*|*/..|*/../*) >> + # 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" >> + ;; > > This is a non-portable construct. Notably, on OS X (and possibly some > BSDs) /bin/pwd does not give the physical path, but $(/bin/pwd -P) > does. Ouch! Junio C Hamano wrote: > 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. Perhaps it's time to start using "cd -P" with the recent release of 1.6.1. But maybe it's also worth finding a fix that doesn't rely on it for any maintenance release of 1.6 that might happen? Brian Gernhardt wrote: > We may have to build this string ourselves with a --show-cd-absolute > for portability. Some options I considered: (1) We could implement --show-cd-absolute in the short term, with the expectation of removing it we switch to "cd -P". Not sure it would really work to try to remove a switch, even if it were undocumented, or documented as deprecated. (2) We could check the output of /bin/pwd for ".." and if they are still present, add the "-P". But I suppose there's no guarantee "-P" would work, and I shy away from additional analysis of the path because I don't want to miss an edge case. (3) We could unset PWD before running /bin/pwd. (Note that in my Bash, the pwd *builtin* prints the same result whether or not I unset PWD first.) I suppose it's possible some /bin/pwd dies when PWD is unset or something, but that seems unlikely. From http://developer.apple.com/DOCUMENTATION/Darwin/Reference/ManPages/man1/pwd.1.html > The -L option does not work unless the PWD environment variable is > exported by the shell. I like option 3 best. Any thoughts? I sent the first rev of this patch to just Brian. It didn't have either of the unit test changes. He said it fixed all but t2300.3, where cd_to_toplevel doesn't actually "cd", so I made the same change to the unit test itself. Can someone with OS X try running the test suite with v2 of this patch? I don't have OS X readily available. Marcel git-sh-setup.sh | 2 +- t/t2300-cd-to-toplevel.sh | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/git-sh-setup.sh b/git-sh-setup.sh index f07d96b..2142308 100755 --- a/git-sh-setup.sh +++ b/git-sh-setup.sh @@ -96,7 +96,7 @@ cd_to_toplevel () { ..|../*|*/..|*/../*) # 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" + phys="$(unset PWD; /bin/pwd)/$cdup" ;; *) # There's no "..", so no need to make things absolute. diff --git a/t/t2300-cd-to-toplevel.sh b/t/t2300-cd-to-toplevel.sh index beddb4e..e42cbfe 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 && - [ "$(/bin/pwd)" = "$TOPLEVEL" ] + [ "$(unset PWD; /bin/pwd)" = "$TOPLEVEL" ] ) ' } -TOPLEVEL="$(/bin/pwd)/repo" +TOPLEVEL="$(unset PWD; /bin/pwd)/repo" mkdir -p repo/sub/dir mv .git repo/ SUBDIRECTORY_OK=1 -- 1.6.0.3 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
