{"thread":{"id":"22150","subject":"[PATCH 2/2] Use $(git rev-parse --show-toplevel) in cd_to_toplevel()","startedAt":"2010-01-10T07:19:21Z","lastAt":"2010-01-11T09:58:13Z","messageCount":7,"participants":["Steven Drake","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"131213","messageId":"alpine.LNX.2.00.1001102016380.2560@vqena.qenxr.bet.am","threadId":"22150","inReplyTo":null,"subject":"[PATCH 2/2] Use $(git rev-parse --show-toplevel) in cd_to_toplevel()","fromName":"Steven Drake","fromEmail":"sdrake@xnet.co.nz","sentAt":"2010-01-10T07:19:21Z","receivedAt":"2010-01-10T07:19:21Z","isPatch":true,"sender":{"key":"sdrake@xnet.co.nz","avatar":null},"body":"as it gives the absolute (aka \"physical\") path of the toplevel directory \nand 'cd -P' is not supported by all shell implementations.\n\nSee NetBSD PR/42168.\nhttp://www.netbsd.org/cgi-bin/query-pr-single.pl?number=42168\n---\n git-sh-setup.sh |   12 +++---------\n 1 files changed, 3 insertions(+), 9 deletions(-)\n\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex dfcb807..3cbec05 100755\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -120,16 +120,10 @@ is_bare_repository () {\n }\n \n cd_to_toplevel () {\n-\tcdup=$(git rev-parse --show-cdup)\n-\tif test ! -z \"$cdup\"\n+\tif test ! -z \"$(git rev-parse --show-cdup)\"\n \tthen\n-\t\t# The \"-P\" option says to follow \"physical\" directory\n-\t\t# structure instead of following symbolic links.  When cdup is\n-\t\t# \"../\", this means following the \"..\" entry in the current\n-\t\t# directory instead textually removing a symlink path element\n-\t\t# from the PWD shell variable.  The \"-P\" behavior is more\n-\t\t# consistent with the C-style chdir used by most of Git.\n-\t\tcd -P \"$cdup\" || {\n+\t\tcdup=$(git rev-parse --show-toplevel)\n+\t\tcd \"$cdup\" || {\n \t\t\techo >&2 \"Cannot chdir to $cdup, the toplevel of the working tree\"\n \t\t\texit 1\n \t\t}\n-- \n1.6.4\n"},{"id":"131215","messageId":"20100110101154.GA18138@progeny.tock","threadId":"22150","inReplyTo":"alpine.LNX.2.00.1001102016380.2560@vqena.qenxr.bet.am","subject":"Re: [PATCH 2/2] Use $(git rev-parse --show-toplevel) in cd_to_toplevel()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-01-10T10:11:54Z","receivedAt":"2010-01-10T10:11:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Steven Drake wrote:\n\n> 'cd -P' is not supported by all shell implementations.\n\nMakes sense.\n\ngit already does not handle those cases where 'cd \"$(pwd)\"' fails\nwhile 'cd .' does not, so your patch shouldn’t break anything. [1]\n\n> diff --git a/git-sh-setup.sh b/git-sh-setup.sh\n> index dfcb807..3cbec05 100755\n> --- a/git-sh-setup.sh\n> +++ b/git-sh-setup.sh\n> @@ -120,16 +120,10 @@ is_bare_repository () {\n>  }\n>  \n>  cd_to_toplevel () {\n> -\tcdup=$(git rev-parse --show-cdup)\n> -\tif test ! -z \"$cdup\"\n> +\tif test ! -z \"$(git rev-parse --show-cdup)\"\n>  \tthen\n> -\t\t# The \"-P\" option says to follow \"physical\" directory\n> -\t\t# structure instead of following symbolic links.  When cdup is\n> -\t\t# \"../\", this means following the \"..\" entry in the current\n> -\t\t# directory instead textually removing a symlink path element\n> -\t\t# from the PWD shell variable.  The \"-P\" behavior is more\n> -\t\t# consistent with the C-style chdir used by most of Git.\n> -\t\tcd -P \"$cdup\" || {\n> +\t\tcdup=$(git rev-parse --show-toplevel)\n> +\t\tcd \"$cdup\" || {\n\nWhy not 'cdup=$(git rev-parse --show-toplevel) && cd \"$cdup\"'\nunconditionally?  That would avoid having to look for the .git dir\ntwice and would mirror setup_work_tree() a bit more closely.\n\nAvoiding -P not only improves portability but makes the function\neasier to understand.  I like it.\n\nThanks,\nJonathan\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/135563/focus=135571\n"},{"id":"131260","messageId":"alpine.LNX.2.00.1001111354470.7728@vqena.qenxr.bet.am","threadId":"22150","inReplyTo":"20100110101154.GA18138@progeny.tock","subject":"Re: [PATCH 2/2] Use $(git rev-parse --show-toplevel) in cd_to_toplevel()","fromName":"Steven Drake","fromEmail":"sdrake@xnet.co.nz","sentAt":"2010-01-11T07:12:57Z","receivedAt":"2010-01-11T07:12:57Z","isPatch":true,"sender":{"key":"sdrake@xnet.co.nz","avatar":null},"body":"On Sun, 10 Jan 2010, Jonathan Nieder wrote:\n> Why not 'cdup=$(git rev-parse --show-toplevel) && cd \"$cdup\"'\n> unconditionally?  That would avoid having to look for the .git dir\n> twice and would mirror setup_work_tree() a bit more closely.\n\nDepends on what you think is the lesser of to evials, running \n'git rev-parse' twice or preforming a cd when theres no need to, but after\nlooking at setup_work_tree() I agree with you and will send in a new patch.\n\n-- \nSteven\n"},{"id":"131262","messageId":"alpine.LNX.2.00.1001112022190.8842@vqena.qenxr.bet.am","threadId":"22150","inReplyTo":"alpine.LNX.2.00.1001102016380.2560@vqena.qenxr.bet.am","subject":"[PATCH 2/2] Use $(git rev-parse --show-toplevel) in cd_to_toplevel()","fromName":"Steven Drake","fromEmail":"sdrake@xnet.co.nz","sentAt":"2010-01-11T07:23:20Z","receivedAt":"2010-01-11T07:23:20Z","isPatch":true,"sender":{"key":"sdrake@xnet.co.nz","avatar":null},"body":" as it gives the absolute (aka \"physical\") path of the toplevel directory\n and 'cd -P' is not supported by all shell implementations.\n\nSee NetBSD PR/42168.\nhttp://www.netbsd.org/cgi-bin/query-pr-single.pl?number=42168\n---\nThis is a revision of my original patch after the input from Jonathan.\n---\n git-sh-setup.sh |   18 ++++--------------\n 1 files changed, 4 insertions(+), 14 deletions(-)\n\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex dfcb807..4ecc578 100755\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -120,20 +120,10 @@ is_bare_repository () {\n }\n \n cd_to_toplevel () {\n-\tcdup=$(git rev-parse --show-cdup)\n-\tif test ! -z \"$cdup\"\n-\tthen\n-\t\t# The \"-P\" option says to follow \"physical\" directory\n-\t\t# structure instead of following symbolic links.  When cdup is\n-\t\t# \"../\", this means following the \"..\" entry in the current\n-\t\t# directory instead textually removing a symlink path element\n-\t\t# from the PWD shell variable.  The \"-P\" behavior is more\n-\t\t# consistent with the C-style chdir used by most of Git.\n-\t\tcd -P \"$cdup\" || {\n-\t\t\techo >&2 \"Cannot chdir to $cdup, the toplevel of the working tree\"\n-\t\t\texit 1\n-\t\t}\n-\tfi\n+\tcdup=$(git rev-parse --show-toplevel) && cd \"$cdup\" || {\n+\t\techo >&2 \"Cannot chdir to $cdup, the toplevel of the working tree\"\n+\t\texit 1\n+\t}\n }\n \n require_work_tree () {\n-- \n1.6.4\n"},{"id":"131264","messageId":"7viqb9w0c8.fsf@alter.siamese.dyndns.org","threadId":"22150","inReplyTo":"alpine.LNX.2.00.1001112022190.8842@vqena.qenxr.bet.am","subject":"Re: [PATCH 2/2] Use $(git rev-parse --show-toplevel) in cd_to_toplevel()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-11T07:46:15Z","receivedAt":"2010-01-11T07:46:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Steven Drake <sdrake@xnet.co.nz> writes:\n\n>  as it gives the absolute (aka \"physical\") path of the toplevel directory\n>  and 'cd -P' is not supported by all shell implementations.\n>\n> See NetBSD PR/42168.\n> http://www.netbsd.org/cgi-bin/query-pr-single.pl?number=42168\n> ---\n> This is a revision of my original patch after the input from Jonathan.\n> ---\n>  git-sh-setup.sh |   18 ++++--------------\n>  1 files changed, 4 insertions(+), 14 deletions(-)\n\nThe patch text may make sense (I haven't thought things through yet) but\nplease make sure your proposed commit log messages conform to the local\nconvention.\n\n (1) We don't chop a sentence in the middle of the subject line and\n     continue the same sentence to the first line of the body of the\n     message;\n\n (2) We don't indent the log message by one SP at the beginning of lines;\n\n (3) Please avoid referring to external resource in the commit log message\n     whenever makes sense; the log should be understandable on its own.\n     Because the first paragraph of your message describes the issue the\n     patch addresses very well already, you don't need \"See NetBSD...\" and\n     URL.  If you want to have them to help the reviewers, place such\n     reference after the three-dash line, just like you wrote \"This is a\n     revision...\"  You would help reviewers even more if you added a\n     pointer to your earlier patch after that sentence;\n\n (4) Sign your patch, before the three-dash line.\n\n> diff --git a/git-sh-setup.sh b/git-sh-setup.sh\n> index dfcb807..4ecc578 100755\n> --- a/git-sh-setup.sh\n> +++ b/git-sh-setup.sh\n> @@ -120,20 +120,10 @@ is_bare_repository () {\n>  }\n>  \n>  cd_to_toplevel () {\n> +\tcdup=$(git rev-parse --show-toplevel) && cd \"$cdup\" || {\n\nPlease line-break immediately after &&; it makes it easier to read in\ngeneral, and it would make \"cd\" stand out in this particular case, as it\nis the most important part of this particular function.\n\nThanks.\n"},{"id":"131275","messageId":"alpine.LNX.2.00.1001112114140.9352@vqena.qenxr.bet.am","threadId":"22150","inReplyTo":"7viqb9w0c8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] Use $(git rev-parse --show-toplevel) in cd_to_toplevel()","fromName":"Steven Drake","fromEmail":"sdrake@xnet.co.nz","sentAt":"2010-01-11T09:02:04Z","receivedAt":"2010-01-11T09:02:04Z","isPatch":true,"sender":{"key":"sdrake@xnet.co.nz","avatar":null},"body":"On Sun, 10 Jan 2010, Junio C Hamano wrote:\n\n>  (3) Please avoid referring to external resource in the commit log message\n>      whenever makes sense; the log should be understandable on its own.\n>      Because the first paragraph of your message describes the issue the\n>      patch addresses very well already, you don't need \"See NetBSD...\" and\n>      URL.  If you want to have them to help the reviewers, place such\n>      reference after the three-dash line, just like you wrote \"This is a\n>      revision...\"  You would help reviewers even more if you added a\n>      pointer to your earlier patch after that sentence;\n\nWondered wether I should have put the extra info after the three-dashes or\nnot, now I know.\n\nI also made sure my second email had References and In-Reply-To headers to \nmy first email.\n\n>  (4) Sign your patch, before the three-dash line.\n\nOpps forgot '--signoff', I've put format.signoff=ture in .gitconfig to solve\nthat problem. \n\nPerhaps a warning message from format-patch of the form:\nWARNING: You have not added a \"Signed-off-by:\" line did you mean to!\n\n> Please line-break immediately after &&; it makes it easier to read in\n> general, and it would make \"cd\" stand out in this particular case, as it\n> is the most important part of this particular function.\n\nGood point, althought did you mean as a general shell script coding rule or\njust in this particular case.\n\nThanks for the feedback.  Did you want me to resend the signed and cleaned up\npatches direct to you?  \n\n-- \nSteven\n"},{"id":"131279","messageId":"7vhbqtt13e.fsf@alter.siamese.dyndns.org","threadId":"22150","inReplyTo":"alpine.LNX.2.00.1001112114140.9352@vqena.qenxr.bet.am","subject":"Re: [PATCH 2/2] Use $(git rev-parse --show-toplevel) in cd_to_toplevel()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-11T09:58:13Z","receivedAt":"2010-01-11T09:58:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Steven Drake <sdrake@xnet.co.nz> writes:\n\n>>  (4) Sign your patch, before the three-dash line.\n>\n> Opps forgot '--signoff', I've put format.signoff=ture in .gitconfig to solve\n> that problem. \n>\n> Perhaps a warning message from format-patch of the form:\n> WARNING: You have not added a \"Signed-off-by:\" line did you mean to!\n\n1. It is usually a good idea to make it a habit of running \"commit -s\",\n   iow, record your sign-off at _commit time_, when working on a project\n   that uses the convention.  You may start contributing by sending a\n   pull-request instead of patches later.\n\n2. scripts/checkpatch.pl script in the Linux kernel project is a good tool\n   to check your patch before submission; you run it as:\n\n   $ perl checkpatch.pl --no-tree 0001-my-changes.patch\n   \n>> Please line-break immediately after &&; it makes it easier to read in\n>> general, and it would make \"cd\" stand out in this particular case, as it\n>> is the most important part of this particular function.\n>\n> Good point, althought did you mean as a general shell script coding rule or\n> just in this particular case.\n\n\"In general\" as a general style suggestion, and \"show 'cd' at the\nbeginning on its own line\" as a more reason to do so for this specific\ncase.\n\n> Thanks for the feedback.  Did you want me to resend the signed and cleaned up\n> patches direct to you?  \n\nOr to the list.\n\nI could actually have fixed them up and commit myself, instead of\nresponding with comments that can be seen as if I were nitpicking.\n\nThe reason why I chose not to was because I hoped you would keep\ncontributing to this project more in the future (I see you have already\nanother commit in our history, and you seem to be reasonably competent,\njudging from the patch description and the way you communicate in the\ndiscussion).  And I wanted to make sure your future patches are easier to\nhandle for me ;-).\n\nThanks.\n"}]}