[PATCH 0/4] Janitorial work on hook templates

[PATCH 0/4] Janitorial work on hook templates

[Richard Hartmann]

Dear all,

attached, you will find a series of small, obvious, and hopefully
uncontroversial patches for the hook templates and the manpage.
They are all tiny, but I still decided to submit distinct patches
to make modification/discussion easier.

Richard Hartmann (6):
  templates: Fewer subprocesses in pre-commit hook
  templates: Reformat pre-commit hook's message
  templates: Fix spelling in pre-commit hook
  Documentation: Update manpage for pre-commit hook
  templates: Fix ASCII art in pre-rebase hook
  template: Fix comment indentation in pre-rebase hook

 Documentation/githooks.txt         |    3 ++-
 templates/hooks--pre-commit.sample |   26 ++++++++++++--------------
 templates/hooks--pre-rebase.sample |   26 +++++++++++++-------------
 3 files changed, 27 insertions(+), 28 deletions(-)

-- 
1.7.10.4

[PATCH 1/6] templates: Fewer subprocesses in pre-commit hook

[Richard Hartmann]

Spawning a new subprocess for every line printed is inefficient.
Thus spawn only one instance of `echo`.

Signed-off-by: Richard Hartmann <richih.mailinglist@gmail.com>
---
 templates/hooks--pre-commit.sample |   24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/templates/hooks--pre-commit.sample b/templates/hooks--pre-commit.sample
index 18c4829..126ae13 100755
--- a/templates/hooks--pre-commit.sample
+++ b/templates/hooks--pre-commit.sample
@@ -31,18 +31,18 @@ if [ "$allownonascii" != "true" ] &&
 	test $(git diff --cached --name-only --diff-filter=A -z $against |
 	  LC_ALL=C tr -d '[ -~]\0' | wc -c) != 0
 then
-	echo "Error: Attempt to add a non-ascii file name."
-	echo
-	echo "This can cause problems if you want to work"
-	echo "with people on other platforms."
-	echo
-	echo "To be portable it is advisable to rename the file ..."
-	echo
-	echo "If you know what you are doing you can disable this"
-	echo "check using:"
-	echo
-	echo "  git config hooks.allownonascii true"
-	echo
+	echo 'Error: Attempt to add a non-ascii file name.
+
+This can cause problems if you want to work
+with people on other platforms.
+
+To be portable it is advisable to rename the file.
+
+If you know what you are doing you can disable this
+check using:
+
+  git config hooks.allownonascii true
+'
 	exit 1
 fi
 
-- 
1.7.10.4

Re: [PATCH 1/6] templates: Fewer subprocesses in pre-commit hook

[Junio C Hamano]

Richard Hartmann <richih.mailinglist@gmail.com> writes:

> Spawning a new subprocess for every line printed is inefficient.
> Thus spawn only one instance of `echo`.
>
> Signed-off-by: Richard Hartmann <richih.mailinglist@gmail.com>
> ---
>  templates/hooks--pre-commit.sample |   24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/templates/hooks--pre-commit.sample b/templates/hooks--pre-commit.sample
> index 18c4829..126ae13 100755
> --- a/templates/hooks--pre-commit.sample
> +++ b/templates/hooks--pre-commit.sample
> @@ -31,18 +31,18 @@ if [ "$allownonascii" != "true" ] &&
>  	test $(git diff --cached --name-only --diff-filter=A -z $against |
>  	  LC_ALL=C tr -d '[ -~]\0' | wc -c) != 0
>  then
> -	echo "Error: Attempt to add a non-ascii file name."
> -	echo
> -	echo "This can cause problems if you want to work"
> -	echo "with people on other platforms."
> -	echo
> -	echo "To be portable it is advisable to rename the file ..."
> -	echo
> -	echo "If you know what you are doing you can disable this"
> -	echo "check using:"
> -	echo
> -	echo "  git config hooks.allownonascii true"
> -	echo
> +	echo 'Error: Attempt to add a non-ascii file name.
> +
> +This can cause problems if you want to work
> +with people on other platforms.
> +
> +To be portable it is advisable to rename the file.
> +
> +If you know what you are doing you can disable this
> +check using:
> +
> +  git config hooks.allownonascii true
> +'
>  	exit 1
>  fi

Thanks.
Writing it as a single here-text

	cat <<-EOF
        Error: Attempt to...

        the message body that is
        multi-line
        EOF

might make it easier for people who want to activate and customize
the message, but honestly this is a borderline "Meh" at least to me.

Will take a look at other patches first before further commenting on
this.

Re: [PATCH 1/6] templates: Fewer subprocesses in pre-commit hook

[Richard Hartmann]

Hi Junio,

if you want a new patch, just say the word.


Richard

Re: [PATCH 1/6] templates: Fewer subprocesses in pre-commit hook

[Jeff King]

On Mon, Jun 10, 2013 at 08:36:00PM +0200, Richard Hartmann wrote:

> Spawning a new subprocess for every line printed is inefficient.
> Thus spawn only one instance of `echo`.

Most modern shells have "echo" as a built-in these days, and do not fork
at all to run it. E.g., try "strace sh -c 'echo foo'" with your shell of
choice; neither dash nor bash will fork at all.

