{"thread":{"id":"46889","subject":"[PATCH] run-command.c: add hint when hook is not executable","startedAt":"2017-10-03T10:59:44Z","lastAt":"2018-03-05T21:19:19Z","messageCount":16,"participants":["Damien","Junio C Hamano","Damien Marié","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"329552","messageId":"0102015ee1e41f17-927a8da1-ac14-4399-8424-fee8a82c2b0a-000000@eu-west-1.amazonses.com","threadId":"46889","inReplyTo":null,"subject":"[PATCH] run-command.c: add hint when hook is not executable","fromName":"Damien","fromEmail":"damien@dam.io","sentAt":"2017-10-03T10:59:37Z","receivedAt":"2017-10-03T10:59:44Z","isPatch":true,"sender":{"key":"damien@dam.io","avatar":"https://avatars.githubusercontent.com/u/1469823?v=4"},"body":"---\n Documentation/config.txt               | 2 ++\n advice.c                               | 2 ++\n advice.h                               | 1 +\n contrib/completion/git-completion.bash | 1 +\n run-command.c                          | 4 ++++\n 5 files changed, 10 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 1ac0ae6adb046..83b1884cf22fc 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -351,6 +351,8 @@ advice.*::\n \taddEmbeddedRepo::\n \t\tAdvice on what to do when you've accidentally added one\n \t\tgit repo inside of another.\n+\thookNotExecutable::\n+\t\tShown when an hook is there but not executable.\n --\n \n core.fileMode::\ndiff --git a/advice.c b/advice.c\nindex d81e1cb7425b0..969ba149daeec 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -17,6 +17,7 @@ int advice_set_upstream_failure = 1;\n int advice_object_name_warning = 1;\n int advice_rm_hints = 1;\n int advice_add_embedded_repo = 1;\n+int advice_hook_not_executable = 1;\n \n static struct {\n \tconst char *name;\n@@ -38,6 +39,7 @@ static struct {\n \t{ \"objectnamewarning\", &advice_object_name_warning },\n \t{ \"rmhints\", &advice_rm_hints },\n \t{ \"addembeddedrepo\", &advice_add_embedded_repo },\n+\t{ \"hooknotexecutable\", &advice_hook_not_executable},\n \n \t/* make this an alias for backward compatibility */\n \t{ \"pushnonfastforward\", &advice_push_update_rejected }\ndiff --git a/advice.h b/advice.h\nindex c84a44531c7d8..061492976b362 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -19,6 +19,7 @@ extern int advice_set_upstream_failure;\n extern int advice_object_name_warning;\n extern int advice_rm_hints;\n extern int advice_add_embedded_repo;\n+extern int advice_hook_not_executable;\n \n int git_default_advice_config(const char *var, const char *value);\n __attribute__((format (printf, 1, 2)))\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex d93441747523a..6324db0c44f17 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2350,6 +2350,7 @@ _git_config ()\n \t\tadvice.rmHints\n \t\tadvice.statusHints\n \t\tadvice.statusUoption\n+\t\tadvice.hookNotExecutable\n \t\talias.\n \t\tam.keepcr\n \t\tam.threeWay\ndiff --git a/run-command.c b/run-command.c\nindex b5e6eb37c0eb3..95d93a23bf87e 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1174,6 +1174,10 @@ const char *find_hook(const char *name)\n \t\tif (access(path.buf, X_OK) >= 0)\n \t\t\treturn path.buf;\n #endif\n+\t\tif (advice_hook_not_executable) {\n+\t\t\tadvise(_(\"The '%s' hook was ignored because it's not set as executable.\"\n+\t\t\t\t\"\\nYou can disable this warning with `git config advice.hookNotExecutable false`\"), name);\n+\t\t}\n \t\treturn NULL;\n \t}\n \treturn path.buf;\n\n--\nhttps://github.com/git/git/pull/411\n"},{"id":"329643","messageId":"xmqqo9pnedar.fsf@gitster.mtv.corp.google.com","threadId":"46889","inReplyTo":"0102015ee1e41f17-927a8da1-ac14-4399-8424-fee8a82c2b0a-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH] run-command.c: add hint when hook is not executable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-04T04:40:12Z","receivedAt":"2017-10-04T04:40:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Damien <damien@dam.io> writes:\n\n> ---\n\nPlease explain why this change makes sense to those who find this\ncommit in \"git log\" output six months down the line, without having\nread the original thread that motivated you to add this feature\nabove this line with three dashes.  Use your full name on the From:\nheader of your mail (or alternatively, you can use an in-body \"From:\"\nto override what your MUA places there) and add sign-off with the\nsame name (cf. Documentation/SubmittingPatches).\n\n>  Documentation/config.txt               | 2 ++\n>  advice.c                               | 2 ++\n>  advice.h                               | 1 +\n>  contrib/completion/git-completion.bash | 1 +\n>  run-command.c                          | 4 ++++\n>  5 files changed, 10 insertions(+)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 1ac0ae6adb046..83b1884cf22fc 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -351,6 +351,8 @@ advice.*::\n>  \taddEmbeddedRepo::\n>  \t\tAdvice on what to do when you've accidentally added one\n>  \t\tgit repo inside of another.\n> +\thookNotExecutable::\n> +\t\tShown when an hook is there but not executable.\n>  --\n\n\"Shown when\" does not tell readers what is shown; many of the other\nentries in this list does so, and it is helpful to choose from the\nlist when a user encounters one of these advice messages and wants\nto squelch it.\n\n> diff --git a/advice.c b/advice.c\n> diff --git a/advice.h b/advice.h\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n\nThe changes to these files looked good.\n\n> diff --git a/run-command.c b/run-command.c\n> index b5e6eb37c0eb3..95d93a23bf87e 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -1174,6 +1174,10 @@ const char *find_hook(const char *name)\n>  \t\tif (access(path.buf, X_OK) >= 0)\n>  \t\t\treturn path.buf;\n>  #endif\n> +\t\tif (advice_hook_not_executable) {\n> +\t\t\tadvise(_(\"The '%s' hook was ignored because it's not set as executable.\"\n> +\t\t\t\t\"\\nYou can disable this warning with `git config advice.hookNotExecutable false`\"), name);\n> +\t\t}\n>  \t\treturn NULL;\n\nAs to the string constant, it is somewhat strange to see it is\nchomped into two and the LF in the middle is given to the latter\nhalf.  If you are going to chomp anyway, perhaps you'd want to chomp\nit further to avoid making the source line too long.\n\nBut more importantly, is this checking the right thing?  Before the\npre-context of this hunk is:\n\n\tif (access(path.buf, X_OK) < 0) {\n\nso we know access(X_OK) failed.  And we didn't have to care how/why\nit failed because the only thing we did was to return NULL when we\ndecide there is no executable hook.\n\nBut for the purpose of your patch, you now do care.  access(X_OK)\nmay have failed with EACCESS (i.e. you have no permission to run the\nscript), in which case your new advise() call may make sense.  But\nit may have failed with ENOENT or ENOTDIR.  And your new advise()\ncall is a disaster, if the user didn't even have such a hook.\n\nWriting a test would have helped notice this, I would think.  You'd\nneed at least the following variations to cover possibilities:\n\n - Without the advise.* configuration, install an executable hook\n   for a command and try to run the command.  Make sure you do not\n   see any advise message shown.\n\n - Drop the executable bit from the hook from the above and run the\n   same command.  Make sure you do see the advise message.  You'd\n   probably need to protect this test piece with POSIXPERM\n   prerequisite.\n\n - Set advise.* configuration to squelch and run the same command.\n   Make sure you do not see the advise message.\n\n - Remove advise.* configuration and the hook, and run the same\n   command.  Make sure you do not see the advise message.\n\n\n"},{"id":"329800","messageId":"CAAQ5yihCj2uM00bYO2ipEDkh5mT4F7CwbQ48Fae5XFRebO=nRA@mail.gmail.com","threadId":"46889","inReplyTo":"xmqqo9pnedar.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] run-command.c: add hint when hook is not executable","fromName":"Damien","fromEmail":"damien@dam.io","sentAt":"2017-10-05T20:48:52Z","receivedAt":"2017-10-05T20:49:19Z","isPatch":true,"sender":{"key":"damien@dam.io","avatar":"https://avatars.githubusercontent.com/u/1469823?v=4"},"body":"On Wed, Oct 4, 2017 at 6:40 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Damien <damien@dam.io> writes:\n>\n>> ---\n>\n> Please explain why this change makes sense to those who find this\n> commit in \"git log\" output six months down the line, without having\n> read the original thread that motivated you to add this feature\n> above this line with three dashes.  Use your full name on the From:\n> header of your mail (or alternatively, you can use an in-body \"From:\"\n> to override what your MUA places there) and add sign-off with the\n> same name (cf. Documentation/SubmittingPatches).\n\nThanks, I'm gonna post another version via submitGit. I'm not sure\nhow to send the \"cover letter\" via submitGit so I'm just gonna explain\nthe patch here and then send it in another message.\n\nFirst, I changed the name of the config to \"advice.ignoredHook\"\nsince it's more generic and it will allow us to add other cases\nsuch as the bad permission case..\n\n> But for the purpose of your patch, you now do care.  access(X_OK)\n> may have failed with EACCESS (i.e. you have no permission to run the\n> script), in which case your new advise() call may make sense.  But\n> it may have failed with ENOENT or ENOTDIR.  And your new advise()\n> call is a disaster, if the user didn't even have such a hook.\n\nFor sure, the previous version was really broken. I've now added a\nEACCES check. But I see two shortcomings left:\n\n  - the message is shown twice (for a \"pre-commit\" hook at least)\n  - I tried to take into account the STRIP_EXTENSION case but\n    it's not tested yet\n\n> Writing a test would have helped notice this, I would think.  You'd\n> need at least the following variations to cover possibilities:\n\nAs you will see, I've added tests but my scripting skills are a bit\nlacking and I used a simple \"if...then false else true\". I've tried\n`grep -v` and similar things but this solution is the only one that\nI found that really works.\n\nThanks for the review, see PATCH in a coming email.\n"},{"id":"329801","messageId":"0102015eee503678-52126849-7580-4832-9d46-0f5ac7a52336-000000@eu-west-1.amazonses.com","threadId":"46889","inReplyTo":"0102015ee1e41f17-927a8da1-ac14-4399-8424-fee8a82c2b0a-000000@eu-west-1.amazonses.com","subject":"[PATCH v2] run-command: add hint when a hook is ignored","fromName":"Damien Marié","fromEmail":"damien@dam.io","sentAt":"2017-10-05T20:53:07Z","receivedAt":"2017-10-05T20:53:15Z","isPatch":true,"sender":{"key":"damien@dam.io","avatar":"https://avatars.githubusercontent.com/u/1469823?v=4"},"body":"When an hook is present but the file is not set as executable then git will\nignore the hook.\nFor now this is silent which can be confusing.\n\nThis commit adds this warning to improve the situation:\n\n  hint: The 'pre-commit' hook was ignored because it's not set as executable.\n  hint: You can disable this warning with `git config advice.ignoredHook false`\n\nTo allow the old use-case of enabling/disabling hooks via the executable flag a\nnew setting is introduced: advice.ignoredHook.\n\nSigned-off-by: Damien Marié <damien@dam.io>\n---\n Documentation/config.txt               |  3 ++\n advice.c                               |  2 +\n advice.h                               |  1 +\n contrib/completion/git-completion.bash |  1 +\n run-command.c                          | 10 +++++\n t/t7519-ignored-hook-warning.sh        | 67 ++++++++++++++++++++++++++++++++++\n 6 files changed, 84 insertions(+)\n create mode 100755 t/t7519-ignored-hook-warning.sh\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 1ac0ae6adb046..9abca499f725c 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -351,6 +351,9 @@ advice.*::\n \taddEmbeddedRepo::\n \t\tAdvice on what to do when you've accidentally added one\n \t\tgit repo inside of another.\n+\tignoredHook::\n+\t\tAdvice shown if an hook is ignored because the it's not\n+\t\tset as executable.\n --\n \n core.fileMode::\ndiff --git a/advice.c b/advice.c\nindex d81e1cb7425b0..970bd2b71bf53 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -17,6 +17,7 @@ int advice_set_upstream_failure = 1;\n int advice_object_name_warning = 1;\n int advice_rm_hints = 1;\n int advice_add_embedded_repo = 1;\n+int advice_ignored_hook = 1;\n \n static struct {\n \tconst char *name;\n@@ -38,6 +39,7 @@ static struct {\n \t{ \"objectnamewarning\", &advice_object_name_warning },\n \t{ \"rmhints\", &advice_rm_hints },\n \t{ \"addembeddedrepo\", &advice_add_embedded_repo },\n+\t{ \"ignoredhook\", &advice_ignored_hook},\n \n \t/* make this an alias for backward compatibility */\n \t{ \"pushnonfastforward\", &advice_push_update_rejected }\ndiff --git a/advice.h b/advice.h\nindex c84a44531c7d8..f525d6f89cb44 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -19,6 +19,7 @@ extern int advice_set_upstream_failure;\n extern int advice_object_name_warning;\n extern int advice_rm_hints;\n extern int advice_add_embedded_repo;\n+extern int advice_ignored_hook;\n \n int git_default_advice_config(const char *var, const char *value);\n __attribute__((format (printf, 1, 2)))\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex d93441747523a..a331ccc556b01 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2350,6 +2350,7 @@ _git_config ()\n \t\tadvice.rmHints\n \t\tadvice.statusHints\n \t\tadvice.statusUoption\n+\t\tadvice.ignoredHook\n \t\talias.\n \t\tam.keepcr\n \t\tam.threeWay\ndiff --git a/run-command.c b/run-command.c\nindex b5e6eb37c0eb3..9456355595cc1 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1169,11 +1169,21 @@ const char *find_hook(const char *name)\n \tstrbuf_reset(&path);\n \tstrbuf_git_path(&path, \"hooks/%s\", name);\n \tif (access(path.buf, X_OK) < 0) {\n+\t\tint err = errno;\n #ifdef STRIP_EXTENSION\n \t\tstrbuf_addstr(&path, STRIP_EXTENSION);\n \t\tif (access(path.buf, X_OK) >= 0)\n \t\t\treturn path.buf;\n+\t\telse if (errno == EACCES)\n+\t\t\terr = errno;\n #endif\n+\t\tif (err == EACCES && advice_ignored_hook) {\n+\t\t\tadvise(_(\n+\t\t\t\t\"The '%s' hook was ignored because \"\n+\t\t\t\t\"it's not set as executable.\\n\"\n+\t\t\t\t\"You can disable this warning with \"\n+\t\t\t\t\"`git config advice.ignoredHook false`.\"), path.buf);\n+\t\t}\n \t\treturn NULL;\n \t}\n \treturn path.buf;\ndiff --git a/t/t7519-ignored-hook-warning.sh b/t/t7519-ignored-hook-warning.sh\nnew file mode 100755\nindex 0000000000000..59052a4429111\n--- /dev/null\n+++ b/t/t7519-ignored-hook-warning.sh\n@@ -0,0 +1,67 @@\n+#!/bin/sh\n+\n+test_description='ignored hook warning'\n+\n+. ./test-lib.sh\n+\n+# install hook that always succeeds\n+HOOKDIR=\"$(git rev-parse --git-dir)/hooks\"\n+HOOK=\"$HOOKDIR/pre-commit\"\n+mkdir -p \"$HOOKDIR\"\n+cat > \"$HOOK\" <<EOF\n+#!/bin/sh\n+exit 0\n+EOF\n+\n+chmod +x \"$HOOK\"\n+\n+test_expect_success 'no warning if proper hook' '\n+\n+    if git commit -m \"more\" 2>&1 >/dev/null | grep hint\n+    then\n+        false\n+    else\n+        true\n+    fi\n+\n+'\n+\n+chmod -x \"$HOOK\"\n+\n+test_expect_success POSIXPERM 'warning if hook not set as executable' '\n+\n+    if git commit -m \"more\" 2>&1 >/dev/null | grep hint\n+    then\n+        true\n+    else\n+        false\n+    fi\n+'\n+\n+test_expect_success 'no warning if advice.ignoredHook set to false' '\n+\n+    git config advice.ignoredHook false &&\n+    if git commit -m \"more\" 2>&1 >/dev/null | grep hint\n+    then\n+        false\n+    else\n+        true\n+    fi\n+'\n+\n+rm \"$HOOK\"\n+\n+test_expect_success 'no warning if unset advice.ignoredHook and hook removed' '\n+\n+    git config --unset advice.ignoredHook &&\n+    if git commit -m \"more\" 2>&1 >/dev/null | grep hint\n+    then\n+        false\n+    else\n+        true\n+    fi\n+'\n+git commit -m \"more\" 2>&1 >/dev/null\n+\n+\n+test_done\n\\ No newline at end of file\n\n--\nhttps://github.com/git/git/pull/411\n"},{"id":"329847","messageId":"xmqq4lrcyj1p.fsf@gitster.mtv.corp.google.com","threadId":"46889","inReplyTo":"0102015eee503678-52126849-7580-4832-9d46-0f5ac7a52336-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v2] run-command: add hint when a hook is ignored","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-06T04:52:34Z","receivedAt":"2017-10-06T04:52:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Damien Marié <damien@dam.io> writes:\n\n>  \tif (access(path.buf, X_OK) < 0) {\n> +\t\tint err = errno;\n\nOK, so we remember how/why we failed in err.\n\n>  #ifdef STRIP_EXTENSION\n>  \t\tstrbuf_addstr(&path, STRIP_EXTENSION);\n>  \t\tif (access(path.buf, X_OK) >= 0)\n>  \t\t\treturn path.buf;\n> +\t\telse if (errno == EACCES)\n> +\t\t\terr = errno;\n\nI think it is easier to reason about if this were not \"else if\", but\njust a simple \"if\".\n\n - We tried foo-hook, and failed.\n - On a platform that foo-hook.exe can also be a hook\n   - We try foo-hook.exe and if it seems OK, we return with smile.\n   - If not, if we know foo-hook.exe exists but we cannot execute,\n     we update err (forgetting the reason why foo-hook was wrong)\n     with the reason why foo-hook.exe is bad.\n\nIt is OK to forget why foo-hook was unhappy, as on a STRIP_EXTENSION\nbuild, we would have tried to run foo-hook.exe anyway.\n\n>  #endif\n\nSo at this point, with or without STRIP_EXTENSION, err tells us why\nthe file we wanted to be available as a hook did not pass our\ncriteria.\n\n> +\t\tif (err == EACCES && advice_ignored_hook) {\n\nAnd we want to do the advise thing only if we know we failed due to\nEACCES and for no other reason.\n\n> +\t\t\tadvise(_(\n> +\t\t\t\t\"The '%s' hook was ignored because \"\n> +\t\t\t\t\"it's not set as executable.\\n\"\n> +\t\t\t\t\"You can disable this warning with \"\n> +\t\t\t\t\"`git config advice.ignoredHook false`.\"), path.buf);\n> +\t\t}\n>  \t\treturn NULL;\n>  \t}\n>  \treturn path.buf;\n\nOverall, the logic looks correct to me.  Note that we may have\ngotten EACCES not because the path lacked the executable bit, but\nbecause the hook directory was unreadable ;-), but in such a case,\nyou cannot tell if \"it's not set as executable\" is true anyway.\n\n> diff --git a/t/t7519-ignored-hook-warning.sh b/t/t7519-ignored-hook-warning.sh\n> new file mode 100755\n> index 0000000000000..59052a4429111\n> --- /dev/null\n> +++ b/t/t7519-ignored-hook-warning.sh\n> @@ -0,0 +1,67 @@\n> +#!/bin/sh\n> +\n> +test_description='ignored hook warning'\n> +\n> +. ./test-lib.sh\n> +\n\nThese days, things like this...\n\n> +# install hook that always succeeds\n> +HOOKDIR=\"$(git rev-parse --git-dir)/hooks\"\n> +HOOK=\"$HOOKDIR/pre-commit\"\n> +mkdir -p \"$HOOKDIR\"\n> +cat > \"$HOOK\" <<EOF\n> +#!/bin/sh\n> +exit 0\n> +EOF\n> +\n> +chmod +x \"$HOOK\"\n\n...should all go to test_expect_success, i.e.\n\n\ttest_expect_success setup '\n\t\t...\n\t\tmkdir -p \"$hookdir\" &&\n\t\twrite_script \"$hookdir/pre-commit\" <<-\\EOF\n\t\texit 0\n\t\tEOF\n\t'\n\nwrite_script takes care of flipping +x on.\n\n> +test_expect_success 'no warning if proper hook' '\n> +\n> +    if git commit -m \"more\" 2>&1 >/dev/null | grep hint\n> +    then\n> +        false\n> +    else\n> +        true\n> +    fi\n> +\n\n - Indents in our shell scripts are done with tab (HT).\n\n - We try to avoid running git command on the LHS of a pipe when we\n   do not have to.\n\n - \"git commit\" may fail due to not having anything worth\n   committing, even before it notices that pre-commit hook is or is\n   not executable.  Avoid relying on the order of things that happen\n   to be true in the current implementation when we do not have to.\n\n - We may see some other hint.  Avoid relying on the set of advises\n   that happens to currently be defined when we do not have to.\n\n - Output from advise() can be localized, so grepping to expect\n   something either is there or is not there would be triggered as\n   an error in GETTEXT_POISON build.  We unfortunately need to use\n   test_i18ngrep to work it around.\n\nPerhaps the above should become more like so:\n\n\tgit commit --allow-empty -m more 2>message &&\n\ttest_i18ngrep ! \"hook was ignored\" message\n\n> +'\n> +\n> +chmod -x \"$HOOK\"\n\nMove this to the beginning of the next one that is protected with\nPOSIXPERM.\n\n> +test_expect_success POSIXPERM 'warning if hook not set as executable' '\n> +\n> +    if git commit -m \"more\" 2>&1 >/dev/null | grep hint\n> +    then\n> +        true\n> +    else\n> +        false\n> +    fi\n> +'\n\n\tchmod -x \"$hookdir/pre-commit\" &&\n\tgit commit --allow-empty -m \"even more\" 2>message &&\n\ttest_i18ngrep \"hook was ignored\" message\n\nor something like that.\n\nThanks.\n"},{"id":"329851","messageId":"xmqqr2ugx2xm.fsf@gitster.mtv.corp.google.com","threadId":"46889","inReplyTo":"xmqq4lrcyj1p.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] run-command: add hint when a hook is ignored","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-06T05:25:57Z","receivedAt":"2017-10-06T05:26:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> diff --git a/t/t7519-ignored-hook-warning.sh b/t/t7519-ignored-hook-warning.sh\n>> new file mode 100755\n\nAnother thing; t7519 is taken by another topic in flight.  Let's use\nt7520 instead.\n"},{"id":"329852","messageId":"xmqqmv54x24j.fsf@gitster.mtv.corp.google.com","threadId":"46889","inReplyTo":"xmqq4lrcyj1p.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] run-command: add hint when a hook is ignored","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-06T05:43:24Z","receivedAt":"2017-10-06T05:43:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Here is a suggested rewrite of t7519 (I used t7520 to avoid crashing\nwith another topic in flight).\n\n - use unused/unallocated 7520 to avoid crashes with bp/fsmonitor\n   topic\n\n - use setup inside test_expect_success\n\n - use test_i18ngrep to avoid gettext-poison build gotchas\n\n - look for specific advise message to make the test more robust\n\n - do not run Git on LHS of a pipe when we do not have to\n\n - use test_config and test_unconfig\n\n\n t/t7520-ignored-hook-warning.sh | 41 +++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 41 insertions(+)\n create mode 100755 t/t7520-ignored-hook-warning.sh\n\ndiff --git a/t/t7520-ignored-hook-warning.sh b/t/t7520-ignored-hook-warning.sh\nnew file mode 100755\nindex 0000000000..634fb7f23a\n--- /dev/null\n+++ b/t/t7520-ignored-hook-warning.sh\n@@ -0,0 +1,41 @@\n+#!/bin/sh\n+\n+test_description='ignored hook warning'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\thookdir=\"$(git rev-parse --git-dir)/hooks\" &&\n+\thook=\"$hookdir/pre-commit\" &&\n+\tmkdir -p \"$hookdir\" &&\n+\twrite_script \"$hook\" <<-\\EOF\n+\texit 0\n+\tEOF\n+'\n+\n+test_expect_success 'no warning if hook is not ignored' '\n+\tgit commit --allow-empty -m \"more\" 2>message &&\n+\ttest_i18ngrep ! -e \"hook was ignored\" message\n+'\n+\n+test_expect_success POSIXPERM 'warning if hook is ignored' '\n+\tchmod -x \"$hook\" &&\n+\tgit commit --allow-empty -m \"even more\" 2>message &&\n+\ttest_i18ngrep -e \"hook was ignored\" message\n+'\n+\n+test_expect_success POSIXPERM 'no warning if advice.ignoredHook set to false' '\n+\ttest_config advice.ignoredHook false &&\n+\tchmod -x \"$hook\" &&\n+\tgit commit --allow-empty -m \"even more\" 2>message &&\n+\ttest_i18ngrep ! -e \"hook was ignored\" message\n+'\n+\n+test_expect_success 'no warning if unset advice.ignoredHook and hook removed' '\n+\trm -f \"$hook\" &&\n+\ttest_unconfig advice.ignoredHook &&\n+\tgit commit --allow-empty -m \"even more\" 2>message &&\n+\ttest_i18ngrep ! -e \"hook was ignored\" message\n+'\n+\n+test_done\n-- \n2.15.0-rc0-155-g07e9c1a78d\n\n"},{"id":"329853","messageId":"xmqqh8vcx1nh.fsf@gitster.mtv.corp.google.com","threadId":"46889","inReplyTo":"xmqq4lrcyj1p.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] run-command: add hint when a hook is ignored","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-06T05:53:38Z","receivedAt":"2017-10-06T05:53:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I think it is easier to reason about if this were not \"else if\", but\n> just a simple \"if\".\n\nAnd here are two small suggested changes to the code portion of your\npatch.\n\n - break if / else if cascade into two independent if / if\n   statements for clarity.\n\n - give the \"ignored hook\" advice only once per <process,hook>\n   pair.\n\n\n\n run-command.c | 19 +++++++++++++------\n 1 file changed, 13 insertions(+), 6 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 0f8a5f7fa2..0e60dd2075 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -5,6 +5,7 @@\n #include \"argv-array.h\"\n #include \"thread-utils.h\"\n #include \"strbuf.h\"\n+#include \"string-list.h\"\n \n void child_process_init(struct child_process *child)\n {\n@@ -1170,19 +1171,25 @@ const char *find_hook(const char *name)\n \tstrbuf_git_path(&path, \"hooks/%s\", name);\n \tif (access(path.buf, X_OK) < 0) {\n \t\tint err = errno;\n+\n #ifdef STRIP_EXTENSION\n \t\tstrbuf_addstr(&path, STRIP_EXTENSION);\n \t\tif (access(path.buf, X_OK) >= 0)\n \t\t\treturn path.buf;\n-\t\telse if (errno == EACCES)\n+\t\tif (errno == EACCES)\n \t\t\terr = errno;\n #endif\n \t\tif (err == EACCES && advice_ignored_hook) {\n-\t\t\tadvise(_(\n-\t\t\t\t\"The '%s' hook was ignored because \"\n-\t\t\t\t\"it's not set as executable.\\n\"\n-\t\t\t\t\"You can disable this warning with \"\n-\t\t\t\t\"`git config advice.ignoredHook false`.\"), path.buf);\n+\t\t\tstatic struct string_list advise_given = STRING_LIST_INIT_DUP;\n+\n+\t\t\tif (!string_list_lookup(&advise_given, name)) {\n+\t\t\t\tstring_list_insert(&advise_given, name);\n+\t\t\t\tadvise(_(\"The '%s' hook was ignored because \"\n+\t\t\t\t\t \"it's not set as executable.\\n\"\n+\t\t\t\t\t \"You can disable this warning with \"\n+\t\t\t\t\t \"`git config advice.ignoredHook false`.\"),\n+\t\t\t\t       path.buf);\n+\t\t\t}\n \t\t}\n \t\treturn NULL;\n \t}\n-- \n2.15.0-rc0-155-g07e9c1a78d\n\n"},{"id":"329860","messageId":"CAAQ5yihg2+BNVG-Dpvq8-qV21uX8xLsDmQpvhUzD5nSvMRFyyQ@mail.gmail.com","threadId":"46889","inReplyTo":"xmqqh8vcx1nh.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] run-command: add hint when a hook is ignored","fromName":"Damien","fromEmail":"damien@dam.io","sentAt":"2017-10-06T08:04:48Z","receivedAt":"2017-10-06T08:05:19Z","isPatch":true,"sender":{"key":"damien@dam.io","avatar":"https://avatars.githubusercontent.com/u/1469823?v=4"},"body":"Thanks for the help, a new patch is coming with those fixes applied.\n\nOn Fri, Oct 6, 2017 at 7:53 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> I think it is easier to reason about if this were not \"else if\", but\n>> just a simple \"if\".\n>\n> And here are two small suggested changes to the code portion of your\n> patch.\n>\n>  - break if / else if cascade into two independent if / if\n>    statements for clarity.\n>\n>  - give the \"ignored hook\" advice only once per <process,hook>\n>    pair.\n>\n>\n>\n>  run-command.c | 19 +++++++++++++------\n>  1 file changed, 13 insertions(+), 6 deletions(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index 0f8a5f7fa2..0e60dd2075 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -5,6 +5,7 @@\n>  #include \"argv-array.h\"\n>  #include \"thread-utils.h\"\n>  #include \"strbuf.h\"\n> +#include \"string-list.h\"\n>\n>  void child_process_init(struct child_process *child)\n>  {\n> @@ -1170,19 +1171,25 @@ const char *find_hook(const char *name)\n>         strbuf_git_path(&path, \"hooks/%s\", name);\n>         if (access(path.buf, X_OK) < 0) {\n>                 int err = errno;\n> +\n>  #ifdef STRIP_EXTENSION\n>                 strbuf_addstr(&path, STRIP_EXTENSION);\n>                 if (access(path.buf, X_OK) >= 0)\n>                         return path.buf;\n> -               else if (errno == EACCES)\n> +               if (errno == EACCES)\n>                         err = errno;\n>  #endif\n>                 if (err == EACCES && advice_ignored_hook) {\n> -                       advise(_(\n> -                               \"The '%s' hook was ignored because \"\n> -                               \"it's not set as executable.\\n\"\n> -                               \"You can disable this warning with \"\n> -                               \"`git config advice.ignoredHook false`.\"), path.buf);\n> +                       static struct string_list advise_given = STRING_LIST_INIT_DUP;\n> +\n> +                       if (!string_list_lookup(&advise_given, name)) {\n> +                               string_list_insert(&advise_given, name);\n> +                               advise(_(\"The '%s' hook was ignored because \"\n> +                                        \"it's not set as executable.\\n\"\n> +                                        \"You can disable this warning with \"\n> +                                        \"`git config advice.ignoredHook false`.\"),\n> +                                      path.buf);\n> +                       }\n>                 }\n>                 return NULL;\n>         }\n> --\n> 2.15.0-rc0-155-g07e9c1a78d\n>\n"},{"id":"329861","messageId":"0102015ef0ba0273-ed29c030-7cdc-4535-a411-6cc443bd5f43-000000@eu-west-1.amazonses.com","threadId":"46889","inReplyTo":"xmqqh8vcx1nh.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v3] run-command: add hint when a hook is ignored","fromName":"Damien Marié","fromEmail":"damien@dam.io","sentAt":"2017-10-06T08:07:55Z","receivedAt":"2017-10-06T08:08:05Z","isPatch":true,"sender":{"key":"damien@dam.io","avatar":"https://avatars.githubusercontent.com/u/1469823?v=4"},"body":"When an hook is present but the file is not set as executable then git will\nignore the hook.\nFor now this is silent which can be confusing.\n\nThis commit adds this warning to improve the situation:\n\n  hint: The 'pre-commit' hook was ignored because it's not set as executable.\n  hint: You can disable this warning with `git config advice.ignoredHook false`\n\nTo allow the old use-case of enabling/disabling hooks via the executable flag a\nnew setting is introduced: advice.ignoredHook.\n\nSigned-off-by: Damien Marié <damien@dam.io>\n---\n Documentation/config.txt               |  3 +++\n advice.c                               |  2 ++\n advice.h                               |  1 +\n contrib/completion/git-completion.bash |  1 +\n run-command.c                          | 18 +++++++++++++++\n t/t7520-ignored-hook-warning.sh        | 41 ++++++++++++++++++++++++++++++++++\n 6 files changed, 66 insertions(+)\n create mode 100755 t/t7520-ignored-hook-warning.sh\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 1ac0ae6adb046..9abca499f725c 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -351,6 +351,9 @@ advice.*::\n \taddEmbeddedRepo::\n \t\tAdvice on what to do when you've accidentally added one\n \t\tgit repo inside of another.\n+\tignoredHook::\n+\t\tAdvice shown if an hook is ignored because the it's not\n+\t\tset as executable.\n --\n \n core.fileMode::\ndiff --git a/advice.c b/advice.c\nindex d81e1cb7425b0..970bd2b71bf53 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -17,6 +17,7 @@ int advice_set_upstream_failure = 1;\n int advice_object_name_warning = 1;\n int advice_rm_hints = 1;\n int advice_add_embedded_repo = 1;\n+int advice_ignored_hook = 1;\n \n static struct {\n \tconst char *name;\n@@ -38,6 +39,7 @@ static struct {\n \t{ \"objectnamewarning\", &advice_object_name_warning },\n \t{ \"rmhints\", &advice_rm_hints },\n \t{ \"addembeddedrepo\", &advice_add_embedded_repo },\n+\t{ \"ignoredhook\", &advice_ignored_hook},\n \n \t/* make this an alias for backward compatibility */\n \t{ \"pushnonfastforward\", &advice_push_update_rejected }\ndiff --git a/advice.h b/advice.h\nindex c84a44531c7d8..f525d6f89cb44 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -19,6 +19,7 @@ extern int advice_set_upstream_failure;\n extern int advice_object_name_warning;\n extern int advice_rm_hints;\n extern int advice_add_embedded_repo;\n+extern int advice_ignored_hook;\n \n int git_default_advice_config(const char *var, const char *value);\n __attribute__((format (printf, 1, 2)))\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex d93441747523a..a331ccc556b01 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2350,6 +2350,7 @@ _git_config ()\n \t\tadvice.rmHints\n \t\tadvice.statusHints\n \t\tadvice.statusUoption\n+\t\tadvice.ignoredHook\n \t\talias.\n \t\tam.keepcr\n \t\tam.threeWay\ndiff --git a/run-command.c b/run-command.c\nindex b5e6eb37c0eb3..288abbba5afd7 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -5,6 +5,7 @@\n #include \"argv-array.h\"\n #include \"thread-utils.h\"\n #include \"strbuf.h\"\n+#include \"string-list.h\"\n \n void child_process_init(struct child_process *child)\n {\n@@ -1169,11 +1170,28 @@ const char *find_hook(const char *name)\n \tstrbuf_reset(&path);\n \tstrbuf_git_path(&path, \"hooks/%s\", name);\n \tif (access(path.buf, X_OK) < 0) {\n+\t\tint err = errno;\n+\n #ifdef STRIP_EXTENSION\n \t\tstrbuf_addstr(&path, STRIP_EXTENSION);\n \t\tif (access(path.buf, X_OK) >= 0)\n \t\t\treturn path.buf;\n+\t\tif (errno == EACCES)\n+\t\t\terr = errno;\n #endif\n+\n+\t\tif (err == EACCES && advice_ignored_hook) {\n+\t\t\tstatic struct string_list advise_given = STRING_LIST_INIT_DUP;\n+\t\t\t\n+\t\t\tif (!string_list_lookup(&advise_given, name)) {\n+\t\t\t\tstring_list_insert(&advise_given, name);\n+\t\t\t\tadvise(_(\"The '%s' hook was ignored because \"\n+\t\t\t\t\t\t\t\"it's not set as executable.\\n\"\n+\t\t\t\t\t\t\t\"You can disable this warning with \"\n+\t\t\t\t\t\t\t\"`git config advice.ignoredHook false`.\"),\n+\t\t\t\t\t\tpath.buf);\n+\t\t\t}\n+\t\t}\n \t\treturn NULL;\n \t}\n \treturn path.buf;\ndiff --git a/t/t7520-ignored-hook-warning.sh b/t/t7520-ignored-hook-warning.sh\nnew file mode 100755\nindex 0000000000000..634fb7f23a040\n--- /dev/null\n+++ b/t/t7520-ignored-hook-warning.sh\n@@ -0,0 +1,41 @@\n+#!/bin/sh\n+\n+test_description='ignored hook warning'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\thookdir=\"$(git rev-parse --git-dir)/hooks\" &&\n+\thook=\"$hookdir/pre-commit\" &&\n+\tmkdir -p \"$hookdir\" &&\n+\twrite_script \"$hook\" <<-\\EOF\n+\texit 0\n+\tEOF\n+'\n+\n+test_expect_success 'no warning if hook is not ignored' '\n+\tgit commit --allow-empty -m \"more\" 2>message &&\n+\ttest_i18ngrep ! -e \"hook was ignored\" message\n+'\n+\n+test_expect_success POSIXPERM 'warning if hook is ignored' '\n+\tchmod -x \"$hook\" &&\n+\tgit commit --allow-empty -m \"even more\" 2>message &&\n+\ttest_i18ngrep -e \"hook was ignored\" message\n+'\n+\n+test_expect_success POSIXPERM 'no warning if advice.ignoredHook set to false' '\n+\ttest_config advice.ignoredHook false &&\n+\tchmod -x \"$hook\" &&\n+\tgit commit --allow-empty -m \"even more\" 2>message &&\n+\ttest_i18ngrep ! -e \"hook was ignored\" message\n+'\n+\n+test_expect_success 'no warning if unset advice.ignoredHook and hook removed' '\n+\trm -f \"$hook\" &&\n+\ttest_unconfig advice.ignoredHook &&\n+\tgit commit --allow-empty -m \"even more\" 2>message &&\n+\ttest_i18ngrep ! -e \"hook was ignored\" message\n+'\n+\n+test_done\n\n--\nhttps://github.com/git/git/pull/411\n"},{"id":"330092","messageId":"xmqqy3ojpr9f.fsf@gitster.mtv.corp.google.com","threadId":"46889","inReplyTo":"0102015ef0ba0273-ed29c030-7cdc-4535-a411-6cc443bd5f43-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v3] run-command: add hint when a hook is ignored","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-10T04:21:16Z","receivedAt":"2017-10-10T04:21:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Damien Marié <damien@dam.io> writes:\n\n> When an hook is present but the file is not set as executable then git will\n> ignore the hook.\n> For now this is silent which can be confusing.\n\nQuite honestly, I do not particulary think this is confusing, and I\nexpect that this change will irritate many people by forcing them to\neither set the advise config or move the ones that they deliberately\nleft unexecutable by renaming them by adding \".disabled\" at the end.\n\nBut these remedies are easy enough, so let's see how well it works\nby merging it to 'next' and cooking it there for a while.\n\nI've spotted two issues during the last-minute scan; I'll squash the\nfixes in in the meantime (if you are not going to change anything\nother than these two, there is no need to resend corrected patches).\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 1ac0ae6adb046..9abca499f725c 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -351,6 +351,9 @@ advice.*::\n>  \taddEmbeddedRepo::\n>  \t\tAdvice on what to do when you've accidentally added one\n>  \t\tgit repo inside of another.\n> +\tignoredHook::\n> +\t\tAdvice shown if an hook is ignored because the it's not\n> +\t\tset as executable.\n\ns/the it's not/the hook is not/;\n\n> @@ -38,6 +39,7 @@ static struct {\n>  \t{ \"objectnamewarning\", &advice_object_name_warning },\n>  \t{ \"rmhints\", &advice_rm_hints },\n>  \t{ \"addembeddedrepo\", &advice_add_embedded_repo },\n> +\t{ \"ignoredhook\", &advice_ignored_hook},\n\ns/ignored_hook/& /;\n\n"},{"id":"330153","messageId":"xmqqmv4ymc7w.fsf@gitster.mtv.corp.google.com","threadId":"46889","inReplyTo":"xmqqy3ojpr9f.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3] run-command: add hint when a hook is ignored","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-11T06:26:43Z","receivedAt":"2017-10-11T06:26:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Quite honestly, I do not particulary think this is confusing, and I\n> expect that this change will irritate many people by forcing them to\n> either set the advise config or move the ones that they deliberately\n> left unexecutable by renaming them by adding \".disabled\" at the end.\n>\n> But these remedies are easy enough, so let's see how well it works\n> by merging it to 'next' and cooking it there for a while.\n\nWell, it turns out that I am among those who are irritated, as all\nthe repositories I work with were rather old, dating back to 2005,\nback when it was a norm to have these sample files installed without\nexecutable bit, to make it easy for those who choose to use them\nas-is to enable them by flipping the executable bit.\n\nAnd I do not find the advice.ignoredhook is giving particularly a\ngood piece of advice.  I suspect that it would be a better practice\nto rename a disabled foo-hook to foo-hook.disabled if the user wants\nto squelch the warning.  It gives them a final chance to review what\nthey left disabled for all these years, and then choose to either\nremove it, or rename it to foo-hook.disabled.  \"ls .git/hooks/\" will\nthen make it clear which ones are disabled without the \"-F\" option,\nwhich is an additional benefit.\n\nAnyway, I am not merging this topic to the upcoming release, so\nhopefully we'll hear from others who try 'next'.\n\nThanks.\n"},{"id":"335706","messageId":"20180103083145.GA7049@sigill.intra.peff.net","threadId":"46889","inReplyTo":"xmqqmv4ymc7w.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3] run-command: add hint when a hook is ignored","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-01-03T08:31:46Z","receivedAt":"2018-01-03T08:31:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 11, 2017 at 03:26:43PM +0900, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Quite honestly, I do not particulary think this is confusing, and I\n> > expect that this change will irritate many people by forcing them to\n> > either set the advise config or move the ones that they deliberately\n> > left unexecutable by renaming them by adding \".disabled\" at the end.\n> >\n> > But these remedies are easy enough, so let's see how well it works\n> > by merging it to 'next' and cooking it there for a while.\n> \n> Well, it turns out that I am among those who are irritated, as all\n> the repositories I work with were rather old, dating back to 2005,\n> back when it was a norm to have these sample files installed without\n> executable bit, to make it easy for those who choose to use them\n> as-is to enable them by flipping the executable bit.\n> [...]\n> Anyway, I am not merging this topic to the upcoming release, so\n> hopefully we'll hear from others who try 'next'.\n\nThis bit me today in a funny way: t5523 started failing.\n\nThe problem is that I was bisecting an unrelated change in old code, and\nI built a commit which predates f98f8cbac0 (Ship sample hooks with .sample\nsuffix, 2008-06-24). That wrote a bare \"update\" file into templates/blt\n(without the \".sample\" suffix). After that, the cruft is forever in my\nbuild directory until I \"git clean -x\".\n\nThe t5523 script tries to run \"git push --quiet\" and make sure that it\nproduces no output, but with this setup it produces a round of \"hint\"\nlines (actually, they come from receive-pack on the remote end but still\nend up on stderr).\n\nSo that raises a few interesting questions to me:\n\n  1. Should receive-pack generate these hints? They get propagated by\n     the client, but there's a reasonable chance that the user can't\n     actually fix the situation.\n\n  2. Should these hints be suppressed with --quiet? I can see an\n     argument that \"--quiet\" only applies to non-errors. And while these\n     are not fatal errors, they're outside the realm of the usual\n     chattiness.\n\n  3. Should our tests be more careful about not looking at the\n     template hooks? I think test_create_repo already disables the hooks\n     directory manually, but many repos will be created by \"git clone\"\n     during the tests.\n\n  4. Should our build system be more clever about dropping non-existent\n     files from templates/blt?\n\nI started down the road of saying \"--quiet should disable all advice\",\nand that patch is below. But I'm having second thoughts on it. It fixes\nthe case in receive-pack, but what should \"commit --quiet\" do? It's\ndocumented only to suppress the status output after commit. Should it\ncover this case? For that matter, the \"quiet\" protocol extension which\nis passed to receive-pack is generally used only for progress reporting,\nand is sent automatically when stderr isn't a tty (which is why the\ntests below must go through some contortions with test_terminal). I'm\nnot sure if it should actually apply to advice.\n\n---\n advice.c                        |  8 ++++++++\n advice.h                        |  6 ++++++\n builtin/receive-pack.c          |  4 +++-\n t/t7520-ignored-hook-warning.sh | 15 +++++++++++++++\n 4 files changed, 32 insertions(+), 1 deletion(-)\n\ndiff --git a/advice.c b/advice.c\nindex 406efc183b..f754af8abe 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -137,3 +137,11 @@ void detach_advice(const char *new_name)\n \n \tfprintf(stderr, fmt, new_name);\n }\n+\n+void disable_advice(void)\n+{\n+\tsize_t i;\n+\n+\tfor (i = 0; i < ARRAY_SIZE(advice_config); i++)\n+\t\t*advice_config[i].preference = 0;\n+}\ndiff --git a/advice.h b/advice.h\nindex 70568fa792..4895554ef3 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -30,4 +30,10 @@ extern void NORETURN die_resolve_conflict(const char *me);\n void NORETURN die_conclude_merge(void);\n void detach_advice(const char *new_name);\n \n+/*\n+ * Turn off all advice flags; this can be used to centrally enforce a --quiet\n+ * option.\n+ */\n+void disable_advice(void);\n+\n #endif /* ADVICE_H */\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex b7ce7c7f52..f257c16776 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1601,8 +1601,10 @@ static struct command *read_head_info(struct oid_array *shallow)\n \t\t\t\treport_status = 1;\n \t\t\tif (parse_feature_request(feature_list, \"side-band-64k\"))\n \t\t\t\tuse_sideband = LARGE_PACKET_MAX;\n-\t\t\tif (parse_feature_request(feature_list, \"quiet\"))\n+\t\t\tif (parse_feature_request(feature_list, \"quiet\")) {\n \t\t\t\tquiet = 1;\n+\t\t\t\tdisable_advice();\n+\t\t\t}\n \t\t\tif (advertise_atomic_push\n \t\t\t    && parse_feature_request(feature_list, \"atomic\"))\n \t\t\t\tuse_atomic = 1;\ndiff --git a/t/t7520-ignored-hook-warning.sh b/t/t7520-ignored-hook-warning.sh\nindex 634fb7f23a..1d27d3e3f0 100755\n--- a/t/t7520-ignored-hook-warning.sh\n+++ b/t/t7520-ignored-hook-warning.sh\n@@ -3,6 +3,7 @@\n test_description='ignored hook warning'\n \n . ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-terminal.sh\n \n test_expect_success setup '\n \thookdir=\"$(git rev-parse --git-dir)/hooks\" &&\n@@ -38,4 +39,18 @@ test_expect_success 'no warning if unset advice.ignoredHook and hook removed' '\n \ttest_i18ngrep ! -e \"hook was ignored\" message\n '\n \n+test_expect_success TTY,POSIXPERM 'push --quiet silences remote hook warnings' '\n+\tgit init --bare dst.git &&\n+\techo \"exit 0\" >dst.git/hooks/update &&\n+\tchmod -x dst.git/hooks/update &&\n+\n+\tgit commit --allow-empty -m one &&\n+\ttest_terminal git push dst.git HEAD 2>message &&\n+\ttest_i18ngrep -e \"hook was ignored\" message &&\n+\n+\tgit commit --allow-empty -m two &&\n+\ttest_terminal git push --quiet dst.git HEAD 2>message &&\n+\ttest_i18ngrep ! -e \"hook was ignored\" message\n+'\n+\n test_done\n-- \n2.16.0.rc0.384.gc477e89267\n\n"},{"id":"335947","messageId":"xmqqk1wwcd2w.fsf@gitster.mtv.corp.google.com","threadId":"46889","inReplyTo":"20180103083145.GA7049@sigill.intra.peff.net","subject":"Re: [PATCH v3] run-command: add hint when a hook is ignored","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-05T19:36:39Z","receivedAt":"2018-01-05T19:36:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The problem is that I was bisecting an unrelated change in old code, and\n> I built a commit which predates f98f8cbac0 (Ship sample hooks with .sample\n> suffix, 2008-06-24). That wrote a bare \"update\" file into templates/blt\n> (without the \".sample\" suffix). After that, the cruft is forever in my\n> build directory until I \"git clean -x\".\n>\n> The t5523 script tries to run \"git push --quiet\" and make sure that it\n> produces no output, but with this setup it produces a round of \"hint\"\n> lines (actually, they come from receive-pack on the remote end but still\n> end up on stderr).\n>\n> So that raises a few interesting questions to me:\n>\n>   1. Should receive-pack generate these hints? They get propagated by\n>      the client, but there's a reasonable chance that the user can't\n>      actually fix the situation.\n\nTrue.  The user could tell the server operator to rename them or\nremove them because they are not doing anything useful, but then\nas long as everybody knows they are not doing anything, it is OK\nto leave that sleeping dog lie, as they are not doing anything\nharmful anyway.\n\nThat brings us one step further back to question if the hints are\nuseful in the first place, though ;-).\n\n>   2. Should these hints be suppressed with --quiet? I can see an\n>      argument that \"--quiet\" only applies to non-errors. And while these\n>      are not fatal errors, they're outside the realm of the usual\n>      chattiness.\n\nI do not think having a non-executable file whose name happens to be\nthe same as a hook is an error in the first place [*1*], so I do not\nthink it is unreasonable for --quiet to squelch.\n\n\tside note [*1*]: we need to make sure that it is clearly\n\tdocumented that such a file is not a hook, of course.\n\n>   3. Should our tests be more careful about not looking at the\n>      template hooks? I think test_create_repo already disables the hooks\n>      directory manually, but many repos will be created by \"git clone\"\n>      during the tests.\n\nThis probably is much deeper issue and templates/hooks--* is merely\na tip of iceberg, I suspect.  In general build artifacts from\ndifferent versions of Git in the same build area could interfere the\ntests in other ways (e.g. a test for \"git help\" output could try to\ngrab a deprecated command left in bin-wrappers/ from previous build\nof an ancient version), and could potentially interfere things other\nthan the tests (e.g. the 'install' target of the Makefile may be\nwritten loosely to install everything in a directory).\n\nIn general, I am a bit pessimistic and suspect that the best we\ncould say is \"if you want to absolutely avoid such interference,\n'make distclean' before switching to test another revision\".\n\n>   4. Should our build system be more clever about dropping non-existent\n>      files from templates/blt?\n\nIt would be a reasonable mitigation that is specific to templates/,\nI suspect.\n"},{"id":"340878","messageId":"20180303071613.GC17312@sigill.intra.peff.net","threadId":"46889","inReplyTo":"xmqqk1wwcd2w.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3] run-command: add hint when a hook is ignored","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-03-03T07:16:13Z","receivedAt":"2018-03-03T07:16:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 05, 2018 at 11:36:39AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > The problem is that I was bisecting an unrelated change in old code, and\n> > I built a commit which predates f98f8cbac0 (Ship sample hooks with .sample\n> > suffix, 2008-06-24). That wrote a bare \"update\" file into templates/blt\n> > (without the \".sample\" suffix). After that, the cruft is forever in my\n> > build directory until I \"git clean -x\".\n> >\n> > The t5523 script tries to run \"git push --quiet\" and make sure that it\n> > produces no output, but with this setup it produces a round of \"hint\"\n> > lines (actually, they come from receive-pack on the remote end but still\n> > end up on stderr).\n> >\n> > So that raises a few interesting questions to me:\n> >\n> >   1. Should receive-pack generate these hints? They get propagated by\n> >      the client, but there's a reasonable chance that the user can't\n> >      actually fix the situation.\n> \n> True.  The user could tell the server operator to rename them or\n> remove them because they are not doing anything useful, but then\n> as long as everybody knows they are not doing anything, it is OK\n> to leave that sleeping dog lie, as they are not doing anything\n> harmful anyway.\n> \n> That brings us one step further back to question if the hints are\n> useful in the first place, though ;-).\n\nYes, that last paragraph definitely crossed my mind. Do we have an\nopinion on doing anything here? (E.g., declaring the hints not worth the\ntrouble and reverting them versus just living with it)?\n\n-Peff\n"},{"id":"341066","messageId":"xmqqo9k28blb.fsf@gitster-ct.c.googlers.com","threadId":"46889","inReplyTo":"20180303071613.GC17312@sigill.intra.peff.net","subject":"Re: [PATCH v3] run-command: add hint when a hook is ignored","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-05T21:19:12Z","receivedAt":"2018-03-05T21:19:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> True.  The user could tell the server operator to rename them or\n>> remove them because they are not doing anything useful, but then\n>> as long as everybody knows they are not doing anything, it is OK\n>> to leave that sleeping dog lie, as they are not doing anything\n>> harmful anyway.\n>> \n>> That brings us one step further back to question if the hints are\n>> useful in the first place, though ;-).\n>\n> Yes, that last paragraph definitely crossed my mind. Do we have an\n> opinion on doing anything here? (E.g., declaring the hints not worth the\n> trouble and reverting them versus just living with it)?\n\nI am tempted to declare that the \"hints\" was not a good idea and\nsuggest reverting them.\n"}]}