[PATCH] pre-commit hook should ignore carriage returns at EOL

[PATCH] pre-commit hook should ignore carriage returns at EOL

[Christian Holtje]

When commit files that use DOS style CRLF end-of-lines, the pre-commit
hook would raise an error.  When combined with the fact that the hooks
get activated by default on windows, it makes life difficult for
people working with DOS files.

This patch causes the pre-commit hook to deal with crlf files
correctly.

Signed-off-by: Christian Höltje <docwhat@gmail.com>
---
  t/t7503-template-hook--pre-commit.sh |   75 +++++++++++++++++++++++++ 
+++++++++
  templates/hooks--pre-commit          |   10 ++++-
  2 files changed, 83 insertions(+), 2 deletions(-)
  create mode 100755 t/t7503-template-hook--pre-commit.sh

diff --git a/t/t7503-template-hook--pre-commit.sh b/t/t7503-template- 
hook--pre-commit.sh
new file mode 100755
index 0000000..8f0c3c9
--- /dev/null
+++ b/t/t7503-template-hook--pre-commit.sh
@@ -0,0 +1,75 @@
+#!/bin/sh
+#
+# Copyright (c) 2008 Christian Höltje
+#
+
+test_description='t7503 templates-hooks--pre-commit
+
+This test verifies that the pre-commit hook shipped with
+git by default works correctly.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'verify that autocrlf is unset' '
+   if git config core.autocrlf
+   then
+     false
+   else
+     test $? -eq 1
+   fi
+'
+
+test_expect_success 'lf without hook' '
+
+	echo "foo" > lf.txt &&
+	git add lf.txt &&
+	git commit -m "lf without hook" lf.txt
+
+'
+
+test_expect_success 'crlf without hook' '
+
+	echo "foo\r" > crlf.txt &&
+	git add crlf.txt &&
+	git commit -m "crlf without hook" crlf.txt
+
+'
+
+# Set up the pre-commit hook.
+HOOKDIR="$(git rev-parse --git-dir)/hooks"
+mkdir -p "${HOOKDIR}"
+cp -r "${HOOKDIR}-disabled/pre-commit" "${HOOKDIR}/pre-commit"
+chmod +x "${HOOKDIR}/pre-commit"
+
+test_expect_success 'lf with hook' '
+
+	echo "bar" >> lf.txt &&
+	git add lf.txt &&
+	git commit -m "lf with hook" lf.txt
+
+'
+test_expect_success 'crlf with hook' '
+
+	echo "bar\r" >> crlf.txt &&
+	git add crlf.txt &&
+	git commit -m "crlf with hook" crlf.txt
+
+'
+
+test_expect_success 'lf with hook white-space failure' '
+
+	echo "bar " >> lf.txt &&
+	git add lf.txt &&
+	! git commit -m "lf with hook" lf.txt
+
+'
+test_expect_success 'crlf with hook white-space failure' '
+
+	echo "bar \r" >> crlf.txt &&
+	git add crlf.txt &&
+	! git commit -m "crlf with hook" crlf.txt
+
+'
+
+test_done
diff --git a/templates/hooks--pre-commit b/templates/hooks--pre-commit
index b25dce6..335ca09 100644
--- a/templates/hooks--pre-commit
+++ b/templates/hooks--pre-commit
@@ -55,8 +55,14 @@ perl -e '
  	if (s/^\+//) {
  	    $lineno++;
  	    chomp;
-	    if (/\s$/) {
-		bad_line("trailing whitespace", $_);
+	    if (/\r$/) {
+		if (/\s\r$/) {
+		    bad_line("trailing whitespace", $_);
+		}
+	    } else {
+		if (/\s$/) {
+		    bad_line("trailing whitespace", $_);
+		}
  	    }
  	    if (/^\s* \t/) {
  		bad_line("indent SP followed by a TAB", $_);
-- 
1.5.5.4

Re: [PATCH] pre-commit hook should ignore carriage returns at EOL

[Alf Clement]

Hi Christian,

thanks for the patch. I use git under Windows and also run often into
these problems,
because I have to (but don't like to) use come compilers under Windows.
I usually comment the two bad_lines() in the pre-commit-hook out by hand:
"trailing whitespace" and "indent SP followed by TAB", because i.e.
Visual Studio writes some files out, which trigger these checks.

Can't we get rid of these checks?

CU,
Alf

On 6/24/08, Christian Holtje <docwhat@gmail.com> wrote:
> When commit files that use DOS style CRLF end-of-lines, the pre-commit
> hook would raise an error.  When combined with the fact that the hooks
> get activated by default on windows, it makes life difficult for
> people working with DOS files.
>
> This patch causes the pre-commit hook to deal with crlf files
> correctly.
>
> Signed-off-by: Christian Höltje <docwhat@gmail.com>
> ---
>   t/t7503-template-hook--pre-commit.sh |   75 +++++++++++++++++++++++++
> +++++++++
>   templates/hooks--pre-commit          |   10 ++++-
>   2 files changed, 83 insertions(+), 2 deletions(-)
>   create mode 100755 t/t7503-template-hook--pre-commit.sh
>
> diff --git a/t/t7503-template-hook--pre-commit.sh b/t/t7503-template-
> hook--pre-commit.sh
> new file mode 100755
> index 0000000..8f0c3c9
> --- /dev/null
> +++ b/t/t7503-template-hook--pre-commit.sh
> @@ -0,0 +1,75 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2008 Christian Höltje
> +#
> +
> +test_description='t7503 templates-hooks--pre-commit
> +
> +This test verifies that the pre-commit hook shipped with
> +git by default works correctly.
> +'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'verify that autocrlf is unset' '
> +   if git config core.autocrlf
> +   then
> +     false
> +   else
> +     test $? -eq 1
> +   fi
> +'
> +
> +test_expect_success 'lf without hook' '
> +
> +	echo "foo" > lf.txt &&
> +	git add lf.txt &&
> +	git commit -m "lf without hook" lf.txt
> +
> +'
> +
> +test_expect_success 'crlf without hook' '
> +
> +	echo "foo\r" > crlf.txt &&
> +	git add crlf.txt &&
> +	git commit -m "crlf without hook" crlf.txt
> +
> +'
> +
> +# Set up the pre-commit hook.
> +HOOKDIR="$(git rev-parse --git-dir)/hooks"
> +mkdir -p "${HOOKDIR}"
> +cp -r "${HOOKDIR}-disabled/pre-commit" "${HOOKDIR}/pre-commit"
> +chmod +x "${HOOKDIR}/pre-commit"
> +
> +test_expect_success 'lf with hook' '
> +
> +	echo "bar" >> lf.txt &&
> +	git add lf.txt &&
> +	git commit -m "lf with hook" lf.txt
> +
> +'
> +test_expect_success 'crlf with hook' '
> +
> +	echo "bar\r" >> crlf.txt &&
> +	git add crlf.txt &&
> +	git commit -m "crlf with hook" crlf.txt
> +
> +'
> +
> +test_expect_success 'lf with hook white-space failure' '
> +
> +	echo "bar " >> lf.txt &&
> +	git add lf.txt &&
> +	! git commit -m "lf with hook" lf.txt
> +
> +'
> +test_expect_success 'crlf with hook white-space failure' '
> +
> +	echo "bar \r" >> crlf.txt &&
> +	git add crlf.txt &&
> +	! git commit -m "crlf with hook" crlf.txt
> +
> +'
> +
> +test_done
> diff --git a/templates/hooks--pre-commit b/templates/hooks--pre-commit
> index b25dce6..335ca09 100644
> --- a/templates/hooks--pre-commit
> +++ b/templates/hooks--pre-commit
> @@ -55,8 +55,14 @@ perl -e '
>   	if (s/^\+//) {
>   	    $lineno++;
>   	    chomp;
> -	    if (/\s$/) {
> -		bad_line("trailing whitespace", $_);
> +	    if (/\r$/) {
> +		if (/\s\r$/) {
> +		    bad_line("trailing whitespace", $_);
> +		}
> +	    } else {
> +		if (/\s$/) {
> +		    bad_line("trailing whitespace", $_);
> +		}
>   	    }
>   	    if (/^\s* \t/) {
>   		bad_line("indent SP followed by a TAB", $_);
> --
> 1.5.5.4
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH] pre-commit hook should ignore carriage returns at EOL

[Ian Hilt]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3387 bytes --]

On Tue, 24 Jun 2008 at 12:23pm -0400, Christian Holtje wrote:

> When commit files that use DOS style CRLF end-of-lines, the pre-commit
> hook would raise an error.  When combined with the fact that the hooks
> get activated by default on windows, it makes life difficult for
> people working with DOS files.
> 
> This patch causes the pre-commit hook to deal with crlf files
> correctly.
> 
> Signed-off-by: Christian Höltje <docwhat@gmail.com>
> ---
> t/t7503-template-hook--pre-commit.sh |   75 ++++++++++++++++++++++++++++++++++
> templates/hooks--pre-commit          |   10 ++++-
> 2 files changed, 83 insertions(+), 2 deletions(-)
> create mode 100755 t/t7503-template-hook--pre-commit.sh
> 
> diff --git a/t/t7503-template-hook--pre-commit.sh
> b/t/t7503-template-hook--pre-commit.sh
> new file mode 100755
> index 0000000..8f0c3c9
> --- /dev/null
> +++ b/t/t7503-template-hook--pre-commit.sh
> @@ -0,0 +1,75 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2008 Christian Höltje
> +#
> +
> +test_description='t7503 templates-hooks--pre-commit
> +
> +This test verifies that the pre-commit hook shipped with
> +git by default works correctly.
> +'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'verify that autocrlf is unset' '
> +   if git config core.autocrlf
> +   then
> +     false
> +   else
> +     test $? -eq 1
> +   fi
> +'
> +
> +test_expect_success 'lf without hook' '
> +
> +	echo "foo" > lf.txt &&
> +	git add lf.txt &&
> +	git commit -m "lf without hook" lf.txt
> +
> +'
> +
> +test_expect_success 'crlf without hook' '
> +
> +	echo "foo\r" > crlf.txt &&

Perhaps you want to use printf "foo\r" instead?  echo "foo\r" does not
produce a CR on my system.

> +	git add crlf.txt &&
> +	git commit -m "crlf without hook" crlf.txt
> +
> +'
> +
> +# Set up the pre-commit hook.
> +HOOKDIR="$(git rev-parse --git-dir)/hooks"
> +mkdir -p "${HOOKDIR}"
> +cp -r "${HOOKDIR}-disabled/pre-commit" "${HOOKDIR}/pre-commit"
> +chmod +x "${HOOKDIR}/pre-commit"
> +
> +test_expect_success 'lf with hook' '
> +
> +	echo "bar" >> lf.txt &&
> +	git add lf.txt &&
> +	git commit -m "lf with hook" lf.txt
> +
> +'
> +test_expect_success 'crlf with hook' '
> +
> +	echo "bar\r" >> crlf.txt &&
> +	git add crlf.txt &&
> +	git commit -m "crlf with hook" crlf.txt
> +
> +'
> +
> +test_expect_success 'lf with hook white-space failure' '
> +
> +	echo "bar " >> lf.txt &&
> +	git add lf.txt &&
> +	! git commit -m "lf with hook" lf.txt
> +
> +'
> +test_expect_success 'crlf with hook white-space failure' '
> +
> +	echo "bar \r" >> crlf.txt &&
> +	git add crlf.txt &&
> +	! git commit -m "crlf with hook" crlf.txt
> +
> +'
> +
> +test_done
> diff --git a/templates/hooks--pre-commit b/templates/hooks--pre-commit
> index b25dce6..335ca09 100644
> --- a/templates/hooks--pre-commit
> +++ b/templates/hooks--pre-commit
> @@ -55,8 +55,14 @@ perl -e '
> 	if (s/^\+//) {
> 	    $lineno++;
> 	    chomp;
> -	    if (/\s$/) {
> -		bad_line("trailing whitespace", $_);
> +	    if (/\r$/) {

Wouldn't it be less redundant to do a test for \s\r$ here instead of
testing for \r$ and then \s\r$?

> +		if (/\s\r$/) {
> +		    bad_line("trailing whitespace", $_);
> +		}
> +	    } else {
> +		if (/\s$/) {
> +		    bad_line("trailing whitespace", $_);
> +		}
> 	    }
> 	    if (/^\s* \t/) {
> 		bad_line("indent SP followed by a TAB", $_);
> 

-- 
Ian Hilt
Ian.Hilt (at) gmx.com
GnuPG key: 0x4AFC1EE3

Re: [PATCH] pre-commit hook should ignore carriage returns at EOL

[Jakub Narebski]

Ian Hilt <Ian.Hilt@gmx.com> writes:

> On Tue, 24 Jun 2008 at 12:23pm -0400, Christian Holtje wrote:

> > --- a/templates/hooks--pre-commit
> > +++ b/templates/hooks--pre-commit
> > @@ -55,8 +55,14 @@ perl -e '
> > 	if (s/^\+//) {
> > 	    $lineno++;
> > 	    chomp;
> > -	    if (/\s$/) {
> > -		bad_line("trailing whitespace", $_);
> > +	    if (/\r$/) {
> 
> Wouldn't it be less redundant to do a test for \s\r$ here instead of
> testing for \r$ and then \s\r$?

I don't think so, because you want next to test for whitespace
where it _doesn't_ end in \r, i.e. this condition is here
because of the 'else' clause.  IIRC.
-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH] pre-commit hook should ignore carriage returns at EOL

[Ian Hilt]

On Tue, 24 Jun 2008 at 12:05pm -0700, Jakub Narebski wrote:

> I don't think so, because you want next to test for whitespace
> where it _doesn't_ end in \r, i.e. this condition is here
> because of the 'else' clause.  IIRC.

What I'm suggesting is this,

	if (/\s\r$/) {
		bad_line("trailing whitespace", $_);
	} else {
		if (/\s$/) {
			bad_line("trailing whitespace", $_);
		}
	}

Why only test for \r when all you want to know is whether there is
whitespace before \r ?  If there isn't whitespace and \r at the end of a
line, then only test for whitespace at the end of a line.


-- 
Ian Hilt
Ian.Hilt (at) gmx.com
GnuPG key: 0x4AFC1EE3

Re: [PATCH] pre-commit hook should ignore carriage returns at EOL

[Jakub Narebski]

On Tue, 24 Jun 2008, Ian Hilt wrote:
> On Tue, 24 Jun 2008 at 12:05pm -0700, Jakub Narebski wrote:
> 
> > I don't think so, because you want next to test for whitespace
> > where it _doesn't_ end in \r, i.e. this condition is here
> > because of the 'else' clause.  IIRC.
> 
> What I'm suggesting is this,
> 
> 	if (/\s\r$/) {
> 		bad_line("trailing whitespace", $_);
> 	} else {
> 		if (/\s$/) {
> 			bad_line("trailing whitespace", $_);
> 		}
> 	}
> 
> Why only test for \r when all you want to know is whether there is
> whitespace before \r ?  If there isn't whitespace and \r at the end
> of a line, then only test for whitespace at the end of a line.

Unfortunately \r matches \s (is whitespace), so if line ends with CR LF
("\r\n") it wouldn't match first regexp, so it would go to 'else' 
clause, where it would match /\s$/ and it shouldn't.

-- 
Jakub Narebski
Poland

Re: [PATCH] pre-commit hook should ignore carriage returns at EOL

[Ian Hilt]

On Tue, 24 Jun 2008 at 10:09pm +0200, Jakub Narebski wrote:

> Unfortunately \r matches \s (is whitespace), so if line ends with CR LF
> ("\r\n") it wouldn't match first regexp, so it would go to 'else' 
> clause, where it would match /\s$/ and it shouldn't.

Right.  Sorry for the noise.

-- 
Ian Hilt
Ian.Hilt (at) gmx.com
GnuPG key: 0x4AFC1EE3

Re: [PATCH] pre-commit hook should ignore carriage returns at EOL

[Christian Holtje]

On Jun 24, 2008, at 2:26 PM, Ian Hilt wrote:
> Perhaps you want to use printf "foo\r" instead?  echo "foo\r" does not
> produce a CR on my system.

...

> Wouldn't it be less redundant to do a test for \s\r$ here instead of
> testing for \r$ and then \s\r$?


The code is checking for \r$ and then doing a different space check  
depending on that, not one after another.

Thanks for the feedback. I'll put up v2 in a second.

Ciao!

Re: [PATCH] pre-commit hook should ignore carriage returns at EOL

[Junio C Hamano]

Christian Holtje <docwhat@gmail.com> writes:

> The code is checking for \r$ and then doing a different space check
> depending on that, not one after another.
>
> Thanks for the feedback. I'll put up v2 in a second.

Please don't.

It's an ancient sample hook that is not be enabled by default.  I do not
want people to be wasting too much time on the relic.

However, if this sample is to be changed at all, please do it right.

If somebody suddenly adds CR at the end of an existing file that ought to
have LF line endings, we _DO_ want to catch that as a breakage.  So the
title of the commit "should ignore carriage returns at EOL" is WRONG.  It
shouldn't, in general.

One thing the hook could and probably should do these days is if the file
type says you _ought to_ have CRLF line endings, actively make sure your
lines do end with CRLF (this is a much stronger and better check than
blindly ignoring CR before LF for such files).  And on the other hand, if
the file should end with LF, do make sure it does not have CR before it.

The person who did the sample hook you are looking at couldn't do so
because there weren't autocrlf nor gitattributes(5) facility back then.
But you can use them now to rewrite this properly.

I wonder if "git diff --check" can be used for most if not all of the
checking, without the big Perl script you are touching in your patch.
That facility did not exist when the current sample hook was written,
either.

Re: [PATCH] pre-commit hook should ignore carriage returns at EOL

[Christian Holtje]

On Jun 24, 2008, at 6:31 PM, Junio C Hamano wrote:

> Christian Holtje <docwhat@gmail.com> writes:
>
>> The code is checking for \r$ and then doing a different space check
>> depending on that, not one after another.
>>
>> Thanks for the feedback. I'll put up v2 in a second.
>
> Please don't.
>
> It's an ancient sample hook that is not be enabled by default.  I do  
> not
> want people to be wasting too much time on the relic

Part of the reason I'm fixing this is because it *is* enabled by  
default in windows.  I don't know why, but cygwin always marks it with  
executable.

> However, if this sample is to be changed at all, please do it right.
>
> If somebody suddenly adds CR at the end of an existing file that  
> ought to
> have LF line endings, we _DO_ want to catch that as a breakage.  So  
> the
> title of the commit "should ignore carriage returns at EOL" is  
> WRONG.  It
> shouldn't, in general.
>
> One thing the hook could and probably should do these days is if the  
> file
> type says you _ought to_ have CRLF line endings, actively make sure  
> your
> lines do end with CRLF (this is a much stronger and better check than
> blindly ignoring CR before LF for such files).  And on the other  
> hand, if
> the file should end with LF, do make sure it does not have CR before  
> it.
>
> The person who did the sample hook you are looking at couldn't do so
> because there weren't autocrlf nor gitattributes(5) facility back  
> then.
> But you can use them now to rewrite this properly.

How can I detect if a file should have CRLF vs. LF?  I didn't do a  
better check because I didn't know how.

> I wonder if "git diff --check" can be used for most if not all of the
> checking, without the big Perl script you are touching in your patch.
> That facility did not exist when the current sample hook was written,
> either.

I like that better.  Ditching the whole perl script in a shell script  
seems better.

I wrote a test case for what you describe above (a crlf file with an  
lf line or a lf file with a crlf) but "git diff --check" doesn't catch  
that.

Based on the information about core.whitespace doesn't git do this  
already?  Maybe we should just delete the pre-commit hook or make it  
empty with a note saying what you can do with it?

Ciao!

Re: [PATCH] pre-commit hook should ignore carriage returns at EOL

[Jakub Narebski]

Christian Holtje <docwhat@gmail.com> writes:

> Based on the information about core.whitespace doesn't git do this
> already?  Maybe we should just delete the pre-commit hook or make it
> empty with a note saying what you can do with it?

pre-commit hook also detects unresolved merge conflicts, which
core.whitespace doesn't (and probably shouldn't).

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH] pre-commit hook should ignore carriage returns at EOL

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Christian Holtje <docwhat@gmail.com> writes:
>
>> Based on the information about core.whitespace doesn't git do this
>> already?  Maybe we should just delete the pre-commit hook or make it
>> empty with a note saying what you can do with it?
>
> pre-commit hook also detects unresolved merge conflicts, which
> core.whitespace doesn't (and probably shouldn't).

But perhaps "diff --check" should.

Re: [PATCH] pre-commit hook should ignore carriage returns at EOL

[Christian Holtje]

On Jun 24, 2008, at 7:39 PM, Junio C Hamano wrote:

> Jakub Narebski <jnareb@gmail.com> writes:
>
>> Christian Holtje <docwhat@gmail.com> writes:
>>
>>> Based on the information about core.whitespace doesn't git do this
>>> already?  Maybe we should just delete the pre-commit hook or make it
>>> empty with a note saying what you can do with it?
>>
>> pre-commit hook also detects unresolved merge conflicts, which
>> core.whitespace doesn't (and probably shouldn't).
>
> But perhaps "diff --check" should.
>

definitely; that sounds like an obvious thing for "diff --check".

Ciao!

Re: [PATCH] pre-commit hook should ignore carriage returns at EOL

[Junio C Hamano]

Christian Holtje <docwhat@gmail.com> writes:

> On Jun 24, 2008, at 6:31 PM, Junio C Hamano wrote:
> ...
>> It's an ancient sample hook that is not be enabled by default.  I do
>> not want people to be wasting too much time on the relic.
>
> Part of the reason I'm fixing this is because it *is* enabled by
> default in windows.  I don't know why, but cygwin always marks it with
> executable.

That's a packaging issue, and I've always wanted to see fixes related to
platform specific packaging issues come from people who _need_ fixes on
their platforms.  I have been waiting forever for that to happen without
complaining to them.  I was hoping some enlightened people will emerge
even from Windows camps, waiting patiently.

But I ran out of patience, and I am finally fed up waiting.

How about doing this everywhere, not just on Windows, for 1.6.0?

-- >8 --
[PATCH] Ship sample hooks with .sample suffix

We used to mark hooks we ship as samples by making them unexecutable, but
some filesystems cannot tell what is executable and what is not.

This makes it much more explicit.  The hooks are suffixed with .sample
(but now are made executable), so enabling it is still one step operation
(instead of "chmod +x $hook", you would do "mv $hook.sample $hook") but
now they won't get accidentally enabled on systems without executable bit.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 Documentation/githooks.txt                         |    3 ++-
 ...applypatch-msg => hooks--applypatch-msg.sample} |    0
 ...{hooks--commit-msg => hooks--commit-msg.sample} |    0
 ...ooks--post-commit => hooks--post-commit.sample} |    0
 ...ks--post-receive => hooks--post-receive.sample} |    0
 ...ooks--post-update => hooks--post-update.sample} |    0
 ...pre-applypatch => hooks--pre-applypatch.sample} |    0
 ...{hooks--pre-commit => hooks--pre-commit.sample} |    0
 ...{hooks--pre-rebase => hooks--pre-rebase.sample} |    0
 ...commit-msg => hooks--prepare-commit-msg.sample} |    0
 templates/{hooks--update => hooks--update.sample}  |    0
 11 files changed, 2 insertions(+), 1 deletions(-)
 rename templates/{hooks--applypatch-msg => hooks--applypatch-msg.sample} (100%)
 mode change 100644 => 100755
 rename templates/{hooks--commit-msg => hooks--commit-msg.sample} (100%)
 mode change 100644 => 100755
 rename templates/{hooks--post-commit => hooks--post-commit.sample} (100%)
 mode change 100644 => 100755
 rename templates/{hooks--post-receive => hooks--post-receive.sample} (100%)
 mode change 100644 => 100755
 rename templates/{hooks--post-update => hooks--post-update.sample} (100%)
 mode change 100644 => 100755
 rename templates/{hooks--pre-applypatch => hooks--pre-applypatch.sample} (100%)
 mode change 100644 => 100755
 rename templates/{hooks--pre-commit => hooks--pre-commit.sample} (100%)
 mode change 100644 => 100755
 rename templates/{hooks--pre-rebase => hooks--pre-rebase.sample} (100%)
 mode change 100644 => 100755
 rename templates/{hooks--prepare-commit-msg => hooks--prepare-commit-msg.sample} (100%)
 mode change 100644 => 100755
 rename templates/{hooks--update => hooks--update.sample} (100%)
 mode change 100644 => 100755

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 4f06ae0..262a4f1 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -17,7 +17,8 @@ Hooks are little scripts you can place in `$GIT_DIR/hooks`
 directory to trigger action at certain points.  When
 `git-init` is run, a handful example hooks are copied in the
 `hooks` directory of the new repository, but by default they are