IMHO the indentation issues make the end result of your patch less
readable (and here-doc with cat is more readable, but actually
_increases_ the number of processes, since cat is not usually a
built-in). So I'd be in favor of keeping it as-is.

-Peff

[PATCH 2/6] templates: Reformat pre-commit hook's message

[Richard Hartmann]

Now that the there's only one echo being spawned, the message can span
the full 80 chars.

Signed-off-by: Richard Hartmann <richih.mailinglist@gmail.com>
---
 templates/hooks--pre-commit.sample |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/templates/hooks--pre-commit.sample b/templates/hooks--pre-commit.sample
index 126ae13..7676c6e 100755
--- a/templates/hooks--pre-commit.sample
+++ b/templates/hooks--pre-commit.sample
@@ -33,13 +33,11 @@ if [ "$allownonascii" != "true" ] &&
 then
 	echo 'Error: Attempt to add a non-ascii file name.
 
-This can cause problems if you want to work
-with people on other platforms.
+This can cause problems if you want to work with people on other platforms.
 
 To be portable it is advisable to rename the file.
 
-If you know what you are doing you can disable this
-check using:
+If you know what you are doing you can disable this check using:
 
   git config hooks.allownonascii true
 '
-- 
1.7.10.4

Re: [PATCH 2/6] templates: Reformat pre-commit hook's message

[Junio C Hamano]

Richard Hartmann <richih.mailinglist@gmail.com> writes:

> Now that the there's only one echo being spawned, the message can span
> the full 80 chars.
>
> Signed-off-by: Richard Hartmann <richih.mailinglist@gmail.com>
> ---
>  templates/hooks--pre-commit.sample |    6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/templates/hooks--pre-commit.sample b/templates/hooks--pre-commit.sample
> index 126ae13..7676c6e 100755
> --- a/templates/hooks--pre-commit.sample
> +++ b/templates/hooks--pre-commit.sample
> @@ -33,13 +33,11 @@ if [ "$allownonascii" != "true" ] &&
>  then
>  	echo 'Error: Attempt to add a non-ascii file name.
>  
> -This can cause problems if you want to work
> -with people on other platforms.
> +This can cause problems if you want to work with people on other platforms.
>  
>  To be portable it is advisable to rename the file.
>  
> -If you know what you are doing you can disable this
> -check using:
> +If you know what you are doing you can disable this check using:
>  
>    git config hooks.allownonascii true
>  '

OK.  Occupying 75-col feels like it is pushing a bit, but the result
does look more readable.

[PATCH 3/6] templates: Fix spelling in pre-commit hook

[Richard Hartmann]

Signed-off-by: Richard Hartmann <richih.mailinglist@gmail.com>
---
 templates/hooks--pre-commit.sample |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/templates/hooks--pre-commit.sample b/templates/hooks--pre-commit.sample
index 7676c6e..a982d99 100755
--- a/templates/hooks--pre-commit.sample
+++ b/templates/hooks--pre-commit.sample
@@ -15,13 +15,13 @@ else
 	against=4b825dc642cb6eb9a060e54bf8d69288fbee4904
 fi
 
-# If you want to allow non-ascii filenames set this variable to true.
+# If you want to allow non-ASCII filenames set this variable to true.
 allownonascii=$(git config hooks.allownonascii)
 
 # Redirect output to stderr.
 exec 1>&2
 
-# Cross platform projects tend to avoid non-ascii filenames; prevent
+# Cross platform projects tend to avoid non-ASCII filenames; prevent
 # them from being added to the repository. We exploit the fact that the
 # printable range starts at the space character and ends with tilde.
 if [ "$allownonascii" != "true" ] &&
@@ -31,7 +31,7 @@ if [ "$allownonascii" != "true" ] &&
 	test $(git diff --cached --name-only --diff-filter=A -z $against |
 	  LC_ALL=C tr -d '[ -~]\0' | wc -c) != 0
 then
-	echo 'Error: Attempt to add a non-ascii file name.
+	echo 'Error: Attempt to add a non-ASCII file name.
 
 This can cause problems if you want to work with people on other platforms.
 
-- 
1.7.10.4

[PATCH 4/6] Documentation: Update manpage for pre-commit hook

[Richard Hartmann]

Signed-off-by: Richard Hartmann <richih.mailinglist@gmail.com>
---
 Documentation/githooks.txt |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index b9003fe..1276730 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -80,7 +80,8 @@ causes the 'git commit' to abort.
 
 The default 'pre-commit' hook, when enabled, catches introduction
 of lines with trailing whitespaces and aborts the commit when
-such a line is found.
+such a line is found. It will also prevent addition of non-ASCII
+file names.
 
 All the 'git commit' hooks are invoked with the environment
 variable `GIT_EDITOR=:` if the command will not bring up an editor
-- 
1.7.10.4

[PATCH 5/6] templates: Fix ASCII art in pre-rebase hook

[Richard Hartmann]

The example assumes 8-char wide tabs and breaks for people with
4-char wide tabs.

