[PATCH 0/2] pre-commit hook updates

[PATCH 0/2] pre-commit hook updates

[Øystein Walle]

The first patch changes t/t7503-pre-commit-hook.sh to use write_script
everywhere, as was suggested by Jeff King in the discussion of the
previous patch.

The second patch is v2 of the patch I sent earlier. I've incorporated
Eric Sunshine's suggestions. I didn't do enough digging; I found
test_expect_failure and assumed this was test_expect_success's twin
brother, but it marked stuff as known breakages so I went with the '!'.
I also found it a bit strange that test_must_fail has a different
signature (to the extent a shell function has one at all). Is my use of
test_must_fail correct?

I agree with Junio Hamano that it's better to provide no argument at all
rather than an empty one. I also agree with Jeff King that "noamend" is
better than an empty argument. I went with the second one since Jeff
seemed to get the last word :)

I'm not sure I like the ternary inside the function call like that, but
I went with it because it gave the smallest footprint (which is probably
not a good argument). I suppose I could have done:

if (amend)
	hook_arg1 = "amend"
else
	hook_arg1 = "noamend"
...
... run_commit_hook(use_editor, index_file, "pre-commit", hook_arg1, NULL);

or create a hook_amend variable.

I'm happy to send a v3.

Øystein Walle (2):
  t7503: use write_script to generate hook scripts
  commit: inform pre-commit that --amend was used

 Documentation/githooks.txt |  3 ++-
 builtin/commit.c           |  3 ++-
 t/t7503-pre-commit-hook.sh | 20 ++++++++++++++------
 3 files changed, 18 insertions(+), 8 deletions(-)

-- 
2.2.0.rc2.23.gca0107e

[PATCH 1/2] t7503: use write_script to generate hook scripts

[Øystein Walle]

Signed-off-by: Øystein Walle <oystwa@gmail.com>
---
 t/t7503-pre-commit-hook.sh | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh
index 984889b..99ed967 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-hook.sh
@@ -24,8 +24,7 @@ test_expect_success '--no-verify with no hook' '
 HOOKDIR="$(git rev-parse --git-dir)/hooks"
 HOOK="$HOOKDIR/pre-commit"
 mkdir -p "$HOOKDIR"
-cat > "$HOOK" <<EOF
-#!/bin/sh
+write_script "$HOOK" <<EOF
 exit 0
 EOF
 chmod +x "$HOOK"
@@ -47,8 +46,7 @@ test_expect_success '--no-verify with succeeding hook' '
 '
 
 # now a hook that fails
-cat > "$HOOK" <<EOF
-#!/bin/sh
+write_script "$HOOK" <<EOF
 exit 1
 EOF
 
@@ -88,8 +86,7 @@ chmod +x "$HOOK"
 
 # a hook that checks $GIT_PREFIX and succeeds inside the
 # success/ subdirectory only
-cat > "$HOOK" <<EOF
-#!/bin/sh
+write_script "$HOOK" <<EOF
 test \$GIT_PREFIX = success/
 EOF
 
-- 
2.2.0.rc3

Re: [PATCH 1/2] t7503: use write_script to generate hook scripts

[Eric Sunshine]

On Tue, Nov 25, 2014 at 5:51 PM, Øystein Walle <oystwa@gmail.com> wrote:
> Signed-off-by: Øystein Walle <oystwa@gmail.com>
> ---
>  t/t7503-pre-commit-hook.sh | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh
> index 984889b..99ed967 100755
> --- a/t/t7503-pre-commit-hook.sh
> +++ b/t/t7503-pre-commit-hook.sh
> @@ -24,8 +24,7 @@ test_expect_success '--no-verify with no hook' '
>  HOOKDIR="$(git rev-parse --git-dir)/hooks"
>  HOOK="$HOOKDIR/pre-commit"
>  mkdir -p "$HOOKDIR"
> -cat > "$HOOK" <<EOF
> -#!/bin/sh
> +write_script "$HOOK" <<EOF
>  exit 0
>  EOF
>  chmod +x "$HOOK"

You can drop the 'chmod' too since write_script does that for you.

> @@ -47,8 +46,7 @@ test_expect_success '--no-verify with succeeding hook' '
>  '
>
>  # now a hook that fails
> -cat > "$HOOK" <<EOF
> -#!/bin/sh
> +write_script "$HOOK" <<EOF
>  exit 1
>  EOF
>
> @@ -88,8 +86,7 @@ chmod +x "$HOOK"
>
>  # a hook that checks $GIT_PREFIX and succeeds inside the
>  # success/ subdirectory only
> -cat > "$HOOK" <<EOF
> -#!/bin/sh
> +write_script "$HOOK" <<EOF
>  test \$GIT_PREFIX = success/
>  EOF
>
> --
> 2.2.0.rc3

Re: [PATCH 1/2] t7503: use write_script to generate hook scripts

[Jeff King]

On Tue, Nov 25, 2014 at 11:51:28PM +0100, Øystein Walle wrote:

> Signed-off-by: Øystein Walle <oystwa@gmail.com>
> ---
>  t/t7503-pre-commit-hook.sh | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh
> index 984889b..99ed967 100755
> --- a/t/t7503-pre-commit-hook.sh
> +++ b/t/t7503-pre-commit-hook.sh
> @@ -24,8 +24,7 @@ test_expect_success '--no-verify with no hook' '
>  HOOKDIR="$(git rev-parse --git-dir)/hooks"
>  HOOK="$HOOKDIR/pre-commit"
>  mkdir -p "$HOOKDIR"
> -cat > "$HOOK" <<EOF
> -#!/bin/sh
> +write_script "$HOOK" <<EOF

While you are touching this line, please make it "<<\EOF". It does not
matter for these simple cases, but as a style, we try to avoid
interpolation unless it is necessary.

-Peff

Re: [PATCH 1/2] t7503: use write_script to generate hook scripts

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Nov 25, 2014 at 11:51:28PM +0100, Øystein Walle wrote:
>
>> Signed-off-by: Øystein Walle <oystwa@gmail.com>
>> ---
>>  t/t7503-pre-commit-hook.sh | 9 +++------
>>  1 file changed, 3 insertions(+), 6 deletions(-)
>> 
>> diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh
>> index 984889b..99ed967 100755
>> --- a/t/t7503-pre-commit-hook.sh
>> +++ b/t/t7503-pre-commit-hook.sh
>> @@ -24,8 +24,7 @@ test_expect_success '--no-verify with no hook' '
>>  HOOKDIR="$(git rev-parse --git-dir)/hooks"
>>  HOOK="$HOOKDIR/pre-commit"
>>  mkdir -p "$HOOKDIR"
>> -cat > "$HOOK" <<EOF
>> -#!/bin/sh
>> +write_script "$HOOK" <<EOF
>
> While you are touching this line, please make it "<<\EOF". It does not
> matter for these simple cases, but as a style, we try to avoid
> interpolation unless it is necessary.

Thanks.  It is more about reducing cognitive burden from the
readers.  An unquoted <<EOF signals you that your eyes need to scan
carefully for $subsitutions to understand what is going on, instead
of coasting it over.

Re: [PATCH 1/2] t7503: use write_script to generate hook scripts

[Jeff King]

On Wed, Nov 26, 2014 at 10:12:08AM -0800, Junio C Hamano wrote:

> >> +write_script "$HOOK" <<EOF
> >
> > While you are touching this line, please make it "<<\EOF". It does not
> > matter for these simple cases, but as a style, we try to avoid
> > interpolation unless it is necessary.
> 
> Thanks.  It is more about reducing cognitive burden from the
> readers.  An unquoted <<EOF signals you that your eyes need to scan
> carefully for $subsitutions to understand what is going on, instead
> of coasting it over.

While we are talking about it, do we have a style preference on
always/never using "<<-" unless necessary? I do not think it is as
important as preventing interpolation, because it does not introduce a
cognitive load in the same way. But consistently using "<<-" is one less
thing for shell newbies to be confused by, and to get wrong when
cargo-culting.

-Peff

Re: [PATCH 1/2] t7503: use write_script to generate hook scripts

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Nov 26, 2014 at 10:12:08AM -0800, Junio C Hamano wrote:
>
>> >> +write_script "$HOOK" <<EOF
>> >
>> > While you are touching this line, please make it "<<\EOF". It does not
>> > matter for these simple cases, but as a style, we try to avoid
>> > interpolation unless it is necessary.
>> 
>> Thanks.  It is more about reducing cognitive burden from the
>> readers.  An unquoted <<EOF signals you that your eyes need to scan
>> carefully for $subsitutions to understand what is going on, instead
>> of coasting it over.
>
> While we are talking about it, do we have a style preference on
> always/never using "<<-" unless necessary? I do not think it is as
> important as preventing interpolation, because it does not introduce a
> cognitive load in the same way. But consistently using "<<-" is one less
> thing for shell newbies to be confused by, and to get wrong when
> cargo-culting.

Not using "<<-" is a sign of another problem that is bigger, namely,
the test vector is prepared outside of a test_expect_success setup
block.  Inside test_expect_success, I'd prefer "<<-" to make it
clear where each test begins and next test begins, which would make
it easier for our to coast over.

[PATCH 2/2] commit: inform pre-commit that --amend was used

[Øystein Walle]

When a commit is amended a pre-commit hook that verifies the commit's
contents might not find what it's looking for if it looks at the
differences against HEAD when HEAD~1 might be more appropriate. Inform
the commit hook that --amend is being used so that hook authors can do
e.g.

    if test "$1" = amend; then
        ...
    else
        ...
    fi

to handle these situations.

Signed-off-by: Øystein Walle <oystwa@gmail.com>
---
 Documentation/githooks.txt |  3 ++-
 builtin/commit.c           |  3 ++-
 t/t7503-pre-commit-hook.sh | 11 +++++++++++
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 9ef2469..fb3e71e 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -73,7 +73,8 @@ pre-commit
 ~~~~~~~~~~
 
 This hook is invoked by 'git commit', and can be bypassed
-with `--no-verify` option.  It takes no parameter, and is
+with `--no-verify` option.  It takes one parameter which is "amend" if
+`--amend` was used when committing and "noamend" if not. It is
 invoked before obtaining the proposed commit log message and
 making a commit.  Exiting with non-zero status from this script
 causes the 'git commit' to abort.
diff --git a/builtin/commit.c b/builtin/commit.c
index e108c53..f9e5d27 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -694,7 +694,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	/* This checks and barfs if author is badly specified */
 	determine_author_info(author_ident);
 
-	if (!no_verify && run_commit_hook(use_editor, index_file, "pre-commit", NULL))
+	if (!no_verify && run_commit_hook(use_editor, index_file, "pre-commit",
+					  amend ? "amend" : "noamend", NULL))
 		return 0;
 
 	if (squash_message) {
diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh
index 99ed967..b400afe 100755
--- a/t/t7503-pre-commit-hook.sh
+++ b/t/t7503-pre-commit-hook.sh
@@ -133,4 +133,15 @@ test_expect_success 'check the author in hook' '
 	git show -s
 '
 
+# a hook that checks if "amend" is passed as an argument
+write_script "$HOOK" <<EOF
+test "\$1" = amend
+EOF
+
+test_expect_success 'check that "amend" argument is given' '
+       git commit --amend --allow-empty
+'
+
+test_must_fail git commit --allow-empty
+
 test_done
-- 
2.2.0.rc3

Re: [PATCH 2/2] commit: inform pre-commit that --amend was used

[Eric Sunshine]

On Tue, Nov 25, 2014 at 5:51 PM, Øystein Walle <oystwa@gmail.com> wrote:
> When a commit is amended a pre-commit hook that verifies the commit's
> contents might not find what it's looking for if it looks at the
> differences against HEAD when HEAD~1 might be more appropriate. Inform
> the commit hook that --amend is being used so that hook authors can do
> e.g.
>
>     if test "$1" = amend; then
>         ...
>     else
>         ...
>     fi
>
> to handle these situations.
>
> Signed-off-by: Øystein Walle <oystwa@gmail.com>
> ---
> diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh
> index 99ed967..b400afe 100755
> --- a/t/t7503-pre-commit-hook.sh
> +++ b/t/t7503-pre-commit-hook.sh
> @@ -133,4 +133,15 @@ test_expect_success 'check the author in hook' '
>         git show -s
>  '
>
> +# a hook that checks if "amend" is passed as an argument
> +write_script "$HOOK" <<EOF
> +test "\$1" = amend
> +EOF
> +
> +test_expect_success 'check that "amend" argument is given' '
> +       git commit --amend --allow-empty
> +'
> +
> +test_must_fail git commit --allow-empty

test_must_fail is a direct replacement for '!' when invoking a git
command which should fail _within_ a test. Your original version of
this test was in the previous round:

    test_expect_success 'check that "amend" argument is NOT given' '
        ! git commit --allow-empty
    '

Just replace '!', like this:

    test_expect_success 'check that "amend" argument is NOT given' '
        test_must_fail git commit --allow-empty
    '

> +
>  test_done
> --
> 2.2.0.rc3

Re: [PATCH 0/2] pre-commit hook updates

[Eric Sunshine]

On Tue, Nov 25, 2014 at 5:51 PM, Øystein Walle <oystwa@gmail.com> wrote:
> The first patch changes t/t7503-pre-commit-hook.sh to use write_script
> everywhere, as was suggested by Jeff King in the discussion of the
> previous patch.
>
> The second patch is v2 of the patch I sent earlier. I've incorporated
> Eric Sunshine's suggestions. I didn't do enough digging; I found
> test_expect_failure and assumed this was test_expect_success's twin
> brother, but it marked stuff as known breakages so I went with the '!'.
> I also found it a bit strange that test_must_fail has a different
> signature (to the extent a shell function has one at all). Is my use of
> test_must_fail correct?

Your use is not correct (as I'll explain when responding to the patch).

> I agree with Junio Hamano that it's better to provide no argument at all
> rather than an empty one. I also agree with Jeff King that "noamend" is
> better than an empty argument. I went with the second one since Jeff
> seemed to get the last word :)

For what it's worth (probably not much), I agree with Junio.

> I'm not sure I like the ternary inside the function call like that, but
> I went with it because it gave the smallest footprint (which is probably
> not a good argument). I suppose I could have done:
>
> if (amend)
>         hook_arg1 = "amend"
> else
>         hook_arg1 = "noamend"
> ...
> ... run_commit_hook(use_editor, index_file, "pre-commit", hook_arg1, NULL);
>
> or create a hook_amend variable.
>
> I'm happy to send a v3.

Re: [PATCH 0/2] pre-commit hook updates

[Jeff King]

On Tue, Nov 25, 2014 at 11:51:27PM +0100, Øystein Walle wrote:

> I agree with Junio Hamano that it's better to provide no argument at all
> rather than an empty one. I also agree with Jeff King that "noamend" is
> better than an empty argument. I went with the second one since Jeff
> seemed to get the last word :)

I am not sure the last word counts for much. :) We'll see if Junio
responds (there, or to your patch). I do not feel _too_ strongly either
way, and I don't have much else to say besides what was said.

-Peff

Re: [PATCH 0/2] pre-commit hook updates

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Nov 25, 2014 at 11:51:27PM +0100, Øystein Walle wrote:
>
>> I agree with Junio Hamano that it's better to provide no argument at all
>> rather than an empty one. I also agree with Jeff King that "noamend" is
>> better than an empty argument. I went with the second one since Jeff
>> seemed to get the last word :)
>
> I am not sure the last word counts for much. :) We'll see if Junio
> responds (there, or to your patch). I do not feel _too_ strongly either
> way, and I don't have much else to say besides what was said.

I _think_ "give only info that is necessary" is cleaner as an
interface in theory, but have two niggles myself:

 1. the hooks must do the "argument parsing" loop (you already
    mentioned this);

 2. the hooks cannot tell if the lack of "amending" argument is
    because the version of Git predates that "amending" hint
    support, or because the user action is a straight "commit" not
    an "commit --amend".

In any case, I do not have strong preference myself.

Re: [PATCH 0/2] pre-commit hook updates

[Jeff King]

On Wed, Nov 26, 2014 at 10:35:22AM -0800, Junio C Hamano wrote:

> I _think_ "give only info that is necessary" is cleaner as an
> interface in theory, but have two niggles myself:
> 
>  1. the hooks must do the "argument parsing" loop (you already
>     mentioned this);
> 
>  2. the hooks cannot tell if the lack of "amending" argument is
>     because the version of Git predates that "amending" hint
>     support, or because the user action is a straight "commit" not
>     an "commit --amend".
> 
> In any case, I do not have strong preference myself.

That agrees with my thinking exactly.

At this point since both of us seem on the fence, I am happy to let
Øystein, as the person who is actually writing the patch, decide.

-Peff