-all disabled.  To enable a hook, make it executable with `chmod +x`.
+all disabled.  To enable a hook, rename it by removing its `.sample`
+suffix.
 
 This document describes the currently defined hooks.
 
diff --git a/templates/hooks--applypatch-msg b/templates/hooks--applypatch-msg.sample
old mode 100644
new mode 100755
similarity index 100%
rename from templates/hooks--applypatch-msg
rename to templates/hooks--applypatch-msg.sample
diff --git a/templates/hooks--commit-msg b/templates/hooks--commit-msg.sample
old mode 100644
new mode 100755
similarity index 100%
rename from templates/hooks--commit-msg
rename to templates/hooks--commit-msg.sample
diff --git a/templates/hooks--post-commit b/templates/hooks--post-commit.sample
old mode 100644
new mode 100755
similarity index 100%
rename from templates/hooks--post-commit
rename to templates/hooks--post-commit.sample
diff --git a/templates/hooks--post-receive b/templates/hooks--post-receive.sample
old mode 100644
new mode 100755
similarity index 100%
rename from templates/hooks--post-receive
rename to templates/hooks--post-receive.sample
diff --git a/templates/hooks--post-update b/templates/hooks--post-update.sample
old mode 100644
new mode 100755
similarity index 100%
rename from templates/hooks--post-update
rename to templates/hooks--post-update.sample
diff --git a/templates/hooks--pre-applypatch b/templates/hooks--pre-applypatch.sample
old mode 100644
new mode 100755
similarity index 100%
rename from templates/hooks--pre-applypatch
rename to templates/hooks--pre-applypatch.sample
diff --git a/templates/hooks--pre-commit b/templates/hooks--pre-commit.sample
old mode 100644
new mode 100755
similarity index 100%
rename from templates/hooks--pre-commit
rename to templates/hooks--pre-commit.sample
diff --git a/templates/hooks--pre-rebase b/templates/hooks--pre-rebase.sample
old mode 100644
new mode 100755
similarity index 100%
rename from templates/hooks--pre-rebase
rename to templates/hooks--pre-rebase.sample
diff --git a/templates/hooks--prepare-commit-msg b/templates/hooks--prepare-commit-msg.sample
old mode 100644
new mode 100755
similarity index 100%
rename from templates/hooks--prepare-commit-msg
rename to templates/hooks--prepare-commit-msg.sample
diff --git a/templates/hooks--update b/templates/hooks--update.sample
old mode 100644
new mode 100755
similarity index 100%
rename from templates/hooks--update
rename to templates/hooks--update.sample