Signed-off-by: Richard Hartmann <richih.mailinglist@gmail.com>
---
 templates/hooks--pre-rebase.sample |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/templates/hooks--pre-rebase.sample b/templates/hooks--pre-rebase.sample
index 053f111..b74cd1d 100755
--- a/templates/hooks--pre-rebase.sample
+++ b/templates/hooks--pre-rebase.sample
@@ -132,14 +132,14 @@ With this workflow, you would want to know:
 
 Let's look at this example:
 
-		   o---o---o---o---o---o---o---o---o---o "next"
-		  /       /           /           /
-		 /   a---a---b A     /           /
-		/   /               /           /
-	       /   /   c---c---c---c B         /
-	      /   /   /             \         /
-	     /   /   /   b---b C     \       /
-	    /   /   /   /             \     /
+                   o---o---o---o---o---o---o---o---o---o "next"
+                  /       /           /           /
+                 /   a---a---b A     /           /
+                /   /               /           /
+               /   /   c---c---c---c B         /
+              /   /   /             \         /
+             /   /   /   b---b C     \       /
+            /   /   /   /             \     /
     ---o---o---o---o---o---o---o---o---o---o---o "master"
 
 
-- 
1.7.10.4

Re: [PATCH 5/6] templates: Fix ASCII art in pre-rebase hook

[Junio C Hamano]

Richard Hartmann <richih.mailinglist@gmail.com> writes:

> The example assumes 8-char wide tabs and breaks for people with
> 4-char wide tabs.

Even though as far as this project is concerned, a tab stop is every
8 columns, this is for consumption by end-users who use Git, not for
people who want to improve the code in Git (which this file is part
of), so this "untabify" may make sense.

Thanks.

> Signed-off-by: Richard Hartmann <richih.mailinglist@gmail.com>
> ---
>  templates/hooks--pre-rebase.sample |   16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/templates/hooks--pre-rebase.sample b/templates/hooks--pre-rebase.sample
> index 053f111..b74cd1d 100755
> --- a/templates/hooks--pre-rebase.sample
> +++ b/templates/hooks--pre-rebase.sample
> @@ -132,14 +132,14 @@ With this workflow, you would want to know:
>  
>  Let's look at this example:
>  
> -		   o---o---o---o---o---o---o---o---o---o "next"
> -		  /       /           /           /
> -		 /   a---a---b A     /           /
> -		/   /               /           /
> -	       /   /   c---c---c---c B         /
> -	      /   /   /             \         /
> -	     /   /   /   b---b C     \       /
> -	    /   /   /   /             \     /
> +                   o---o---o---o---o---o---o---o---o---o "next"
> +                  /       /           /           /
> +                 /   a---a---b A     /           /
> +                /   /               /           /
> +               /   /   c---c---c---c B         /
> +              /   /   /             \         /
> +             /   /   /   b---b C     \       /
> +            /   /   /   /             \     /
>      ---o---o---o---o---o---o---o---o---o---o---o "master"

Re: [PATCH 5/6] templates: Fix ASCII art in pre-rebase hook

[Jeff King]

On Mon, Jun 10, 2013 at 08:36:04PM +0200, Richard Hartmann wrote:

> The example assumes 8-char wide tabs and breaks for people with
> 4-char wide tabs.

If you end up re-rolling, it might be worth saying "Let's just convert
all of the tabs to spaces" in the commit message. I was curious what
your solution was (all spaces, or consistent start-tab indentation
followed by spaces), and it was somewhat hard to see in the patch since
the changes were pure whitespace. :)

-Peff

[PATCH 6/6] template: Fix comment indentation in pre-rebase hook

[Richard Hartmann]

The other hooks use two whitespace for indentation instead of tabs
to signify code in the example/echo output.
Follow the same layout in templates/hooks--pre-rebase.sample

Signed-off-by: Richard Hartmann <richih.mailinglist@gmail.com>
---
 templates/hooks--pre-rebase.sample |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/templates/hooks--pre-rebase.sample b/templates/hooks--pre-rebase.sample
index b74cd1d..43426e0 100755
--- a/templates/hooks--pre-rebase.sample
+++ b/templates/hooks--pre-rebase.sample
@@ -157,13 +157,13 @@ B to be deleted.
 
 To compute (1):
 
-	git rev-list ^master ^topic next
-	git rev-list ^master        next
+  git rev-list ^master ^topic next
+  git rev-list ^master        next
 
-	if these match, topic has not merged in next at all.
+  if these match, topic has not merged in next at all.
 
 To compute (2):
 
-	git rev-list master..topic
+  git rev-list master..topic
 
-	if this is empty, it is fully merged to "master".
+  if this is empty, it is fully merged to "master".
-- 
1.7.10.4

Re: [PATCH 6/6] template: Fix comment indentation in pre-rebase hook

[Junio C Hamano]

Richard Hartmann <richih.mailinglist@gmail.com> writes:

