Issue 6843044: code review 6843044: misc/git: add gofmt git pre-commit hook (Closed)
|
Created:
11 years, 5 months ago by adg Modified:
11 years, 5 months ago Reviewers:
CC:
golang-dev, bradfitz, ftrvxmtrx, fss, r, minux1 Visibility:
Public. |
Descriptionmisc/git: add gofmt git pre-commit hook
Patch Set 1 #
Total comments: 1
Patch Set 2 : diff -r f5c69f87ef34 https://code.google.com/p/go #
Total comments: 2
Patch Set 3 : diff -r f5c69f87ef34 https://code.google.com/p/go #
Total comments: 10
Patch Set 4 : diff -r cbb5a28097e9 https://code.google.com/p/go #
Total comments: 2
Patch Set 5 : diff -r cbb5a28097e9 https://code.google.com/p/go #
Total comments: 8
Patch Set 6 : diff -r cbb5a28097e9 https://code.google.com/p/go #
Total comments: 2
Patch Set 7 : diff -r cbb5a28097e9 https://code.google.com/p/go #Patch Set 8 : diff -r 99e41aa77582 https://code.google.com/p/go #MessagesTotal messages: 30
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
Sign in to reply to this message.
LGTM On Tue, Nov 13, 2012 at 5:38 AM, <adg@golang.org> wrote: > Reviewers: golang-dev_googlegroups.com, > > Message: > Hello golang-dev@googlegroups.com, > > I'd like you to review this change to > https://code.google.com/p/go > > > Description: > misc/git: add gofmt git pre-commit hook > > Please review this at http://codereview.appspot.com/**6843044/<http://codereview.appspot.com/6843044/> > > Affected files: > A misc/git/pre-commit > > > Index: misc/git/pre-commit > ==============================**==============================**======= > new file mode 100755 > --- /dev/null > +++ b/misc/git/pre-commit > @@ -0,0 +1,28 @@ > +#!/usr/bin/env bash > +# Copyright 2012 The Go Authors. All rights reserved. > +# Use of this source code is governed by a BSD-style > +# license that can be found in the LICENSE file. > + > +# git gofmt pre-commit hook > +# > +# To use, store as .git/hooks/pre-commit inside you repository and make > sure > +# it has execute permissions (chmod +x pre-commit). > + > +gofiles="$(git diff --cached --name-only --diff-filter=ACM | grep '.go$')" > +[ "$gofiles" == "" ] && exit 0 > + > +unformatted="$(gofmt -l $gofiles)" > +[ "$unformatted" == "" ] && exit 0 > + > +# Some files are not gofmt'd. Print message and fail. > + > +echo "Go files must be formatted with gofmt. Please run:" >&2 > +echo -n " gofmt -w" >&2 > +dir="$(pwd)" > +for fn in $unformatted; do > + echo " \\" >&2 > + echo -n " " ${dir}/${fn} >&2 > +done > +echo >&2 > + > +exit 1 > > >
Sign in to reply to this message.
I'm going to point to this from a blog post, so picky comments about my bash skills are welcome. Particularly anything that will make the script more portable. On 13 November 2012 15:58, Brad Fitzpatrick <bradfitz@golang.org> wrote: > LGTM > > > On Tue, Nov 13, 2012 at 5:38 AM, <adg@golang.org> wrote: > >> Reviewers: golang-dev_googlegroups.com, >> >> Message: >> Hello golang-dev@googlegroups.com, >> >> I'd like you to review this change to >> https://code.google.com/p/go >> >> >> Description: >> misc/git: add gofmt git pre-commit hook >> >> Please review this at http://codereview.appspot.com/**6843044/<http://codereview.appspot.com/6843044/> >> >> Affected files: >> A misc/git/pre-commit >> >> >> Index: misc/git/pre-commit >> ==============================**==============================**======= >> new file mode 100755 >> --- /dev/null >> +++ b/misc/git/pre-commit >> @@ -0,0 +1,28 @@ >> +#!/usr/bin/env bash >> +# Copyright 2012 The Go Authors. All rights reserved. >> +# Use of this source code is governed by a BSD-style >> +# license that can be found in the LICENSE file. >> + >> +# git gofmt pre-commit hook >> +# >> +# To use, store as .git/hooks/pre-commit inside you repository and make >> sure >> +# it has execute permissions (chmod +x pre-commit). >> + >> +gofiles="$(git diff --cached --name-only --diff-filter=ACM | grep >> '.go$')" >> +[ "$gofiles" == "" ] && exit 0 >> + >> +unformatted="$(gofmt -l $gofiles)" >> +[ "$unformatted" == "" ] && exit 0 >> + >> +# Some files are not gofmt'd. Print message and fail. >> + >> +echo "Go files must be formatted with gofmt. Please run:" >&2 >> +echo -n " gofmt -w" >&2 >> +dir="$(pwd)" >> +for fn in $unformatted; do >> + echo " \\" >&2 >> + echo -n " " ${dir}/${fn} >&2 >> +done >> +echo >&2 >> + >> +exit 1 >> >> >> >
Sign in to reply to this message.
echo -n is not portable, printf is better. Also, I think "/usr/bin/env bash" could be replaced with just "/bin/sh", as there seem to be no bashisms anyway. On 2012/11/13 15:11:50, adg wrote: > I'm going to point to this from a blog post, so picky comments about my > bash skills are welcome. Particularly anything that will make the script > more portable. > > > On 13 November 2012 15:58, Brad Fitzpatrick <mailto:bradfitz@golang.org> wrote: > > > LGTM > > > > > > On Tue, Nov 13, 2012 at 5:38 AM, <mailto:adg@golang.org> wrote: > > > >> Reviewers: http://golang-dev_googlegroups.com, > >> > >> Message: > >> Hello mailto:golang-dev@googlegroups.com, > >> > >> I'd like you to review this change to > >> https://code.google.com/p/go > >> > >> > >> Description: > >> misc/git: add gofmt git pre-commit hook > >> > >> Please review this at > http://codereview.appspot.com/**6843044/%3Chttp://codereview.appspot.com/6843...> > >> > >> Affected files: > >> A misc/git/pre-commit > >> > >> > >> Index: misc/git/pre-commit > >> ==============================**==============================**======= > >> new file mode 100755 > >> --- /dev/null > >> +++ b/misc/git/pre-commit > >> @@ -0,0 +1,28 @@ > >> +#!/usr/bin/env bash > >> +# Copyright 2012 The Go Authors. All rights reserved. > >> +# Use of this source code is governed by a BSD-style > >> +# license that can be found in the LICENSE file. > >> + > >> +# git gofmt pre-commit hook > >> +# > >> +# To use, store as .git/hooks/pre-commit inside you repository and make > >> sure > >> +# it has execute permissions (chmod +x pre-commit). > >> + > >> +gofiles="$(git diff --cached --name-only --diff-filter=ACM | grep > >> '.go$')" > >> +[ "$gofiles" == "" ] && exit 0 > >> + > >> +unformatted="$(gofmt -l $gofiles)" > >> +[ "$unformatted" == "" ] && exit 0 > >> + > >> +# Some files are not gofmt'd. Print message and fail. > >> + > >> +echo "Go files must be formatted with gofmt. Please run:" >&2 > >> +echo -n " gofmt -w" >&2 > >> +dir="$(pwd)" > >> +for fn in $unformatted; do > >> + echo " \\" >&2 > >> + echo -n " " ${dir}/${fn} >&2 > >> +done > >> +echo >&2 > >> + > >> +exit 1 > >> > >> > >> > >
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, bradfitz@golang.org, ftrvxmtrx@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
http://codereview.appspot.com/6843044/diff/1/misc/git/pre-commit File misc/git/pre-commit (right): http://codereview.appspot.com/6843044/diff/1/misc/git/pre-commit#newcode21 misc/git/pre-commit:21: dir="$(pwd)" You can drop this variable and use the ${PWD} environment variable instead.
Sign in to reply to this message.
http://codereview.appspot.com/6843044/diff/2002/misc/git/pre-commit File misc/git/pre-commit (right): http://codereview.appspot.com/6843044/diff/2002/misc/git/pre-commit#newcode12 misc/git/pre-commit:12: [ "$gofiles" == "" ] && exit 0 Either s/==/=/ or [ -z "$gofiles" ] http://codereview.appspot.com/6843044/diff/2002/misc/git/pre-commit#newcode15 misc/git/pre-commit:15: [ "$unformatted" == "" ] && exit 0 See above.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, bradfitz@golang.org, ftrvxmtrx@gmail.com, franciscossouza@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
http://codereview.appspot.com/6843044/diff/2004/misc/git/pre-commit File misc/git/pre-commit (right): http://codereview.appspot.com/6843044/diff/2004/misc/git/pre-commit#newcode21 misc/git/pre-commit:21: for fn in $unformatted; do echo "$unformatted" | while read fn; do Or else it'll break on paths with spaces.
Sign in to reply to this message.
https://codereview.appspot.com/6843044/diff/2004/misc/git/pre-commit File misc/git/pre-commit (right): https://codereview.appspot.com/6843044/diff/2004/misc/git/pre-commit#newcode8 misc/git/pre-commit:8: # To use, store as .git/hooks/pre-commit inside you repository and make sure s/you/your/ https://codereview.appspot.com/6843044/diff/2004/misc/git/pre-commit#newcode9 misc/git/pre-commit:9: # it has execute permissions (chmod +x pre-commit). delete the parenthetical clause. if you don't know how to do this you shouldn't be playing with these tools https://codereview.appspot.com/6843044/diff/2004/misc/git/pre-commit#newcode11 misc/git/pre-commit:11: gofiles="$(git diff --cached --name-only --diff-filter=ACM | grep '.go$')" quotes unnecessary on $() (several instances) https://codereview.appspot.com/6843044/diff/2004/misc/git/pre-commit#newcode19 misc/git/pre-commit:19: printf "Go files must be formatted with gofmt. Please run:\n" >&2 printf? this isn't C. use echo >&2 (and put the >&2 next to the echo, not at the end of the line, so it's easier to see)
Sign in to reply to this message.
I propose this version to solve the space in filename problem:
if [ "x$1" == "x--files" ]; then
files=""
while shift; do
[ -z "$1" ] && continue
r="`gofmt -l "$1"`"
if [ ! -z "$r" ]; then
files="$files \\
'${PWD}/$1'"
fi;
done
if [ -z "$files" ]; then
exit 0
else
echo "please ... $files"
exit 1;
fi
else
git diff --cached --name-only -z --diff-filter=ACM | xargs -0 "$0" --files
exit $?
fi
Sign in to reply to this message.
On Tue, Nov 13, 2012 at 3:35 PM, minux <minux.ma@gmail.com> wrote: > I propose this version to solve the space in filename problem: I'd propose to not solve this problem. -- Francisco Souza
Sign in to reply to this message.
On 2012/11/13 17:42:22, fss wrote: > On Tue, Nov 13, 2012 at 3:35 PM, minux <mailto:minux.ma@gmail.com> wrote: > > I propose this version to solve the space in filename problem: > > I'd propose to not solve this problem. > > > -- > Francisco Souza Agree, especially considering both complexity of a fix and chance to have spaces in path.
Sign in to reply to this message.
On Wed, Nov 14, 2012 at 1:42 AM, Francisco Souza <franciscossouza@gmail.com>wrote: > On Tue, Nov 13, 2012 at 3:35 PM, minux <minux.ma@gmail.com> wrote: > > I propose this version to solve the space in filename problem: > I'd propose to not solve this problem. > I think the easiest hook is just something like this: r=`git diff --cached --name-only -z --diff-filter=ACM | xargs -0 gofmt -l -w` if [ -z "$r" ]; then exit 0 else echo "Gofmt'ed, please inspect and re-add these files: $r" exit 1 fi one remaining problem is that this file changes the working tree directly. a deeper problem is that, we can't assume that files in the working tree is the same as files in the index. I'd rather choose not to handle that case.
Sign in to reply to this message.
I'd prefer not totally rewrite this. I like that it: a) provides a command line to fix the problem b) doesn't modify your files automatically https://codereview.appspot.com/6843044/diff/2004/misc/git/pre-commit File misc/git/pre-commit (right): https://codereview.appspot.com/6843044/diff/2004/misc/git/pre-commit#newcode8 misc/git/pre-commit:8: # To use, store as .git/hooks/pre-commit inside you repository and make sure On 2012/11/13 17:14:49, r wrote: > s/you/your/ Done. https://codereview.appspot.com/6843044/diff/2004/misc/git/pre-commit#newcode9 misc/git/pre-commit:9: # it has execute permissions (chmod +x pre-commit). On 2012/11/13 17:14:49, r wrote: > delete the parenthetical clause. if you don't know how to do this you shouldn't > be playing with these tools Done. https://codereview.appspot.com/6843044/diff/2004/misc/git/pre-commit#newcode11 misc/git/pre-commit:11: gofiles="$(git diff --cached --name-only --diff-filter=ACM | grep '.go$')" On 2012/11/13 17:14:49, r wrote: > quotes unnecessary on $() (several instances) Done. https://codereview.appspot.com/6843044/diff/2004/misc/git/pre-commit#newcode19 misc/git/pre-commit:19: printf "Go files must be formatted with gofmt. Please run:\n" >&2 On 2012/11/13 17:14:49, r wrote: > printf? this isn't C. I'm using printf because 'echo -n' isn't portable. > use echo >&2 > (and put the >&2 next to the echo, not at the end of the line, so it's easier to > see) Done. https://codereview.appspot.com/6843044/diff/2004/misc/git/pre-commit#newcode21 misc/git/pre-commit:21: for fn in $unformatted; do On 2012/11/13 15:55:03, ftrvxmtrx wrote: > echo "$unformatted" | while read fn; do > > Or else it'll break on paths with spaces. I think it's already broken for paths with spaces. I'm happy to ignore that problem.
Sign in to reply to this message.
Hello golang-dev@googlegroups.com, bradfitz@golang.org, ftrvxmtrx@gmail.com, franciscossouza@gmail.com, r@golang.org, minux.ma@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
http://codereview.appspot.com/6843044/diff/11001/misc/git/pre-commit File misc/git/pre-commit (right): http://codereview.appspot.com/6843044/diff/11001/misc/git/pre-commit#newcode20 misc/git/pre-commit:20: printf >&2 " gofmt -w" >&2 two times >&2?
Sign in to reply to this message.
https://codereview.appspot.com/6843044/diff/11001/misc/git/pre-commit File misc/git/pre-commit (right): https://codereview.appspot.com/6843044/diff/11001/misc/git/pre-commit#newcode20 misc/git/pre-commit:20: printf >&2 " gofmt -w" >&2 On 2012/11/14 09:56:50, ftrvxmtrx wrote: > two times >&2? Fixed.
Sign in to reply to this message.
LGTM On 2012/11/14 09:58:38, adg wrote: > https://codereview.appspot.com/6843044/diff/11001/misc/git/pre-commit > File misc/git/pre-commit (right): > > https://codereview.appspot.com/6843044/diff/11001/misc/git/pre-commit#newcode20 > misc/git/pre-commit:20: printf >&2 " gofmt -w" >&2 > On 2012/11/14 09:56:50, ftrvxmtrx wrote: > > two times >&2? > > Fixed.
Sign in to reply to this message.
https://codereview.appspot.com/6843044/diff/1005/misc/git/pre-commit File misc/git/pre-commit (right): https://codereview.appspot.com/6843044/diff/1005/misc/git/pre-commit#newcode9 misc/git/pre-commit:9: # it has execute permissions. do you want to document that it doesn't handle spaces in filenames? also, it can only handle the case where files staged to be committed (in the index) is the same as the corresponding file in the working copy. https://codereview.appspot.com/6843044/diff/1005/misc/git/pre-commit#newcode14 misc/git/pre-commit:14: unformatted=$(gofmt -l $gofiles) do you want to ignore messages on stderr? gofmt will display some confusing messages when some file is not found (due to spaces in the filename) https://codereview.appspot.com/6843044/diff/1005/misc/git/pre-commit#newcode19 misc/git/pre-commit:19: echo >&2 "Go files must be formatted with gofmt. Please run:\n" echo with \n? https://codereview.appspot.com/6843044/diff/1005/misc/git/pre-commit#newcode23 misc/git/pre-commit:23: printf >&2 " %s/%s" "${PWD}" "${fn}" why {}? "$PWD" "$fn" is ok.
Sign in to reply to this message.
https://codereview.appspot.com/6843044/diff/1005/misc/git/pre-commit File misc/git/pre-commit (right): https://codereview.appspot.com/6843044/diff/1005/misc/git/pre-commit#newcode9 misc/git/pre-commit:9: # it has execute permissions. On 2012/11/14 10:34:09, minux wrote: > do you want to document that it doesn't handle spaces in > filenames? Documented. > also, it can only handle the case where files staged to > be committed (in the index) is the same as the corresponding > file in the working copy. I'm ok with that. https://codereview.appspot.com/6843044/diff/1005/misc/git/pre-commit#newcode14 misc/git/pre-commit:14: unformatted=$(gofmt -l $gofiles) On 2012/11/14 10:34:09, minux wrote: > do you want to ignore messages on stderr? > gofmt will display some confusing messages when some > file is not found (due to spaces in the filename) I'd prefer not to hide the messages. At least then the user has a hope of figuring out what went wrong. https://codereview.appspot.com/6843044/diff/1005/misc/git/pre-commit#newcode19 misc/git/pre-commit:19: echo >&2 "Go files must be formatted with gofmt. Please run:\n" On 2012/11/14 10:34:09, minux wrote: > echo with \n? Done. https://codereview.appspot.com/6843044/diff/1005/misc/git/pre-commit#newcode23 misc/git/pre-commit:23: printf >&2 " %s/%s" "${PWD}" "${fn}" On 2012/11/14 10:34:09, minux wrote: > why {}? "$PWD" "$fn" is ok. Done.
Sign in to reply to this message.
LGTM On 2012/11/14 10:39:53, adg wrote: > https://codereview.appspot.com/6843044/diff/1005/misc/git/pre-commit > File misc/git/pre-commit (right): > > https://codereview.appspot.com/6843044/diff/1005/misc/git/pre-commit#newcode9 > misc/git/pre-commit:9: # it has execute permissions. > On 2012/11/14 10:34:09, minux wrote: > > do you want to document that it doesn't handle spaces in > > filenames? > > Documented. > > > also, it can only handle the case where files staged to > > be committed (in the index) is the same as the corresponding > > file in the working copy. > > I'm ok with that. > > https://codereview.appspot.com/6843044/diff/1005/misc/git/pre-commit#newcode14 > misc/git/pre-commit:14: unformatted=$(gofmt -l $gofiles) > On 2012/11/14 10:34:09, minux wrote: > > do you want to ignore messages on stderr? > > gofmt will display some confusing messages when some > > file is not found (due to spaces in the filename) > > I'd prefer not to hide the messages. At least then the user has a hope of > figuring out what went wrong. > > https://codereview.appspot.com/6843044/diff/1005/misc/git/pre-commit#newcode19 > misc/git/pre-commit:19: echo >&2 "Go files must be formatted with gofmt. Please > run:\n" > On 2012/11/14 10:34:09, minux wrote: > > echo with \n? > > Done. > > https://codereview.appspot.com/6843044/diff/1005/misc/git/pre-commit#newcode23 > misc/git/pre-commit:23: printf >&2 " %s/%s" "${PWD}" "${fn}" > On 2012/11/14 10:34:09, minux wrote: > > why {}? "$PWD" "$fn" is ok. > > Done.
Sign in to reply to this message.
LGTM.
Sign in to reply to this message.
Tell me more about echo -n not being portable for this purpose. I'd like to understand why a day one Unix command isn't allowed to print a simple string, but a Johnny-come-lately I've never used is the one everyone knows is better. -rob
Sign in to reply to this message.
On Wed, Nov 14, 2012 at 12:35 PM, Rob Pike <r@golang.org> wrote: > Tell me more about echo -n not being portable for this purpose. I'd > like to understand why a day one Unix command isn't allowed to print a > simple string, but a Johnny-come-lately I've never used is the one > everyone knows is better. % sh sh-3.2$ echo -n abc -n abc sh-3.2$ /bin/echo -n abc abcsh-3.2$
Sign in to reply to this message.
I am surprised to learn (again) that 'sh' today means the PWB shell and its sequelae. I keep hoping we'd left that behind long ago. Shocking to think that Unix 7th edition is too modern for today's computers. I give up. -rob
Sign in to reply to this message.
http://codereview.appspot.com/6843044/diff/12002/misc/git/pre-commit File misc/git/pre-commit (right): http://codereview.appspot.com/6843044/diff/12002/misc/git/pre-commit#newcode27 misc/git/pre-commit:27: echo >&2 by the time you've put the backslash in, it's multiline anyway. and the backslashes are scary. why not just for fn in $unformatted; do echo "gofmt -w $PWD/$fn" done you're overthinking it
Sign in to reply to this message.
https://codereview.appspot.com/6843044/diff/12002/misc/git/pre-commit File misc/git/pre-commit (right): https://codereview.appspot.com/6843044/diff/12002/misc/git/pre-commit#newcode27 misc/git/pre-commit:27: echo >&2 On 2012/11/14 15:08:50, r wrote: > by the time you've put the backslash in, it's multiline anyway. and the > backslashes are scary. > > why not just > for fn in $unformatted; do > echo "gofmt -w $PWD/$fn" > done > > you're overthinking it Done.
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=68613f8e57b8 *** misc/git: add gofmt git pre-commit hook R=golang-dev, bradfitz, ftrvxmtrx, franciscossouza, r, minux.ma CC=golang-dev http://codereview.appspot.com/6843044
Sign in to reply to this message.
|
