* [PATCH 0/2] pre-commit hook updates
@ 2014-11-25 22:51 Øystein Walle
[not found] ` <cover.1416955873.git.oystwa@gmail.com>
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Øystein Walle @ 2014-11-25 22:51 UTC (permalink / raw)
To: git; +Cc: Ø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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] t7503: use write_script to generate hook scripts
[not found] ` <cover.1416955873.git.oystwa@gmail.com>
@ 2014-11-25 22:51 ` Øystein Walle
2014-11-26 1:25 ` Eric Sunshine
2014-11-26 4:51 ` Jeff King
2014-11-25 22:51 ` [PATCH 2/2] commit: inform pre-commit that --amend was used Øystein Walle
1 sibling, 2 replies; 13+ messages in thread
From: Øystein Walle @ 2014-11-25 22:51 UTC (permalink / raw)
To: git; +Cc: Ø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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] commit: inform pre-commit that --amend was used
[not found] ` <cover.1416955873.git.oystwa@gmail.com>
2014-11-25 22:51 ` [PATCH 1/2] t7503: use write_script to generate hook scripts Øystein Walle
@ 2014-11-25 22:51 ` Øystein Walle
2014-11-26 1:36 ` Eric Sunshine
1 sibling, 1 reply; 13+ messages in thread
From: Øystein Walle @ 2014-11-25 22:51 UTC (permalink / raw)
To: git; +Cc: Ø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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] t7503: use write_script to generate hook scripts
2014-11-25 22:51 ` [PATCH 1/2] t7503: use write_script to generate hook scripts Øystein Walle
@ 2014-11-26 1:25 ` Eric Sunshine
2014-11-26 4:51 ` Jeff King
1 sibling, 0 replies; 13+ messages in thread
From: Eric Sunshine @ 2014-11-26 1:25 UTC (permalink / raw)
To: Øystein Walle; +Cc: Git List
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] pre-commit hook updates
2014-11-25 22:51 [PATCH 0/2] pre-commit hook updates Øystein Walle
[not found] ` <cover.1416955873.git.oystwa@gmail.com>
@ 2014-11-26 1:32 ` Eric Sunshine
2014-11-26 4:52 ` Jeff King
2 siblings, 0 replies; 13+ messages in thread
From: Eric Sunshine @ 2014-11-26 1:32 UTC (permalink / raw)
To: Øystein Walle; +Cc: Git List
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] commit: inform pre-commit that --amend was used
2014-11-25 22:51 ` [PATCH 2/2] commit: inform pre-commit that --amend was used Øystein Walle
@ 2014-11-26 1:36 ` Eric Sunshine
0 siblings, 0 replies; 13+ messages in thread
From: Eric Sunshine @ 2014-11-26 1:36 UTC (permalink / raw)
To: Øystein Walle; +Cc: Git List
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] t7503: use write_script to generate hook scripts
2014-11-25 22:51 ` [PATCH 1/2] t7503: use write_script to generate hook scripts Øystein Walle
2014-11-26 1:25 ` Eric Sunshine
@ 2014-11-26 4:51 ` Jeff King
2014-11-26 18:12 ` Junio C Hamano
1 sibling, 1 reply; 13+ messages in thread
From: Jeff King @ 2014-11-26 4:51 UTC (permalink / raw)
To: Øystein Walle; +Cc: git
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] pre-commit hook updates
2014-11-25 22:51 [PATCH 0/2] pre-commit hook updates Øystein Walle
[not found] ` <cover.1416955873.git.oystwa@gmail.com>
2014-11-26 1:32 ` [PATCH 0/2] pre-commit hook updates Eric Sunshine
@ 2014-11-26 4:52 ` Jeff King
2014-11-26 18:35 ` Junio C Hamano
2 siblings, 1 reply; 13+ messages in thread
From: Jeff King @ 2014-11-26 4:52 UTC (permalink / raw)
To: Øystein Walle; +Cc: git
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] t7503: use write_script to generate hook scripts
2014-11-26 4:51 ` Jeff King
@ 2014-11-26 18:12 ` Junio C Hamano
2014-11-26 19:03 ` Jeff King
0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2014-11-26 18:12 UTC (permalink / raw)
To: Jeff King; +Cc: Øystein Walle, git
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] pre-commit hook updates
2014-11-26 4:52 ` Jeff King
@ 2014-11-26 18:35 ` Junio C Hamano
2014-11-26 18:56 ` Jeff King
0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2014-11-26 18:35 UTC (permalink / raw)
To: Jeff King; +Cc: Øystein Walle, git
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] pre-commit hook updates
2014-11-26 18:35 ` Junio C Hamano
@ 2014-11-26 18:56 ` Jeff King
0 siblings, 0 replies; 13+ messages in thread
From: Jeff King @ 2014-11-26 18:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Øystein Walle, git
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] t7503: use write_script to generate hook scripts
2014-11-26 18:12 ` Junio C Hamano
@ 2014-11-26 19:03 ` Jeff King
2014-11-26 20:07 ` Junio C Hamano
0 siblings, 1 reply; 13+ messages in thread
From: Jeff King @ 2014-11-26 19:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Øystein Walle, git
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] t7503: use write_script to generate hook scripts
2014-11-26 19:03 ` Jeff King
@ 2014-11-26 20:07 ` Junio C Hamano
0 siblings, 0 replies; 13+ messages in thread
From: Junio C Hamano @ 2014-11-26 20:07 UTC (permalink / raw)
To: Jeff King; +Cc: Øystein Walle, git
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-11-26 20:07 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-11-25 22:51 [PATCH 0/2] pre-commit hook updates Øystein Walle
[not found] ` <cover.1416955873.git.oystwa@gmail.com>
2014-11-25 22:51 ` [PATCH 1/2] t7503: use write_script to generate hook scripts Øystein Walle
2014-11-26 1:25 ` Eric Sunshine
2014-11-26 4:51 ` Jeff King
2014-11-26 18:12 ` Junio C Hamano
2014-11-26 19:03 ` Jeff King
2014-11-26 20:07 ` Junio C Hamano
2014-11-25 22:51 ` [PATCH 2/2] commit: inform pre-commit that --amend was used Øystein Walle
2014-11-26 1:36 ` Eric Sunshine
2014-11-26 1:32 ` [PATCH 0/2] pre-commit hook updates Eric Sunshine
2014-11-26 4:52 ` Jeff King
2014-11-26 18:35 ` Junio C Hamano
2014-11-26 18:56 ` Jeff King
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.