[PATCH] Ship sample hooks with .sample suffix

[Junio C Hamano]

We used to mark hooks we ship as samples by making them unexecutable, but
some filesystems cannot tell what is executable and what is not.

This makes it much more explicit.  The hooks are suffixed with .sample
(but now are made executable), so enabling it is still one step operation
(instead of "chmod +x $hook", you would do "mv $hook.sample $hook") but
now they won't get accidentally enabled on systems without executable bit.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This is for actual application.  Hopefully I got all the "to enable
   this hook, make it executable" in the samples right.

 Documentation/githooks.txt                         |    3 ++-
 ...applypatch-msg => hooks--applypatch-msg.sample} |    2 +-
 ...{hooks--commit-msg => hooks--commit-msg.sample} |    2 +-
 ...ooks--post-commit => hooks--post-commit.sample} |    2 +-
 ...ks--post-receive => hooks--post-receive.sample} |   13 ++++++-------
 ...ooks--post-update => hooks--post-update.sample} |    2 +-
 ...pre-applypatch => hooks--pre-applypatch.sample} |    2 +-
 ...{hooks--pre-commit => hooks--pre-commit.sample} |    2 +-
 ...{hooks--pre-rebase => hooks--pre-rebase.sample} |    0
 ...commit-msg => hooks--prepare-commit-msg.sample} |    2 +-
 templates/{hooks--update => hooks--update.sample}  |    2 +-
 11 files changed, 16 insertions(+), 16 deletions(-)
 rename templates/{hooks--applypatch-msg => hooks--applypatch-msg.sample} (86%)
 mode change 100644 => 100755
 rename templates/{hooks--commit-msg => hooks--commit-msg.sample} (93%)
 mode change 100644 => 100755
 rename templates/{hooks--post-commit => hooks--post-commit.sample} (63%)
 mode change 100644 => 100755
 rename templates/{hooks--post-receive => hooks--post-receive.sample} (40%)
 mode change 100644 => 100755
 rename templates/{hooks--post-update => hooks--post-update.sample} (63%)
 mode change 100644 => 100755
 rename templates/{hooks--pre-applypatch => hooks--pre-applypatch.sample} (84%)
 mode change 100644 => 100755
 rename templates/{hooks--pre-commit => hooks--pre-commit.sample} (96%)
 mode change 100644 => 100755
 rename templates/{hooks--pre-rebase => hooks--pre-rebase.sample} (100%)
 mode change 100644 => 100755
 rename templates/{hooks--prepare-commit-msg => hooks--prepare-commit-msg.sample} (94%)
 mode change 100644 => 100755
 rename templates/{hooks--update => hooks--update.sample} (97%)
 mode change 100644 => 100755

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 4f06ae0..262a4f1 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -17,7 +17,8 @@ Hooks are little scripts you can place in `$GIT_DIR/hooks`
 directory to trigger action at certain points.  When
 `git-init` is run, a handful example hooks are copied in the
 `hooks` directory of the new repository, but by default they are