> The other hooks use two whitespace for indentation instead of tabs
> to signify code in the example/echo output.
> Follow the same layout in templates/hooks--pre-rebase.sample
>
> Signed-off-by: Richard Hartmann <richih.mailinglist@gmail.com>
> ---
>  templates/hooks--pre-rebase.sample |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/templates/hooks--pre-rebase.sample b/templates/hooks--pre-rebase.sample
> index b74cd1d..43426e0 100755
> --- a/templates/hooks--pre-rebase.sample
> +++ b/templates/hooks--pre-rebase.sample
> @@ -157,13 +157,13 @@ B to be deleted.
>  
>  To compute (1):
>  
> -	git rev-list ^master ^topic next
> -	git rev-list ^master        next
> +  git rev-list ^master ^topic next
> +  git rev-list ^master        next
>  
> -	if these match, topic has not merged in next at all.
> +  if these match, topic has not merged in next at all.
>  
>  To compute (2):
>  
> -	git rev-list master..topic
> +  git rev-list master..topic
>  
> -	if this is empty, it is fully merged to "master".
> +  if this is empty, it is fully merged to "master".

I think offsetting the actual commands to the right is correct, but
"if these match" and "if this is empty" should be flushed to left as
this patch shows.

Re: [PATCH 6/6] template: Fix comment indentation in pre-rebase hook

[Richard Hartmann]

On Mon, Jun 10, 2013 at 9:52 PM, Junio C Hamano <gitster@pobox.com> wrote:


> I think offsetting the actual commands to the right is correct, but
> "if these match" and "if this is empty" should be flushed to left as
> this patch shows.

I actually considered this and decided against it as it seemed to be
deliberate. Should I re-roll and re-send?

I will gladly re-send the whole, or part of the, series once I know
which patches are OK and which need more work.


Richard

Re: [PATCH 6/6] template: Fix comment indentation in pre-rebase hook

[Junio C Hamano]

Richard Hartmann <richih.mailinglist@gmail.com> writes:

> On Mon, Jun 10, 2013 at 9:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>
>> I think offsetting the actual commands to the right is correct, but
>> "if these match" and "if this is empty" should be flushed to left as
>> this patch shows.
>
> I actually considered this and decided against it as it seemed to be
> deliberate. Should I re-roll and re-send?
>
> I will gladly re-send the whole, or part of the, series once I know
> which patches are OK and which need more work.

[PATCH 1/6] templates: Fewer subprocesses in pre-commit hook

  I agree with Peff that "less fork" is a bad justification for this
  change, and also

                echo 'First line
        second line
        third lie'

  looks somewhat bad.

[PATCH 2/6] templates: Reformat pre-commit hook's message

  I think it is a good thing to make the output short by widening.

[PATCH 3/6] templates: Fix spelling in pre-commit hook

  Good.

[PATCH 4/6] Documentation: Update manpage for pre-commit hook

  I debated myself if it should say "The hook _by default_ prevents
  addition of non-ASCII filenames", hinting that it can be
  configured out if it is unwanted.

  Other than that, I think it is a good addition.

[PATCH 5/6] templates: Fix ASCII art in pre-rebase hook

  Good, but see Peff's comments on the explanation.

[PATCH 6/6] template: Fix comment indentation in pre-rebase hook

  After reading the original once again, it is fine as-is without
  the change at all, I think.  Alternatively, "if these match" and
  "if this is empty" lines can be flushed to the left, which also is
  readable.

Thanks.

Re: [PATCH 6/6] template: Fix comment indentation in pre-rebase hook

[Richard Hartmann]

On Tue, Jun 11, 2013 at 12:57 AM, Junio C Hamano <gitster@pobox.com> wrote:

> [PATCH 1/6] templates: Fewer subprocesses in pre-commit hook
>
>   I agree with Peff that "less fork" is a bad justification for this
>   change, and also
>
>                 echo 'First line
>         second line
>         third lie'
>
>   looks somewhat bad.

The repeated echo also looks bad, imo. Also, 2/6 depends on this to
save lines. Should I rewrite with EOF, keep as is, or drop?


> [PATCH 2/6] templates: Reformat pre-commit hook's message
>
>   I think it is a good thing to make the output short by widening.

As I said, 2/6 depends on 1/6 to some extent.


> [PATCH 4/6] Documentation: Update manpage for pre-commit hook
>
>   I debated myself if it should say "The hook _by default_ prevents
>   addition of non-ASCII filenames", hinting that it can be
>   configured out if it is unwanted.
>
>   Other than that, I think it is a good addition.

Will update once I know the complete TODO.


> [PATCH 5/6] templates: Fix ASCII art in pre-rebase hook
>
>   Good, but see Peff's comments on the explanation.

OK.


> [PATCH 6/6] template: Fix comment indentation in pre-rebase hook
>
>   After reading the original once again, it is fine as-is without
>   the change at all, I think.  Alternatively, "if these match" and
>   "if this is empty" lines can be flushed to the left, which also is
>   readable.

I think I will flush and capitalize, then.


Richard

[PATCH 0/6] Update to janitorial work on hook templates

[Richard Hartmann]

Dear all,

I worked Jeff's and Junio's feedback into this patch series, referencing
the old commits.

As stated earlier, you are welcome to drop 1/6, but 2/6 depends on it.
Your choice, both is fine by me.

Thanks,
Richard

