[PATCH rfc v2] git-sh-setup: Fix scripts whose PWD is a symlink to a work-dir on OS X

[PATCH rfc v2] git-sh-setup: Fix scripts whose PWD is a symlink to a work-dir on OS X

[Marcel M. Cary]

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

Re: [PATCH rfc v2] git-sh-setup: Fix scripts whose PWD is a symlink to a work-dir on OS X

[Marcel Koeppen]

Hi,

Am 30.12.2008 um 16:10 schrieb Marcel M. Cary:

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

the patch fixes t2300-cd-to-toplevel and t5521-pull-symlink for me.

	Marcel

[I don't know why my replies get lost, so I dropped all individual  
recipients on this third try...]

[PATCH v2 tested] git-sh-setup: Fix scripts whose PWD is a symlink to a work-dir on OS X

[Marcel M. Cary]

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.

Signed-off-by: Marcel M. Cary <marcel@oak.homeunix.org>
Tested-by: Marcel Koeppen <git-dev@marzelpan.de>
---

Junio C Hamano wrote:
> I think I saw a success report on the list.  Care to resend it with
> Sign-off (by you) and
> 
>         Tested-by: tester <test@er.xz> (on PLATFORM)
> 
> lines as you see necessary for application?

Same as before but with S-o-b/T-b lines.

Marcel Koeppen wrote:
> [I don't know why my replies get lost, so I dropped all individual
> recipients on this third try...]

I noticed that Brian Gernhardt's message also didn't make it to the
list, even though it was addressed to the list.  I'm not sure why.


 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.1

Re: [PATCH rfc v2] git-sh-setup: Fix scripts whose PWD is a symlink to a work-dir on OS X

[Junio C Hamano]

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

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

I think I saw a success report on the list.  Care to resend it with
Sign-off (by you) and

	Tested-by: tester <test@er.xz> (on PLATFORM)

lines as you see necessary for application?

Thanks.

Re: [PATCH rfc v2] git-sh-setup: Fix scripts whose PWD is a symlink to a work-dir on OS X

[Wincent Colaiuta]

El 3/1/2009, a las 23:01, Junio C Hamano escribió:

> "Marcel M. Cary" <marcel@oak.homeunix.org> writes:
>
>> 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.
>
> I think I saw a success report on the list.  Care to resend it with
> Sign-off (by you) and
>
> 	Tested-by: tester <test@er.xz> (on PLATFORM)
>
> lines as you see necessary for application?
>
> Thanks.

I also tested it and can confirm that it fixes the failures on Mac OS  
X 10.5.5. So feel free to add:

Tested-by: Wincent Colaiuta <win@wincent.com> (on Mac OS X 10.5.5)

Cheers,
Wincent

Re: [PATCH rfc v2] git-sh-setup: Fix scripts whose PWD is a symlink to a work-dir on OS X

[Marcel Koeppen]

Am 03.01.2009 um 23:01 schrieb Junio C Hamano:

> "Marcel M. Cary" <marcel@oak.homeunix.org> writes:
>
>> 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.
>
> I think I saw a success report on the list.  Care to resend it with
> Sign-off (by you) and
>
> 	Tested-by: tester <test@er.xz> (on PLATFORM)
>
> lines as you see necessary for application?
>
> Thanks.


Hi,

please add

Tested-by: Marcel Koeppen <git-dev@marzelpan.de> (on Mac OS X 10.5.6)

	Marcel

[PATCH v2 tested-v2] git-sh-setup: Fix scripts whose PWD is a symlink to a work-dir on OS X

[Marcel M. Cary]

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.

Signed-off-by: Marcel M. Cary <marcel@oak.homeunix.org>
Tested-by: Marcel Koeppen <git-dev@marzelpan.de> (on Mac OS X 10.5.6)
---

> please add
> 
> Tested-by: Marcel Koeppen <git-dev@marzelpan.de> (on Mac OS X 10.5.6)

Now with the OS, in detail.


 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.1

Re: [PATCH v2 tested-v2] git-sh-setup: Fix scripts whose PWD is a symlink to a work-dir on OS X

[Junio C Hamano]

Thanks; queued.