-all disabled.  To enable a hook, make it executable with `chmod +x`.
+all disabled.  To enable a hook, rename it by removing its `.sample`
+suffix.
 
 This document describes the currently defined hooks.
 
diff --git a/templates/hooks--applypatch-msg b/templates/hooks--applypatch-msg.sample
old mode 100644
new mode 100755
similarity index 86%
rename from templates/hooks--applypatch-msg
rename to templates/hooks--applypatch-msg.sample
index 02de1ef..8b2a2fe
--- a/templates/hooks--applypatch-msg
+++ b/templates/hooks--applypatch-msg.sample
@@ -7,7 +7,7 @@
 # appropriate message if it wants to stop the commit.  The hook is
 # allowed to edit the commit message file.
 #
-# To enable this hook, make this file executable.
+# To enable this hook, rename this file to "applypatch-msg".
 
 . git-sh-setup
 test -x "$GIT_DIR/hooks/commit-msg" &&
diff --git a/templates/hooks--commit-msg b/templates/hooks--commit-msg.sample
old mode 100644
new mode 100755
similarity index 93%
rename from templates/hooks--commit-msg
rename to templates/hooks--commit-msg.sample
index 4ef86eb..6ef1d29
--- a/templates/hooks--commit-msg
+++ b/templates/hooks--commit-msg.sample
@@ -6,7 +6,7 @@
 # status after issuing an appropriate message if it wants to stop the
 # commit.  The hook is allowed to edit the commit message file.
 #