Richard Hartmann (6):
  templates: Use heredoc in pre-commit hook
  templates: Reformat pre-commit hook's message
  templates: Fix spelling in pre-commit hook
  Documentation: Update manpage for pre-commit hook
  templates: Fix ASCII art in pre-rebase hook
  template: Fix comment indentation in pre-rebase hook

 Documentation/githooks.txt         |    3 ++-
 templates/hooks--pre-commit.sample |   27 +++++++++++++--------------
 templates/hooks--pre-rebase.sample |   26 +++++++++++++-------------
 3 files changed, 28 insertions(+), 28 deletions(-)

-- 
1.7.10.4

[PATCH 1/6] templates: Use heredoc in pre-commit hook

[Richard Hartmann]

Spawning a new subprocess for every line printed is inefficient.
Use heredoc, instead.

Based on 98770971aef8d1cbc78876d9023d10aa25df0526 in original patch
series from 2013-06-10.

Signed-off-by: Richard Hartmann <richih.mailinglist@gmail.com>
---
 templates/hooks--pre-commit.sample |   25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/templates/hooks--pre-commit.sample b/templates/hooks--pre-commit.sample
index 18c4829..889967c 100755
--- a/templates/hooks--pre-commit.sample
+++ b/templates/hooks--pre-commit.sample
@@ -31,18 +31,19 @@ if [ "$allownonascii" != "true" ] &&
 	test $(git diff --cached --name-only --diff-filter=A -z $against |
 	  LC_ALL=C tr -d '[ -~]\0' | wc -c) != 0
 then
-	echo "Error: Attempt to add a non-ascii file name."
-	echo
-	echo "This can cause problems if you want to work"
-	echo "with people on other platforms."
-	echo
-	echo "To be portable it is advisable to rename the file ..."
-	echo
-	echo "If you know what you are doing you can disable this"
-	echo "check using:"
-	echo
-	echo "  git config hooks.allownonascii true"
-	echo
+	cat <<-EOF
+Error: Attempt to add a non-ascii file name.
+
+This can cause problems if you want to work
+with people on other platforms.
+
+To be portable it is advisable to rename the file.
+
+If you know what you are doing you can disable this
+check using:
+
+  git config hooks.allownonascii true
+EOF
 	exit 1
 fi
 
-- 
1.7.10.4

Re: [PATCH 1/6] templates: Use heredoc in pre-commit hook

[Jonathan Nieder]

Hi,

Richard Hartmann wrote:

> Spawning a new subprocess for every line printed is inefficient.
> Use heredoc, instead.

