{"thread":{"id":"38050","subject":"[PATCH 0/2] pre-commit hook updates","startedAt":"2014-11-25T22:51:27Z","lastAt":"2014-11-26T20:07:35Z","messageCount":13,"participants":["Øystein Walle","Eric Sunshine","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"252545","messageId":"cover.1416953772.git.oystwa@gmail.com","threadId":"38050","inReplyTo":null,"subject":"[PATCH 0/2] pre-commit hook updates","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2014-11-25T22:51:27Z","receivedAt":"2014-11-25T22:51:27Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"The first patch changes t/t7503-pre-commit-hook.sh to use write_script\neverywhere, as was suggested by Jeff King in the discussion of the\nprevious patch.\n\nThe second patch is v2 of the patch I sent earlier. I've incorporated\nEric Sunshine's suggestions. I didn't do enough digging; I found\ntest_expect_failure and assumed this was test_expect_success's twin\nbrother, but it marked stuff as known breakages so I went with the '!'.\nI also found it a bit strange that test_must_fail has a different\nsignature (to the extent a shell function has one at all). Is my use of\ntest_must_fail correct?\n\nI agree with Junio Hamano that it's better to provide no argument at all\nrather than an empty one. I also agree with Jeff King that \"noamend\" is\nbetter than an empty argument. I went with the second one since Jeff\nseemed to get the last word :)\n\nI'm not sure I like the ternary inside the function call like that, but\nI went with it because it gave the smallest footprint (which is probably\nnot a good argument). I suppose I could have done:\n\nif (amend)\n\thook_arg1 = \"amend\"\nelse\n\thook_arg1 = \"noamend\"\n...\n... run_commit_hook(use_editor, index_file, \"pre-commit\", hook_arg1, NULL);\n\nor create a hook_amend variable.\n\nI'm happy to send a v3.\n\nØystein Walle (2):\n t7503: use write_script to generate hook scripts\n commit: inform pre-commit that --amend was used\n\n Documentation/githooks.txt | 3 ++-\n builtin/commit.c | 3 ++-\n t/t7503-pre-commit-hook.sh | 20 ++++++++++++++------\n 3 files changed, 18 insertions(+), 8 deletions(-)\n\n-- \n2.2.0.rc2.23.gca0107e\n"},{"id":"252546","messageId":"78f25aaa60554f7e3b917c565df0f89fb9c08921.1416955873.git.oystwa@gmail.com","threadId":"38050","inReplyTo":"cover.1416953772.git.oystwa@gmail.com","subject":"[PATCH 1/2] t7503: use write_script to generate hook scripts","fromName":"Øystein Walle","fromEmail":"oystwa@gmail.com","sentAt":"2014-11-25T22:51:28Z","receivedAt":"2014-11-25T22:51:28Z","isPatch":true,"sender":{"key":"oystwa@gmail.com","avatar":"https://avatars.githubusercontent.com/u/794585?v=4"},"body":"Signed-off-by: Øystein Walle \n---\n t/t7503-pre-commit-hook.sh | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh\nindex 984889b..99ed967 100755\n--- a/t/t7503-pre-commit-hook.sh\n+++ b/t/t7503-pre-commit-hook.sh\n@@ -24,8 +24,7 @@ test_expect_success '--no-verify with no hook' '\n HOOKDIR=\"$(git rev-parse --git-dir)/hooks\"\n HOOK=\"$HOOKDIR/pre-commit\"\n mkdir -p \"$HOOKDIR\"\n-cat > \"$HOOK\" < \"$HOOK\" < \"$HOOK\" <\n---\n Documentation/githooks.txt | 3 ++-\n builtin/commit.c | 3 ++-\n t/t7503-pre-commit-hook.sh | 11 +++++++++++\n 3 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 9ef2469..fb3e71e 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -73,7 +73,8 @@ pre-commit\n ~~~~~~~~~~\n \n This hook is invoked by 'git commit', and can be bypassed\n-with `--no-verify` option. It takes no parameter, and is\n+with `--no-verify` option. It takes one parameter which is \"amend\" if\n+`--amend` was used when committing and \"noamend\" if not. It is\n invoked before obtaining the proposed commit log message and\n making a commit. Exiting with non-zero status from this script\n causes the 'git commit' to abort.\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e108c53..f9e5d27 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -694,7 +694,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t/* This checks and barfs if author is badly specified */\n \tdetermine_author_info(author_ident);\n \n-\tif (!no_verify && run_commit_hook(use_editor, index_file, \"pre-commit\", NULL))\n+\tif (!no_verify && run_commit_hook(use_editor, index_file, \"pre-commit\",\n+\t\t\t\t\t amend ? \"amend\" : \"noamend\", NULL))\n \t\treturn 0;\n \n \tif (squash_message) {\ndiff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh\nindex 99ed967..b400afe 100755\n--- a/t/t7503-pre-commit-hook.sh\n+++ b/t/t7503-pre-commit-hook.sh\n@@ -133,4 +133,15 @@ test_expect_success 'check the author in hook' '\n \tgit show -s\n '\n \n+# a hook that checks if \"amend\" is passed as an argument\n+write_script \"$HOOK\" < wrote:\n> Signed-off-by: Øystein Walle \n> ---\n> t/t7503-pre-commit-hook.sh | 9 +++------\n> 1 file changed, 3 insertions(+), 6 deletions(-)\n>\n> diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh\n> index 984889b..99ed967 100755\n> --- a/t/t7503-pre-commit-hook.sh\n> +++ b/t/t7503-pre-commit-hook.sh\n> @@ -24,8 +24,7 @@ test_expect_success '--no-verify with no hook' '\n> HOOKDIR=\"$(git rev-parse --git-dir)/hooks\"\n> HOOK=\"$HOOKDIR/pre-commit\"\n> mkdir -p \"$HOOKDIR\"\n> -cat > \"$HOOK\" < -#!/bin/sh\n> +write_script \"$HOOK\" < exit 0\n> EOF\n> chmod +x \"$HOOK\"\n\nYou can drop the 'chmod' too since write_script does that for you.\n\n> @@ -47,8 +46,7 @@ test_expect_success '--no-verify with succeeding hook' '\n> '\n>\n> # now a hook that fails\n> -cat > \"$HOOK\" < -#!/bin/sh\n> +write_script \"$HOOK\" < exit 1\n> EOF\n>\n> @@ -88,8 +86,7 @@ chmod +x \"$HOOK\"\n>\n> # a hook that checks $GIT_PREFIX and succeeds inside the\n> # success/ subdirectory only\n> -cat > \"$HOOK\" < -#!/bin/sh\n> +write_script \"$HOOK\" < test \\$GIT_PREFIX = success/\n> EOF\n>\n> --\n> 2.2.0.rc3\n"},{"id":"252562","messageId":"CAPig+cS76GWES3hXF4Bdjg5G7QFcTKhzLyjM55LPMSD0zEinPQ@mail.gmail.com","threadId":"38050","inReplyTo":"cover.1416953772.git.oystwa@gmail.com","subject":"Re: [PATCH 0/2] pre-commit hook updates","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-11-26T01:32:27Z","receivedAt":"2014-11-26T01:32:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 25, 2014 at 5:51 PM, Øystein Walle wrote:\n> The first patch changes t/t7503-pre-commit-hook.sh to use write_script\n> everywhere, as was suggested by Jeff King in the discussion of the\n> previous patch.\n>\n> The second patch is v2 of the patch I sent earlier. I've incorporated\n> Eric Sunshine's suggestions. I didn't do enough digging; I found\n> test_expect_failure and assumed this was test_expect_success's twin\n> brother, but it marked stuff as known breakages so I went with the '!'.\n> I also found it a bit strange that test_must_fail has a different\n> signature (to the extent a shell function has one at all). Is my use of\n> test_must_fail correct?\n\nYour use is not correct (as I'll explain when responding to the patch).\n\n> I agree with Junio Hamano that it's better to provide no argument at all\n> rather than an empty one. I also agree with Jeff King that \"noamend\" is\n> better than an empty argument. I went with the second one since Jeff\n> seemed to get the last word :)\n\nFor what it's worth (probably not much), I agree with Junio.\n\n> I'm not sure I like the ternary inside the function call like that, but\n> I went with it because it gave the smallest footprint (which is probably\n> not a good argument). I suppose I could have done:\n>\n> if (amend)\n> hook_arg1 = \"amend\"\n> else\n> hook_arg1 = \"noamend\"\n> ...\n> ... run_commit_hook(use_editor, index_file, \"pre-commit\", hook_arg1, NULL);\n>\n> or create a hook_amend variable.\n>\n> I'm happy to send a v3.\n"},{"id":"252564","messageId":"CAPig+cQXAbh1d1XcnOfvCn6XNYB2OrzBbxz7zNGFPLA-hQbXhA@mail.gmail.com","threadId":"38050","inReplyTo":"b9ef551633329702559e4cee6528b68ab757b531.1416955873.git.oystwa@gmail.com","subject":"Re: [PATCH 2/2] commit: inform pre-commit that --amend was used","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-11-26T01:36:07Z","receivedAt":"2014-11-26T01:36:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 25, 2014 at 5:51 PM, Øystein Walle wrote:\n> When a commit is amended a pre-commit hook that verifies the commit's\n> contents might not find what it's looking for if it looks at the\n> differences against HEAD when HEAD~1 might be more appropriate. Inform\n> the commit hook that --amend is being used so that hook authors can do\n> e.g.\n>\n> if test \"$1\" = amend; then\n> ...\n> else\n> ...\n> fi\n>\n> to handle these situations.\n>\n> Signed-off-by: Øystein Walle \n> ---\n> diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh\n> index 99ed967..b400afe 100755\n> --- a/t/t7503-pre-commit-hook.sh\n> +++ b/t/t7503-pre-commit-hook.sh\n> @@ -133,4 +133,15 @@ test_expect_success 'check the author in hook' '\n> git show -s\n> '\n>\n> +# a hook that checks if \"amend\" is passed as an argument\n> +write_script \"$HOOK\" < +test \"\\$1\" = amend\n> +EOF\n> +\n> +test_expect_success 'check that \"amend\" argument is given' '\n> + git commit --amend --allow-empty\n> +'\n> +\n> +test_must_fail git commit --allow-empty\n\ntest_must_fail is a direct replacement for '!' when invoking a git\ncommand which should fail _within_ a test. Your original version of\nthis test was in the previous round:\n\n test_expect_success 'check that \"amend\" argument is NOT given' '\n ! git commit --allow-empty\n '\n\nJust replace '!', like this:\n\n test_expect_success 'check that \"amend\" argument is NOT given' '\n test_must_fail git commit --allow-empty\n '\n\n> +\n> test_done\n> --\n> 2.2.0.rc3\n"},{"id":"252570","messageId":"20141126045127.GC15252@peff.net","threadId":"38050","inReplyTo":"78f25aaa60554f7e3b917c565df0f89fb9c08921.1416955873.git.oystwa@gmail.com","subject":"Re: [PATCH 1/2] t7503: use write_script to generate hook scripts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-26T04:51:27Z","receivedAt":"2014-11-26T04:51:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 25, 2014 at 11:51:28PM +0100, Øystein Walle wrote:\n\n> Signed-off-by: Øystein Walle \n> ---\n> t/t7503-pre-commit-hook.sh | 9 +++------\n> 1 file changed, 3 insertions(+), 6 deletions(-)\n> \n> diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh\n> index 984889b..99ed967 100755\n> --- a/t/t7503-pre-commit-hook.sh\n> +++ b/t/t7503-pre-commit-hook.sh\n> @@ -24,8 +24,7 @@ test_expect_success '--no-verify with no hook' '\n> HOOKDIR=\"$(git rev-parse --git-dir)/hooks\"\n> HOOK=\"$HOOKDIR/pre-commit\"\n> mkdir -p \"$HOOKDIR\"\n> -cat > \"$HOOK\" < -#!/bin/sh\n> +write_script \"$HOOK\" < I agree with Junio Hamano that it's better to provide no argument at all\n> rather than an empty one. I also agree with Jeff King that \"noamend\" is\n> better than an empty argument. I went with the second one since Jeff\n> seemed to get the last word :)\n\nI am not sure the last word counts for much. :) We'll see if Junio\nresponds (there, or to your patch). I do not feel _too_ strongly either\nway, and I don't have much else to say besides what was said.\n\n-Peff\n"},{"id":"252587","messageId":"xmqqk32hrfuv.fsf@gitster.dls.corp.google.com","threadId":"38050","inReplyTo":"20141126045127.GC15252@peff.net","subject":"Re: [PATCH 1/2] t7503: use write_script to generate hook scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-26T18:12:08Z","receivedAt":"2014-11-26T18:12:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Nov 25, 2014 at 11:51:28PM +0100, Øystein Walle wrote:\n>\n>> Signed-off-by: Øystein Walle \n>> ---\n>> t/t7503-pre-commit-hook.sh | 9 +++------\n>> 1 file changed, 3 insertions(+), 6 deletions(-)\n>> \n>> diff --git a/t/t7503-pre-commit-hook.sh b/t/t7503-pre-commit-hook.sh\n>> index 984889b..99ed967 100755\n>> --- a/t/t7503-pre-commit-hook.sh\n>> +++ b/t/t7503-pre-commit-hook.sh\n>> @@ -24,8 +24,7 @@ test_expect_success '--no-verify with no hook' '\n>> HOOKDIR=\"$(git rev-parse --git-dir)/hooks\"\n>> HOOK=\"$HOOKDIR/pre-commit\"\n>> mkdir -p \"$HOOKDIR\"\n>> -cat > \"$HOOK\" <> -#!/bin/sh\n>> +write_script \"$HOOK\" <\n> While you are touching this line, please make it \"<<\\EOF\". It does not\n> matter for these simple cases, but as a style, we try to avoid\n> interpolation unless it is necessary.\n\nThanks. It is more about reducing cognitive burden from the\nreaders. An unquoted < writes:\n\n> On Tue, Nov 25, 2014 at 11:51:27PM +0100, Øystein Walle wrote:\n>\n>> I agree with Junio Hamano that it's better to provide no argument at all\n>> rather than an empty one. I also agree with Jeff King that \"noamend\" is\n>> better than an empty argument. I went with the second one since Jeff\n>> seemed to get the last word :)\n>\n> I am not sure the last word counts for much. :) We'll see if Junio\n> responds (there, or to your patch). I do not feel _too_ strongly either\n> way, and I don't have much else to say besides what was said.\n\nI _think_ \"give only info that is necessary\" is cleaner as an\ninterface in theory, but have two niggles myself:\n\n 1. the hooks must do the \"argument parsing\" loop (you already\n mentioned this);\n\n 2. the hooks cannot tell if the lack of \"amending\" argument is\n because the version of Git predates that \"amending\" hint\n support, or because the user action is a straight \"commit\" not\n an \"commit --amend\".\n\nIn any case, I do not have strong preference myself.\n"},{"id":"252592","messageId":"20141126185610.GA1734@peff.net","threadId":"38050","inReplyTo":"xmqqfvd5res5.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/2] pre-commit hook updates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-26T18:56:11Z","receivedAt":"2014-11-26T18:56:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 26, 2014 at 10:35:22AM -0800, Junio C Hamano wrote:\n\n> I _think_ \"give only info that is necessary\" is cleaner as an\n> interface in theory, but have two niggles myself:\n> \n> 1. the hooks must do the \"argument parsing\" loop (you already\n> mentioned this);\n> \n> 2. the hooks cannot tell if the lack of \"amending\" argument is\n> because the version of Git predates that \"amending\" hint\n> support, or because the user action is a straight \"commit\" not\n> an \"commit --amend\".\n> \n> In any case, I do not have strong preference myself.\n\nThat agrees with my thinking exactly.\n\nAt this point since both of us seem on the fence, I am happy to let\nØystein, as the person who is actually writing the patch, decide.\n\n-Peff\n"},{"id":"252594","messageId":"20141126190315.GB1734@peff.net","threadId":"38050","inReplyTo":"xmqqk32hrfuv.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] t7503: use write_script to generate hook scripts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-26T19:03:15Z","receivedAt":"2014-11-26T19:03:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 26, 2014 at 10:12:08AM -0800, Junio C Hamano wrote:\n\n> >> +write_script \"$HOOK\" < >\n> > While you are touching this line, please make it \"<<\\EOF\". It does not\n> > matter for these simple cases, but as a style, we try to avoid\n> > interpolation unless it is necessary.\n> \n> Thanks. It is more about reducing cognitive burden from the\n> readers. An unquoted < carefully for $subsitutions to understand what is going on, instead\n> of coasting it over.\n\nWhile we are talking about it, do we have a style preference on\nalways/never using \"<<-\" unless necessary? I do not think it is as\nimportant as preventing interpolation, because it does not introduce a\ncognitive load in the same way. But consistently using \"<<-\" is one less\nthing for shell newbies to be confused by, and to get wrong when\ncargo-culting.\n\n-Peff\n"},{"id":"252597","messageId":"xmqqy4qxpvy0.fsf@gitster.dls.corp.google.com","threadId":"38050","inReplyTo":"20141126190315.GB1734@peff.net","subject":"Re: [PATCH 1/2] t7503: use write_script to generate hook scripts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-26T20:07:35Z","receivedAt":"2014-11-26T20:07:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Nov 26, 2014 at 10:12:08AM -0800, Junio C Hamano wrote:\n>\n>> >> +write_script \"$HOOK\" <> >\n>> > While you are touching this line, please make it \"<<\\EOF\". It does not\n>> > matter for these simple cases, but as a style, we try to avoid\n>> > interpolation unless it is necessary.\n>> \n>> Thanks. It is more about reducing cognitive burden from the\n>> readers. An unquoted <> carefully for $subsitutions to understand what is going on, instead\n>> of coasting it over.\n>\n> While we are talking about it, do we have a style preference on\n> always/never using \"<<-\" unless necessary? I do not think it is as\n> important as preventing interpolation, because it does not introduce a\n> cognitive load in the same way. But consistently using \"<<-\" is one less\n> thing for shell newbies to be confused by, and to get wrong when\n> cargo-culting.\n\nNot using \"<<-\" is a sign of another problem that is bigger, namely,\nthe test vector is prepared outside of a test_expect_success setup\nblock. Inside test_expect_success, I'd prefer \"<<-\" to make it\nclear where each test begins and next test begins, which would make\nit easier for our to coast over.\n"}]}