-# To enable this hook, make this file executable.
+# To enable this hook, rename this file to "commit-msg".
 
 # Uncomment the below to add a Signed-off-by line to the message.
 # Doing this in a hook is a bad idea in general, but the prepare-commit-msg
diff --git a/templates/hooks--post-commit b/templates/hooks--post-commit.sample
old mode 100644
new mode 100755
similarity index 63%
rename from templates/hooks--post-commit
rename to templates/hooks--post-commit.sample
index 8be6f34..2266821
--- a/templates/hooks--post-commit
+++ b/templates/hooks--post-commit.sample
@@ -3,6 +3,6 @@
 # An example hook script that is called after a successful
 # commit is made.
 #
-# To enable this hook, make this file executable.
+# To enable this hook, rename this file to "post-commit".
 
 : Nothing
diff --git a/templates/hooks--post-receive b/templates/hooks--post-receive.sample
old mode 100644
new mode 100755
similarity index 40%
rename from templates/hooks--post-receive
rename to templates/hooks--post-receive.sample
index b70c8fd..18d2e0f
--- a/templates/hooks--post-receive
+++ b/templates/hooks--post-receive.sample
@@ -1,16 +1,15 @@
 #!/bin/sh
 #
-# An example hook script for the post-receive event
+# An example hook script for the "post-receive" event.
 #