I think this makes sense as a code clarity, simplicity, and
internationalizability improvement, but don't like the precedent of
eliminating 'echo' for the sake of fork removal (unless we have
measurements showing it's worthwhile, which would be included here).

Maybe a simpler commit message could sidestep the issue?

	Use a heredoc instead of an "echo" for each line.

> Based on 98770971aef8d1cbc78876d9023d10aa25df0526 in original patch
> series from 2013-06-10.

Please don't include this.  The audience for the commit message
doesn't have that commit to compare to.

If you want to preserve the original date, the way to do that is
a "Date:" field at the top of the message body.

	Date: Fri, 28 Jun 2013 21:16:19 +0530

	Spawning a new subprocess for ...

[...]
> --- a/templates/hooks--pre-commit.sample
> +++ b/templates/hooks--pre-commit.sample
> @@ -31,18 +31,19 @@ if [ "$allownonascii" != "true" ] &&
>  	test $(git diff --cached --name-only --diff-filter=A -z $against |
>  	  LC_ALL=C tr -d '[ -~]\0' | wc -c) != 0
>  then
> -	echo "Error: Attempt to add a non-ascii file name."
> -	echo
> -	echo "This can cause problems if you want to work"
> -	echo "with people on other platforms."
> -	echo
> -	echo
> -	echo "If you know what you are doing you can disable this"
> -	echo "check using:"
> -	echo
> -	echo "  git config hooks.allownonascii true"
> -	echo
> +	cat <<-EOF
> +Error: Attempt to add a non-ascii file name.

Using

	cat <<\EOF

would make reading easier since the reader then doesn't have to worry
about whether the text being cat'ed is indented or uses variable
substitutions.

> -	echo "To be portable it is advisable to rename the file ..."
> +To be portable it is advisable to rename the file.

Yes, nice.

With the above nits addressed, this change looks to be going in the
right direction.  Thanks.

Hope that helps,
Jonathan

Re: [PATCH 1/6] templates: Use heredoc in pre-commit hook

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> Based on 98770971aef8d1cbc78876d9023d10aa25df0526 in original patch
>> series from 2013-06-10.
>
> Please don't include this.  The audience for the commit message
> doesn't have that commit to compare to.
>
> If you want to preserve the original date, the way to do that is
> a "Date:" field at the top of the message body.
>
> 	Date: Fri, 28 Jun 2013 21:16:19 +0530

And you generally should not do that, either.

The first date of the publication of _this_ version is recorded on
the Date: header of this message, not the "original path series"
that this round which is _based on_ (meaning, "different from") that
old one.  We do not want to see the date of the old one, either.

>
> 	Spawning a new subprocess for ...
>
> [...]
>> --- a/templates/hooks--pre-commit.sample
>> +++ b/templates/hooks--pre-commit.sample
>> @@ -31,18 +31,19 @@ if [ "$allownonascii" != "true" ] &&
>>  	test $(git diff --cached --name-only --diff-filter=A -z $against |
>>  	  LC_ALL=C tr -d '[ -~]\0' | wc -c) != 0
>>  then
>> -	echo "Error: Attempt to add a non-ascii file name."
>> -	echo
>> -	echo "This can cause problems if you want to work"
>> -	echo "with people on other platforms."
>> -	echo
>> -	echo
>> -	echo "If you know what you are doing you can disable this"
>> -	echo "check using:"
>> -	echo
>> -	echo "  git config hooks.allownonascii true"
>> -	echo
>> +	cat <<-EOF
>> +Error: Attempt to add a non-ascii file name.
>
> Using
>
> 	cat <<\EOF
>
> would make reading easier since the reader then doesn't have to worry
> about whether the text being cat'ed is indented or uses variable
> substitutions.
>
>> -	echo "To be portable it is advisable to rename the file ..."
>> +To be portable it is advisable to rename the file.
>
> Yes, nice.
>
> With the above nits addressed, this change looks to be going in the
> right direction.  Thanks.
>
> Hope that helps,
> Jonathan

Re: [PATCH 1/6] templates: Use heredoc in pre-commit hook

[Junio C Hamano]

Richard Hartmann <richih.mailinglist@gmail.com> writes:

> Spawning a new subprocess for every line printed is inefficient.

This is not a valid justification at all, is it?  

Shells on modern distros and platforms have "echo" built-in, so this
patch replaces series of writes internal to the shell with a fork to
cat with heredoc (which often is implemented with a temporary file).




> Use heredoc, instead.
>
> Based on 98770971aef8d1cbc78876d9023d10aa25df0526 in original patch
> series from 2013-06-10.
>
> Signed-off-by: Richard Hartmann <richih.mailinglist@gmail.com>
> ---
>  templates/hooks--pre-commit.sample |   25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/templates/hooks--pre-commit.sample b/templates/hooks--pre-commit.sample
> index 18c4829..889967c 100755
> --- a/templates/hooks--pre-commit.sample
> +++ b/templates/hooks--pre-commit.sample
> @@ -31,18 +31,19 @@ if [ "$allownonascii" != "true" ] &&
>  	test $(git diff --cached --name-only --diff-filter=A -z $against |
>  	  LC_ALL=C tr -d '[ -~]\0' | wc -c) != 0
>  then
> -	echo "Error: Attempt to add a non-ascii file name."
> -	echo
> -	echo "This can cause problems if you want to work"
> -	echo "with people on other platforms."
> -	echo
> -	echo "To be portable it is advisable to rename the file ..."
> -	echo
> -	echo "If you know what you are doing you can disable this"
> -	echo "check using:"
> -	echo
> -	echo "  git config hooks.allownonascii true"
> -	echo
> +	cat <<-EOF
> +Error: Attempt to add a non-ascii file name.
> +
> +This can cause problems if you want to work
> +with people on other platforms.
> +
> +To be portable it is advisable to rename the file.
> +
> +If you know what you are doing you can disable this
> +check using:
> +
> +  git config hooks.allownonascii true
> +EOF
>  	exit 1
>  fi

Re: [PATCH 1/6] templates: Use heredoc in pre-commit hook

[Richard Hartmann]

On Sun, Jul 14, 2013 at 9:20 PM, Junio C Hamano <gitster@pobox.com> wrote:

> Shells on modern distros and platforms have "echo" built-in, so this
> patch replaces series of writes internal to the shell with a fork to
> cat with heredoc (which often is implemented with a temporary file).

True; fwiw, I replaced my one single echo with heredoc as you
suggested I do that. I don't mind undoing that, or I can drop it from
this series altogether.

Guidance would be appreciated. :)


Richard

Re: [PATCH 1/6] templates: Use heredoc in pre-commit hook

[Jonathan Nieder]

Richard Hartmann wrote:

>       fwiw, I replaced my one single echo with heredoc as you
> suggested I do that. I don't mind undoing that, or I can drop it from
> this series altogether.
>
> Guidance would be appreciated. :)

Thanks for your work, and no problem.

Both Junio's and my responses were about the (confusing and false)
commit message.  Code is not the only thing that matters when
submitting a patch --- commit messages become part of the product,
too, and are especially important as documentation that guides future
contributors.

So my advice is to fix the commit message, prepare improvements to
later patches in the series with help from reviewers where needed,
and then resubmit.

My review also included some advice about the code.  Naturally I
would be happy if that was of use, too. ;-)

Hope that helps,
Jonathan

Re: [PATCH 1/6] templates: Use heredoc in pre-commit hook

[Junio C Hamano]

Richard Hartmann <richih.mailinglist@gmail.com> writes:

> On Sun, Jul 14, 2013 at 9:20 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> Shells on modern distros and platforms have "echo" built-in, so this
>> patch replaces series of writes internal to the shell with a fork to
>> cat with heredoc (which often is implemented with a temporary file).
>
> True; fwiw, I replaced my one single echo with heredoc as you
> suggested I do that. I don't mind undoing that, or I can drop it from
> this series altogether.

