[PATCH 2/2] Use $(git rev-parse --show-toplevel) in cd_to_toplevel()

[PATCH 2/2] Use $(git rev-parse --show-toplevel) in cd_to_toplevel()

[Steven Drake]

as it gives the absolute (aka "physical") path of the toplevel directory 
and 'cd -P' is not supported by all shell implementations.

See NetBSD PR/42168.
http://www.netbsd.org/cgi-bin/query-pr-single.pl?number=42168
---
 git-sh-setup.sh |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index dfcb807..3cbec05 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -120,16 +120,10 @@ is_bare_repository () {
 }
 
 cd_to_toplevel () {
-	cdup=$(git rev-parse --show-cdup)
-	if test ! -z "$cdup"
+	if test ! -z "$(git rev-parse --show-cdup)"
 	then
-		# 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" || {
+		cdup=$(git rev-parse --show-toplevel)
+		cd "$cdup" || {
 			echo >&2 "Cannot chdir to $cdup, the toplevel of the working tree"
 			exit 1
 		}
-- 
1.6.4

Re: [PATCH 2/2] Use $(git rev-parse --show-toplevel) in cd_to_toplevel()

[Jonathan Nieder]

Steven Drake wrote:

> 'cd -P' is not supported by all shell implementations.

Makes sense.

git already does not handle those cases where 'cd "$(pwd)"' fails
while 'cd .' does not, so your patch shouldn’t break anything. [1]

> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index dfcb807..3cbec05 100755
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -120,16 +120,10 @@ is_bare_repository () {
>  }
>  
>  cd_to_toplevel () {
> -	cdup=$(git rev-parse --show-cdup)
> -	if test ! -z "$cdup"
> +	if test ! -z "$(git rev-parse --show-cdup)"
>  	then
> -		# 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" || {
> +		cdup=$(git rev-parse --show-toplevel)
> +		cd "$cdup" || {

Why not 'cdup=$(git rev-parse --show-toplevel) && cd "$cdup"'
unconditionally?  That would avoid having to look for the .git dir
twice and would mirror setup_work_tree() a bit more closely.

Avoiding -P not only improves portability but makes the function
easier to understand.  I like it.

Thanks,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/135563/focus=135571

Re: [PATCH 2/2] Use $(git rev-parse --show-toplevel) in cd_to_toplevel()

[Steven Drake]

On Sun, 10 Jan 2010, Jonathan Nieder wrote:
> Why not 'cdup=$(git rev-parse --show-toplevel) && cd "$cdup"'
> unconditionally?  That would avoid having to look for the .git dir
> twice and would mirror setup_work_tree() a bit more closely.

Depends on what you think is the lesser of to evials, running 
'git rev-parse' twice or preforming a cd when theres no need to, but after
looking at setup_work_tree() I agree with you and will send in a new patch.

-- 
Steven

[PATCH 2/2] Use $(git rev-parse --show-toplevel) in cd_to_toplevel()

[Steven Drake]

 as it gives the absolute (aka "physical") path of the toplevel directory
 and 'cd -P' is not supported by all shell implementations.

See NetBSD PR/42168.
http://www.netbsd.org/cgi-bin/query-pr-single.pl?number=42168
---
This is a revision of my original patch after the input from Jonathan.
---
 git-sh-setup.sh |   18 ++++--------------
 1 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index dfcb807..4ecc578 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -120,20 +120,10 @@ is_bare_repository () {
 }
 
 cd_to_toplevel () {
-	cdup=$(git rev-parse --show-cdup)
-	if test ! -z "$cdup"
-	then
-		# 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
+	cdup=$(git rev-parse --show-toplevel) && cd "$cdup" || {
+		echo >&2 "Cannot chdir to $cdup, the toplevel of the working tree"
+		exit 1
+	}
 }
 
 require_work_tree () {
-- 
1.6.4

Re: [PATCH 2/2] Use $(git rev-parse --show-toplevel) in cd_to_toplevel()

[Junio C Hamano]

Steven Drake <sdrake@xnet.co.nz> writes:

>  as it gives the absolute (aka "physical") path of the toplevel directory
>  and 'cd -P' is not supported by all shell implementations.
>
> See NetBSD PR/42168.
> http://www.netbsd.org/cgi-bin/query-pr-single.pl?number=42168
> ---
> This is a revision of my original patch after the input from Jonathan.
> ---
>  git-sh-setup.sh |   18 ++++--------------
>  1 files changed, 4 insertions(+), 14 deletions(-)

The patch text may make sense (I haven't thought things through yet) but
please make sure your proposed commit log messages conform to the local
convention.

 (1) We don't chop a sentence in the middle of the subject line and
     continue the same sentence to the first line of the body of the
     message;

 (2) We don't indent the log message by one SP at the beginning of lines;

 (3) Please avoid referring to external resource in the commit log message
     whenever makes sense; the log should be understandable on its own.
     Because the first paragraph of your message describes the issue the
     patch addresses very well already, you don't need "See NetBSD..." and
     URL.  If you want to have them to help the reviewers, place such
     reference after the three-dash line, just like you wrote "This is a
     revision..."  You would help reviewers even more if you added a
     pointer to your earlier patch after that sentence;

 (4) Sign your patch, before the three-dash line.

> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index dfcb807..4ecc578 100755
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -120,20 +120,10 @@ is_bare_repository () {
>  }
>  
>  cd_to_toplevel () {
> +	cdup=$(git rev-parse --show-toplevel) && cd "$cdup" || {

Please line-break immediately after &&; it makes it easier to read in
general, and it would make "cd" stand out in this particular case, as it
is the most important part of this particular function.

Thanks.

Re: [PATCH 2/2] Use $(git rev-parse --show-toplevel) in cd_to_toplevel()

[Steven Drake]

On Sun, 10 Jan 2010, Junio C Hamano wrote:

>  (3) Please avoid referring to external resource in the commit log message
>      whenever makes sense; the log should be understandable on its own.
>      Because the first paragraph of your message describes the issue the
>      patch addresses very well already, you don't need "See NetBSD..." and
>      URL.  If you want to have them to help the reviewers, place such
>      reference after the three-dash line, just like you wrote "This is a
>      revision..."  You would help reviewers even more if you added a
>      pointer to your earlier patch after that sentence;

Wondered wether I should have put the extra info after the three-dashes or
not, now I know.

I also made sure my second email had References and In-Reply-To headers to 
my first email.

>  (4) Sign your patch, before the three-dash line.

Opps forgot '--signoff', I've put format.signoff=ture in .gitconfig to solve
that problem. 

Perhaps a warning message from format-patch of the form:
WARNING: You have not added a "Signed-off-by:" line did you mean to!

> Please line-break immediately after &&; it makes it easier to read in
> general, and it would make "cd" stand out in this particular case, as it
> is the most important part of this particular function.

Good point, althought did you mean as a general shell script coding rule or
just in this particular case.

Thanks for the feedback.  Did you want me to resend the signed and cleaned up
patches direct to you?  

-- 
Steven

Re: [PATCH 2/2] Use $(git rev-parse --show-toplevel) in cd_to_toplevel()

[Junio C Hamano]

Steven Drake <sdrake@xnet.co.nz> writes:

>>  (4) Sign your patch, before the three-dash line.
>
> Opps forgot '--signoff', I've put format.signoff=ture in .gitconfig to solve
> that problem. 
>
> Perhaps a warning message from format-patch of the form:
> WARNING: You have not added a "Signed-off-by:" line did you mean to!

1. It is usually a good idea to make it a habit of running "commit -s",
   iow, record your sign-off at _commit time_, when working on a project
   that uses the convention.  You may start contributing by sending a
   pull-request instead of patches later.

2. scripts/checkpatch.pl script in the Linux kernel project is a good tool
   to check your patch before submission; you run it as:

   $ perl checkpatch.pl --no-tree 0001-my-changes.patch
   
>> Please line-break immediately after &&; it makes it easier to read in
>> general, and it would make "cd" stand out in this particular case, as it
>> is the most important part of this particular function.
>
> Good point, althought did you mean as a general shell script coding rule or
> just in this particular case.

"In general" as a general style suggestion, and "show 'cd' at the
beginning on its own line" as a more reason to do so for this specific
case.

> Thanks for the feedback.  Did you want me to resend the signed and cleaned up
> patches direct to you?  

Or to the list.

I could actually have fixed them up and commit myself, instead of
responding with comments that can be seen as if I were nitpicking.

The reason why I chose not to was because I hoped you would keep
contributing to this project more in the future (I see you have already
another commit in our history, and you seem to be reasonably competent,
judging from the patch description and the way you communicate in the
discussion).  And I wanted to make sure your future patches are easier to
handle for me ;-).

Thanks.

[PATCH 1/2] Add 'git rev-parse --show-toplevel' option.

[Steven Drake]

 Shows the absolute path of the top-level working directory.
---
 Documentation/git-rev-parse.txt |    3 +++
 builtin-rev-parse.c             |    6 ++++++
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 82045a2..dc829b3 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -112,6 +112,9 @@ OPTIONS
 --remotes::
 	Show tag refs found in `$GIT_DIR/refs/remotes`.
 
+--show-toplevel::
+	Show the absolute path of the top-level directory.
+
 --show-prefix::
 	When the command is invoked from a subdirectory, show the
 	path of the current directory relative to the top-level
diff --git a/builtin-rev-parse.c b/builtin-rev-parse.c
index 37d0233..96ab8bb 100644
--- a/builtin-rev-parse.c
+++ b/builtin-rev-parse.c
@@ -581,6 +581,12 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				for_each_remote_ref(show_reference, NULL);
 				continue;
 			}
+			if (!strcmp(arg, "--show-toplevel")) {
+				const char *work_tree = get_git_work_tree();
+				if (work_tree)
+					printf("%s\n", work_tree);
+				continue;
+			}
 			if (!strcmp(arg, "--show-prefix")) {
 				if (prefix)
 					puts(prefix);
-- 
1.6.4

[PATCH 2/2] Use $(git rev-parse --show-toplevel) in cd_to_toplevel().

[Steven Drake]

rev-parse --show-toplevel gives the absolute (aka "physical") path of the 
toplevel directory and is more portable as 'cd -P' is not supported by all 
shell implementations.

This is also closer to what setup_work_tree() does.

Signed-off-by: Steven Drake <sdrake@xnet.co.nz>
---
 git-sh-setup.sh |   19 +++++--------------
 1 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index dfcb807..d56426d 100755
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -120,20 +120,11 @@ is_bare_repository () {
 }
 
 cd_to_toplevel () {
-	cdup=$(git rev-parse --show-cdup)
-	if test ! -z "$cdup"
-	then
-		# 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
+	cdup=$(git rev-parse --show-toplevel) &&
+	cd "$cdup" || {
+		echo >&2 "Cannot chdir to $cdup, the toplevel of the working tree"
+		exit 1
+	}
 }
 
 require_work_tree () {
-- 
1.6.4