-# This script is run after receive-pack has accepted a pack and the
-# repository has been updated.  It is passed arguments in through stdin
-# in the form
+# The "post-receive" script is run after receive-pack has accepted a pack
+# and the repository has been updated.  It is passed arguments in through
+# stdin in the form
 #  <oldrev> <newrev> <refname>
 # For example:
 #  aa453216d1b3e49e7f6f98441fa56946ddcd6a20 68f7abf4e6f922807889f52bc043ecd31b79f814 refs/heads/master
 #
-# see contrib/hooks/ for an sample, or uncomment the next line (on debian)
-#
-
+# see contrib/hooks/ for an sample, or uncomment the next line and
+# rename the file to "post-receive".
 
 #. /usr/share/doc/git-core/contrib/hooks/post-receive-email
diff --git a/templates/hooks--post-update b/templates/hooks--post-update.sample
old mode 100644
new mode 100755
similarity index 63%
rename from templates/hooks--post-update
rename to templates/hooks--post-update.sample
index bcba893..5323b56
--- a/templates/hooks--post-update
+++ b/templates/hooks--post-update.sample
@@ -3,6 +3,6 @@
 # An example hook script to prepare a packed repository for use over
 # dumb transports.
 #
-# To enable this hook, make this file executable by "chmod +x post-update".
+# To enable this hook, rename this file to "post-update".
 
 exec git-update-server-info
diff --git a/templates/hooks--pre-applypatch b/templates/hooks--pre-applypatch.sample
old mode 100644
new mode 100755
similarity index 84%
rename from templates/hooks--pre-applypatch
rename to templates/hooks--pre-applypatch.sample
index eeccc93..b1f187c
--- a/templates/hooks--pre-applypatch
+++ b/templates/hooks--pre-applypatch.sample
@@ -6,7 +6,7 @@
 # The hook should exit with non-zero status after issuing an
 # appropriate message if it wants to stop the commit.
 #
-# To enable this hook, make this file executable.
+# To enable this hook, rename this file to "pre-applypatch".
 
 . git-sh-setup
 test -x "$GIT_DIR/hooks/pre-commit" &&
diff --git a/templates/hooks--pre-commit b/templates/hooks--pre-commit.sample
old mode 100644
new mode 100755
similarity index 96%
rename from templates/hooks--pre-commit
rename to templates/hooks--pre-commit.sample
index b25dce6..71c10f2
--- a/templates/hooks--pre-commit
+++ b/templates/hooks--pre-commit.sample
@@ -5,7 +5,7 @@
 # exit with non-zero status after issuing an appropriate message if
 # it wants to stop the commit.
 #
-# To enable this hook, make this file executable.
+# To enable this hook, rename this file to "pre-commit".
 
 # This is slightly modified from Andrew Morton's Perfect Patch.
 # Lines you introduce should not have trailing whitespace.
diff --git a/templates/hooks--pre-rebase b/templates/hooks--pre-rebase.sample
old mode 100644
new mode 100755
similarity index 100%
rename from templates/hooks--pre-rebase
rename to templates/hooks--pre-rebase.sample
diff --git a/templates/hooks--prepare-commit-msg b/templates/hooks--prepare-commit-msg.sample
old mode 100644
new mode 100755
similarity index 94%
rename from templates/hooks--prepare-commit-msg
rename to templates/hooks--prepare-commit-msg.sample
index d3c1da3..aa42acf
--- a/templates/hooks--prepare-commit-msg
+++ b/templates/hooks--prepare-commit-msg.sample
@@ -7,7 +7,7 @@
 # message file.  If the hook fails with a non-zero status,
 # the commit is aborted.
 #
-# To enable this hook, make this file executable.
+# To enable this hook, rename this file to "prepare-commit-msg".
 
 # This hook includes three examples.  The first comments out the
 # "Conflicts:" part of a merge commit.
diff --git a/templates/hooks--update b/templates/hooks--update.sample
old mode 100644
new mode 100755
similarity index 97%
rename from templates/hooks--update
rename to templates/hooks--update.sample
index 4b69268..93c6055
--- a/templates/hooks--update
+++ b/templates/hooks--update.sample
@@ -3,7 +3,7 @@
 # An example hook script to blocks unannotated tags from entering.
 # Called by git-receive-pack with arguments: refname sha1-old sha1-new
 #
-# To enable this hook, make this file executable by "chmod +x update".
+# To enable this hook, rename this file to "update".
 #
 # Config
 # ------
-- 
1.5.6.56.g29b0d

Re: [PATCH] Ship sample hooks with .sample suffix

[Junio C Hamano]

I'll queue the patch to 'master' and I suspect that this will make 31d6632
(Windows: Change the name of hook scripts to make them not executable.,
2007-11-30) unnecessary.

I've looked at diff between ee1e0a3 (the original "pull" request) and
8fd39ba (after WS fixup on borrowed code and other changes).

They all looked fine except for git.c::main() where do .. while does not
have a { } around the body (which is not required by C language but it
just looked funny).

I am very tempted to pull MinGW series directly to 'master', or at least
to 'next', perhaps after dropping 31d6632.  What's your confidence level
on the series?  I think we should give this series extra priority so that
any potential issues with other series (like dr/ceil) that do pathname
traversal will become more apparent sooner rather than later.

Re: [PATCH] Ship sample hooks with .sample suffix

[Johannes Sixt]

Zitat von Junio C Hamano <gitster@pobox.com>:
> I am very tempted to pull MinGW series directly to 'master', or at least
> to 'next', perhaps after dropping 31d6632.  What's your confidence level
> on the series?  I think we should give this series extra priority so that
> any potential issues with other series (like dr/ceil) that do pathname
> traversal will become more apparent sooner rather than later.

I am very confident in the series as far as using it on Windows is concerned.
But I must admit that I have started to use it on Linux only recently.
Therefore, cooking it in 'next' for a few days is probably not a mistake.

-- Hannes

Re: [PATCH] Ship sample hooks with .sample suffix

[Junio C Hamano]

Johannes Sixt <johannes.sixt@telecom.at> writes:

> I am very confident in the series as far as using it on Windows is concerned.
> But I must admit that I have started to use it on Linux only recently.
> Therefore, cooking it in 'next' for a few days is probably not a mistake.

The question obviously was about using outside Windows context as we have
been talking about merging it into my history ;-)

I'll push out the master with the *.sample patch hopefully soon.

Re: [PATCH] Ship sample hooks with .sample suffix

[Johannes Sixt]

Junio C Hamano schrieb:
> I'll queue the patch to 'master' and I suspect that this will make 31d6632
> (Windows: Change the name of hook scripts to make them not executable.,
> 2007-11-30) unnecessary.

I've updated

git://repo.or.cz/git/mingw/j6t.git for-junio

and the interdiff to the previous version is merely the reverse of
31d6632. For your convenience I've merged it into current next:

git://repo.or.cz/git/mingw/j6t.git merge-mingw-into-next

-- Hannes

Re: [PATCH] Ship sample hooks with .sample suffix

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Junio C Hamano schrieb:
>> I'll queue the patch to 'master' and I suspect that this will make 31d6632
>> (Windows: Change the name of hook scripts to make them not executable.,
>> 2007-11-30) unnecessary.
>
> I've updated
>
> git://repo.or.cz/git/mingw/j6t.git for-junio
>
> and the interdiff to the previous version is merely the reverse of
> 31d6632. For your convenience I've merged it into current next:
>
> git://repo.or.cz/git/mingw/j6t.git merge-mingw-into-next

Thanks.  This round I am looking forward to very much being able to merge
this to 'next' for real, not just parking in 'pu', but it is getting late
so it may have to wait til tomorrow.

Re: [PATCH] Ship sample hooks with .sample suffix

[Peter Baumann]

On Tue, Jun 24, 2008 at 07:09:03PM -0700, Junio C Hamano wrote:
> We used to mark hooks we ship as samples by making them unexecutable, but
> some filesystems cannot tell what is executable and what is not.
> 
> This makes it much more explicit.  The hooks are suffixed with .sample
> (but now are made executable), so enabling it is still one step operation
> (instead of "chmod +x $hook", you would do "mv $hook.sample $hook") but
> now they won't get accidentally enabled on systems without executable bit.
> 

Wouldn't it be better to name the hooks $hook.deactivated so its obvious
to anybody that they are not executed? Just my 2 cents.

-Peter