Steven Drake [off-list ref] 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.
quoted hunk
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.
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
Steven Drake [off-list ref] writes:
quoted
(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
quoted
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.