FAQ
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/

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

Search Discussions

  • Brad Fitzpatrick at Nov 13, 2012 at 2:58 pm
    LGTM
    On Tue, Nov 13, 2012 at 5:38 AM, 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

  • Andrew Gerrand at Nov 13, 2012 at 3:11 pm
    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 wrote:

    LGTM

    On Tue, Nov 13, 2012 at 5:38 AM, 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

  • Adg at Nov 13, 2012 at 3:29 pm
    Hello golang-dev@googlegroups.com, bradfitz@golang.org,
    ftrvxmtrx@gmail.com (cc: golang-dev@googlegroups.com),

    Please take another look.


    http://codereview.appspot.com/6843044/
  • Franciscossouza at Nov 13, 2012 at 3:32 pm
    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.

    http://codereview.appspot.com/6843044/
  • Adg at Nov 13, 2012 at 3:45 pm
    Hello golang-dev@googlegroups.com, bradfitz@golang.org,
    ftrvxmtrx@gmail.com, franciscossouza@gmail.com (cc:
    golang-dev@googlegroups.com),

    Please take another look.


    http://codereview.appspot.com/6843044/
  • R at Nov 13, 2012 at 5:14 pm
    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)

    https://codereview.appspot.com/6843044/
  • Minux at Nov 13, 2012 at 5:35 pm
    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
  • Francisco Souza at Nov 13, 2012 at 5:42 pm

    On Tue, Nov 13, 2012 at 3:35 PM, minux wrote:
    I propose this version to solve the space in filename problem:
    I'd propose to not solve this problem.


    --
    Francisco Souza
  • Minux at Nov 13, 2012 at 6:20 pm

    On Wed, Nov 14, 2012 at 1:42 AM, Francisco Souza wrote:
    On Tue, Nov 13, 2012 at 3:35 PM, minux 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.
  • Ftrvxmtrx at Nov 13, 2012 at 8:37 pm

    On 2012/11/13 17:42:22, fss wrote:
    On Tue, Nov 13, 2012 at 3:35 PM, minux
    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.

    https://codereview.appspot.com/6843044/
  • Ftrvxmtrx at Nov 13, 2012 at 8:37 pm
    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.

    http://codereview.appspot.com/6843044/
  • Ftrvxmtrx at Nov 13, 2012 at 8:37 pm
    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
    wrote:
    LGTM

    On Tue, Nov 13, 2012 at 5:38 AM, 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/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



    http://codereview.appspot.com/6843044/
  • Ftrvxmtrx at Nov 13, 2012 at 8:37 pm
    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.

    http://codereview.appspot.com/6843044/
  • Adg at Nov 14, 2012 at 9:51 am
    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.

    https://codereview.appspot.com/6843044/
  • Adg at Nov 14, 2012 at 9:51 am
    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.


    http://codereview.appspot.com/6843044/
  • Adg at Nov 14, 2012 at 9:58 am
    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.

    https://codereview.appspot.com/6843044/
  • Minux Ma at Nov 14, 2012 at 10:34 am
    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.

    https://codereview.appspot.com/6843044/
  • Adg at Nov 14, 2012 at 10:39 am
    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.

    https://codereview.appspot.com/6843044/
  • Franciscossouza at Nov 14, 2012 at 10:52 am
    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.
    https://codereview.appspot.com/6843044/
  • Minux Ma at Nov 14, 2012 at 10:54 am
  • Rob Pike at Nov 14, 2012 at 2:35 pm
    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
  • Francisco Souza at Nov 14, 2012 at 2:47 pm

    On Wed, Nov 14, 2012 at 12:35 PM, Rob Pike 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$
  • Rob Pike at Nov 14, 2012 at 3:04 pm
    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
  • R at Nov 14, 2012 at 3:08 pm
    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

    http://codereview.appspot.com/6843044/
  • Ftrvxmtrx at Nov 14, 2012 at 4:40 pm
    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.


    http://codereview.appspot.com/6843044/
  • Ftrvxmtrx at Nov 14, 2012 at 4:40 pm
  • Adg at Nov 15, 2012 at 4:48 pm
    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.

    https://codereview.appspot.com/6843044/
  • R at Nov 15, 2012 at 6:24 pm
  • Adg at Nov 15, 2012 at 6:59 pm
    *** 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


    http://codereview.appspot.com/6843044/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedNov 13, '12 at 1:38p
activeNov 15, '12 at 6:59p
posts30
users6
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase