pre-commit hook updates

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 :)

Quoted 4 lines · Øystein Walle

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.

Signed-off-by Øystein Walle <[email protected]>

---
 t/t7503-pre-commit-hook.sh | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

t/t7503-pre-commit-hook.sh +0 -0

index 984889b..99ed967 100755

Quoted 5 lines · Øystein Walle

Signed-off-by Øystein Walle <[email protected]>

---
 t/t7503-pre-commit-hook.sh | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

t/t7503-pre-commit-hook.sh test_expect_success '--no-verify with no hook' ' · line 27

Quoted 11 lines

t/t7503-pre-commit-hook.sh +1 -2

index 984889b..99ed967 100755
@@ -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.

Quoted 5 lines

Quoted 1 line

+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.

Quoted 10 lines · Junio C Hamano

Quoted 5 lines

Quoted 1 line

+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.

Signed-off-by Øystein Walle <[email protected]>

---
 t/t7503-pre-commit-hook.sh | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

t/t7503-pre-commit-hook.sh +0 -0

index 984889b..99ed967 100755

t/t7503-pre-commit-hook.sh +2 -5

@@ -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

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.

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 <[email protected]>

---

t/t7503-pre-commit-hook.sh +0 -0

index 99ed967..b400afe 100755
@@ -133,4 +133,15 @@ test_expect_success 'check the author in hook' '
        git show -s
 '
 

t/t7503-pre-commit-hook.sh +1 -1

+
 test_done
--

2.2.0.rc3