The _real_ reason you wanted to do this change in the context of
this series is to make it easier to reword the messages and also
have the messages span the full width of the source line, to match
the expected output better, isn't it?  Git is not _only_ about
performance, so even if using "cat <<here" might make things slower
(I do not think it is measurable), that reason "this way, it is
easier to see how the output given to the users would look like" may
well justify this change.

I just wanted to see the proposed log message state the real reason,
not a performance justification that can be invalidated.

Thanks.

[PATCH 2/6] templates: Reformat pre-commit hook's message

[Richard Hartmann]

Now that we're using heredoc, the message can span the full 80 chars.

Verbatim copy of 634709b489bb3db79f59127fd6bf79c5fd9b5ddf in original
patch series from 2013-06-10.

Signed-off-by: Richard Hartmann <richih.mailinglist@gmail.com>
---
 templates/hooks--pre-commit.sample |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/templates/hooks--pre-commit.sample b/templates/hooks--pre-commit.sample
index 889967c..e09cf89 100755
--- a/templates/hooks--pre-commit.sample
+++ b/templates/hooks--pre-commit.sample
@@ -34,13 +34,11 @@ then
 	cat <<-EOF
 Error: Attempt to add a non-ascii file name.
 
-This can cause problems if you want to work
-with people on other platforms.
+This can cause problems if you want to work with people on other platforms.
 
 To be portable it is advisable to rename the file.
 
-If you know what you are doing you can disable this
-check using:
+If you know what you are doing you can disable this check using:
 
   git config hooks.allownonascii true
 EOF
-- 
1.7.10.4

Re: [PATCH 2/6] templates: Reformat pre-commit hook's message

[Jonathan Nieder]

Richard Hartmann wrote:

> Now that we're using heredoc, the message can span the full 80 chars.

The output is going to a console and not an email, so makes sense. :)

> Verbatim copy of 634709b489bb3db79f59127fd6bf79c5fd9b5ddf in original
> patch series from 2013-06-10.

As in patch 1, please drop this.  I'll stop mentioning that for the
later patches, but the same comment applies there.

Thanks,
Jonathan

[PATCH 3/6] templates: Fix spelling in pre-commit hook

[Richard Hartmann]

Based on 0b9b01276553de8097442c3c996b7a49367dd234 in original patch
series.

Signed-off-by: Richard Hartmann <richih.mailinglist@gmail.com>
---
 templates/hooks--pre-commit.sample |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/templates/hooks--pre-commit.sample b/templates/hooks--pre-commit.sample
index e09cf89..78baef6 100755
--- a/templates/hooks--pre-commit.sample
+++ b/templates/hooks--pre-commit.sample
@@ -15,13 +15,13 @@ else
 	against=4b825dc642cb6eb9a060e54bf8d69288fbee4904
 fi
 
-# If you want to allow non-ascii filenames set this variable to true.
+# If you want to allow non-ASCII filenames set this variable to true.
 allownonascii=$(git config hooks.allownonascii)
 
 # Redirect output to stderr.
 exec 1>&2
 
-# Cross platform projects tend to avoid non-ascii filenames; prevent
+# Cross platform projects tend to avoid non-ASCII filenames; prevent
 # them from being added to the repository. We exploit the fact that the
 # printable range starts at the space character and ends with tilde.
 if [ "$allownonascii" != "true" ] &&
@@ -32,7 +32,7 @@ if [ "$allownonascii" != "true" ] &&
 	  LC_ALL=C tr -d '[ -~]\0' | wc -c) != 0
 then
 	cat <<-EOF
-Error: Attempt to add a non-ascii file name.
+Error: Attempt to add a non-ASCII file name.
 
 This can cause problems if you want to work with people on other platforms.
 
-- 
1.7.10.4

[PATCH 4/6] Documentation: Update manpage for pre-commit hook

[Richard Hartmann]

Verbatim copy of 4b8234b2693af634a77ea059331d1658e070f6d7 in original
patch series from 2013-06-10.

Signed-off-by: Richard Hartmann <richih.mailinglist@gmail.com>
---
 Documentation/githooks.txt |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index b9003fe..1276730 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -80,7 +80,8 @@ causes the 'git commit' to abort.
 
 The default 'pre-commit' hook, when enabled, catches introduction
 of lines with trailing whitespaces and aborts the commit when
-such a line is found.
+such a line is found. It will also prevent addition of non-ASCII
+file names.
 
 All the 'git commit' hooks are invoked with the environment
 variable `GIT_EDITOR=:` if the command will not bring up an editor
-- 
1.7.10.4

Re: [PATCH 4/6] Documentation: Update manpage for pre-commit hook

[Jonathan Nieder]

Richard Hartmann wrote:

> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -80,7 +80,8 @@ causes the 'git commit' to abort.
>  
>  The default 'pre-commit' hook, when enabled, catches introduction
>  of lines with trailing whitespaces and aborts the commit when
> -such a line is found.
> +such a line is found. It will also prevent addition of non-ASCII
> +file names.

The tenses are inconsistent here ("catches" versus "will also").

It also seems odd to call the sample hooks "default" hooks, but that's
a wider problem and should probably be fixed by one commit all at once
(maybe imitating the wording of the prepare-commit-message
description).  Previously enabling them was a matter of a "chmod +x"
and the wording made more sense.

How about:

	The default 'pre-commit' hook, when enabled, prevents introduction
	of lines with trailing whitespace and prevents introduction of
	files with non-ASCII filenames unless the hooks.allowNonAscii
	configuration variable is true.

Re: [PATCH 4/6] Documentation: Update manpage for pre-commit hook

[Junio C Hamano]

Richard Hartmann <richih.mailinglist@gmail.com> writes:

> Verbatim copy of 4b8234b2693af634a77ea059331d1658e070f6d7 in original
> patch series from 2013-06-10.

As Jonathan said, this is not a commit log message.

I've applied up to 3/6 with fixups, but will stop here for now.

>
> Signed-off-by: Richard Hartmann <richih.mailinglist@gmail.com>
> ---
>  Documentation/githooks.txt |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index b9003fe..1276730 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -80,7 +80,8 @@ causes the 'git commit' to abort.
>  
>  The default 'pre-commit' hook, when enabled, catches introduction
>  of lines with trailing whitespaces and aborts the commit when
> -such a line is found.
> +such a line is found. It will also prevent addition of non-ASCII
> +file names.
>  
>  All the 'git commit' hooks are invoked with the environment
>  variable `GIT_EDITOR=:` if the command will not bring up an editor

[PATCH 5/6] templates: Fix ASCII art in pre-rebase hook

[Richard Hartmann]

The example assumes 8-char wide tabs and breaks for people with
4-char wide tabs. Convert all of those tabs to whitespace, instead.

Verbatim copy of 11edd8a05778700382e6a21cfc0a6b5b72eff852 in original
patch series from 2013-06-10.

Signed-off-by: Richard Hartmann <richih.mailinglist@gmail.com>
---
 templates/hooks--pre-rebase.sample |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/templates/hooks--pre-rebase.sample b/templates/hooks--pre-rebase.sample
index 053f111..b74cd1d 100755
--- a/templates/hooks--pre-rebase.sample
+++ b/templates/hooks--pre-rebase.sample
@@ -132,14 +132,14 @@ With this workflow, you would want to know:
 
 Let's look at this example:
 
-		   o---o---o---o---o---o---o---o---o---o "next"
-		  /       /           /           /
-		 /   a---a---b A     /           /
-		/   /               /           /
-	       /   /   c---c---c---c B         /
-	      /   /   /             \         /
-	     /   /   /   b---b C     \       /
-	    /   /   /   /             \     /
+                   o---o---o---o---o---o---o---o---o---o "next"
+                  /       /           /           /
+                 /   a---a---b A     /           /
+                /   /               /           /
+               /   /   c---c---c---c B         /
+              /   /   /             \         /
+             /   /   /   b---b C     \       /
+            /   /   /   /             \     /
     ---o---o---o---o---o---o---o---o---o---o---o "master"
 
 
-- 
1.7.10.4

Re: [PATCH 5/6] templates: Fix ASCII art in pre-rebase hook

[Jonathan Nieder]

Richard Hartmann wrote:

> The example assumes 8-char wide tabs and breaks for people with
> 4-char wide tabs. Convert all of those tabs to whitespace, instead.

Makes sense --- we cannot assume much about the end-user's editor
setup used to look at sample hooks.

Thanks.

[PATCH 6/6] template: Fix comment indentation in pre-rebase hook

[Richard Hartmann]

The other hooks use two whitespace for indentation instead of tabs
to signify code in the example/echo output.
Follow the same layout in templates/hooks--pre-rebase.sample

Based on d153a68bebfabc1db5241d02ee75fa5cb4538ab0 in original patch
series from 2013-06-10.

Signed-off-by: Richard Hartmann <richih.mailinglist@gmail.com>
---
 templates/hooks--pre-rebase.sample |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/templates/hooks--pre-rebase.sample b/templates/hooks--pre-rebase.sample
index b74cd1d..cec3474 100755
--- a/templates/hooks--pre-rebase.sample
+++ b/templates/hooks--pre-rebase.sample
@@ -157,13 +157,13 @@ B to be deleted.
 
 To compute (1):
 
-	git rev-list ^master ^topic next
-	git rev-list ^master        next
+  git rev-list ^master ^topic next
+  git rev-list ^master        next
 
-	if these match, topic has not merged in next at all.
+if these match, topic has not merged in next at all.
 
 To compute (2):
 
-	git rev-list master..topic
+  git rev-list master..topic
 
-	if this is empty, it is fully merged to "master".
+if this is empty, it is fully merged to "master".
-- 
1.7.10.4

Re: [PATCH 6/6] template: Fix comment indentation in pre-rebase hook

[Jonathan Nieder]

Richard Hartmann wrote:

> The other hooks use two whitespace for indentation instead of tabs
> to signify code in the example/echo output.
> Follow the same layout in templates/hooks--pre-rebase.sample

I don't understand the point of this one.  Is it just consistency for
the sake of consistency?  Aren't other parts of git inconsistent in
this area?