[PATCH] run-command.c: add hint when hook is not executable

[PATCH] run-command.c: add hint when hook is not executable

[Damien]

---
 Documentation/config.txt               | 2 ++
 advice.c                               | 2 ++
 advice.h                               | 1 +
 contrib/completion/git-completion.bash | 1 +
 run-command.c                          | 4 ++++
 5 files changed, 10 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1ac0ae6adb046..83b1884cf22fc 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -351,6 +351,8 @@ advice.*::
 	addEmbeddedRepo::
 		Advice on what to do when you've accidentally added one
 		git repo inside of another.
+	hookNotExecutable::
+		Shown when an hook is there but not executable.
 --
 
 core.fileMode::
diff --git a/advice.c b/advice.c
index d81e1cb7425b0..969ba149daeec 100644
--- a/advice.c
+++ b/advice.c
@@ -17,6 +17,7 @@ int advice_set_upstream_failure = 1;
 int advice_object_name_warning = 1;
 int advice_rm_hints = 1;
 int advice_add_embedded_repo = 1;
+int advice_hook_not_executable = 1;
 
 static struct {
 	const char *name;
@@ -38,6 +39,7 @@ static struct {
 	{ "objectnamewarning", &advice_object_name_warning },
 	{ "rmhints", &advice_rm_hints },
 	{ "addembeddedrepo", &advice_add_embedded_repo },
+	{ "hooknotexecutable", &advice_hook_not_executable},
 
 	/* make this an alias for backward compatibility */
 	{ "pushnonfastforward", &advice_push_update_rejected }
diff --git a/advice.h b/advice.h
index c84a44531c7d8..061492976b362 100644
--- a/advice.h
+++ b/advice.h
@@ -19,6 +19,7 @@ extern int advice_set_upstream_failure;
 extern int advice_object_name_warning;
 extern int advice_rm_hints;
 extern int advice_add_embedded_repo;
+extern int advice_hook_not_executable;
 
 int git_default_advice_config(const char *var, const char *value);
 __attribute__((format (printf, 1, 2)))
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index d93441747523a..6324db0c44f17 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2350,6 +2350,7 @@ _git_config ()
 		advice.rmHints
 		advice.statusHints
 		advice.statusUoption
+		advice.hookNotExecutable
 		alias.
 		am.keepcr
 		am.threeWay
diff --git a/run-command.c b/run-command.c
index b5e6eb37c0eb3..95d93a23bf87e 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1174,6 +1174,10 @@ const char *find_hook(const char *name)
 		if (access(path.buf, X_OK) >= 0)
 			return path.buf;
 #endif
+		if (advice_hook_not_executable) {
+			advise(_("The '%s' hook was ignored because it's not set as executable."
+				"\nYou can disable this warning with `git config advice.hookNotExecutable false`"), name);
+		}
 		return NULL;
 	}
 	return path.buf;

--
https://github.com/git/git/pull/411

Re: [PATCH] run-command.c: add hint when hook is not executable

[Junio C Hamano]

Damien <damien@dam.io> writes:

> ---

Please explain why this change makes sense to those who find this
commit in "git log" output six months down the line, without having
read the original thread that motivated you to add this feature
above this line with three dashes.  Use your full name on the From:
header of your mail (or alternatively, you can use an in-body "From:"
to override what your MUA places there) and add sign-off with the
same name (cf. Documentation/SubmittingPatches).

>  Documentation/config.txt               | 2 ++
>  advice.c                               | 2 ++
>  advice.h                               | 1 +
>  contrib/completion/git-completion.bash | 1 +
>  run-command.c                          | 4 ++++
>  5 files changed, 10 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 1ac0ae6adb046..83b1884cf22fc 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -351,6 +351,8 @@ advice.*::
>  	addEmbeddedRepo::
>  		Advice on what to do when you've accidentally added one
>  		git repo inside of another.
> +	hookNotExecutable::
> +		Shown when an hook is there but not executable.
>  --

"Shown when" does not tell readers what is shown; many of the other
entries in this list does so, and it is helpful to choose from the
list when a user encounters one of these advice messages and wants
to squelch it.

> diff --git a/advice.c b/advice.c
> diff --git a/advice.h b/advice.h
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash

The changes to these files looked good.

> diff --git a/run-command.c b/run-command.c
> index b5e6eb37c0eb3..95d93a23bf87e 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1174,6 +1174,10 @@ const char *find_hook(const char *name)
>  		if (access(path.buf, X_OK) >= 0)
>  			return path.buf;
>  #endif
> +		if (advice_hook_not_executable) {
> +			advise(_("The '%s' hook was ignored because it's not set as executable."
> +				"\nYou can disable this warning with `git config advice.hookNotExecutable false`"), name);
> +		}
>  		return NULL;

As to the string constant, it is somewhat strange to see it is
chomped into two and the LF in the middle is given to the latter
half.  If you are going to chomp anyway, perhaps you'd want to chomp
it further to avoid making the source line too long.

But more importantly, is this checking the right thing?  Before the
pre-context of this hunk is:

	if (access(path.buf, X_OK) < 0) {

so we know access(X_OK) failed.  And we didn't have to care how/why
it failed because the only thing we did was to return NULL when we
decide there is no executable hook.

But for the purpose of your patch, you now do care.  access(X_OK)
may have failed with EACCESS (i.e. you have no permission to run the
script), in which case your new advise() call may make sense.  But
it may have failed with ENOENT or ENOTDIR.  And your new advise()
call is a disaster, if the user didn't even have such a hook.

Writing a test would have helped notice this, I would think.  You'd
need at least the following variations to cover possibilities:

 - Without the advise.* configuration, install an executable hook
   for a command and try to run the command.  Make sure you do not
   see any advise message shown.

 - Drop the executable bit from the hook from the above and run the
   same command.  Make sure you do see the advise message.  You'd
   probably need to protect this test piece with POSIXPERM
   prerequisite.

 - Set advise.* configuration to squelch and run the same command.
   Make sure you do not see the advise message.

 - Remove advise.* configuration and the hook, and run the same
   command.  Make sure you do not see the advise message.

Re: [PATCH] run-command.c: add hint when hook is not executable

[Damien]

On Wed, Oct 4, 2017 at 6:40 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Damien <damien@dam.io> writes:
>
>> ---
>
> Please explain why this change makes sense to those who find this
> commit in "git log" output six months down the line, without having
> read the original thread that motivated you to add this feature
> above this line with three dashes.  Use your full name on the From:
> header of your mail (or alternatively, you can use an in-body "From:"
> to override what your MUA places there) and add sign-off with the
> same name (cf. Documentation/SubmittingPatches).

Thanks, I'm gonna post another version via submitGit. I'm not sure
how to send the "cover letter" via submitGit so I'm just gonna explain
the patch here and then send it in another message.

First, I changed the name of the config to "advice.ignoredHook"
since it's more generic and it will allow us to add other cases
such as the bad permission case..

> But for the purpose of your patch, you now do care.  access(X_OK)
> may have failed with EACCESS (i.e. you have no permission to run the
> script), in which case your new advise() call may make sense.  But
> it may have failed with ENOENT or ENOTDIR.  And your new advise()
> call is a disaster, if the user didn't even have such a hook.

For sure, the previous version was really broken. I've now added a
EACCES check. But I see two shortcomings left:

  - the message is shown twice (for a "pre-commit" hook at least)
  - I tried to take into account the STRIP_EXTENSION case but
    it's not tested yet

> Writing a test would have helped notice this, I would think.  You'd
> need at least the following variations to cover possibilities:

As you will see, I've added tests but my scripting skills are a bit
lacking and I used a simple "if...then false else true". I've tried
`grep -v` and similar things but this solution is the only one that
I found that really works.

Thanks for the review, see PATCH in a coming email.

[PATCH v2] run-command: add hint when a hook is ignored

[Damien Marié]

When an hook is present but the file is not set as executable then git will
ignore the hook.
For now this is silent which can be confusing.

This commit adds this warning to improve the situation:

  hint: The 'pre-commit' hook was ignored because it's not set as executable.
  hint: You can disable this warning with `git config advice.ignoredHook false`

To allow the old use-case of enabling/disabling hooks via the executable flag a
new setting is introduced: advice.ignoredHook.

Signed-off-by: Damien Marié <damien@dam.io>
---
 Documentation/config.txt               |  3 ++
 advice.c                               |  2 +
 advice.h                               |  1 +
 contrib/completion/git-completion.bash |  1 +
 run-command.c                          | 10 +++++
 t/t7519-ignored-hook-warning.sh        | 67 ++++++++++++++++++++++++++++++++++
 6 files changed, 84 insertions(+)
 create mode 100755 t/t7519-ignored-hook-warning.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1ac0ae6adb046..9abca499f725c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -351,6 +351,9 @@ advice.*::
 	addEmbeddedRepo::
 		Advice on what to do when you've accidentally added one
 		git repo inside of another.
+	ignoredHook::
+		Advice shown if an hook is ignored because the it's not
+		set as executable.
 --
 
 core.fileMode::
diff --git a/advice.c b/advice.c
index d81e1cb7425b0..970bd2b71bf53 100644
--- a/advice.c
+++ b/advice.c
@@ -17,6 +17,7 @@ int advice_set_upstream_failure = 1;
 int advice_object_name_warning = 1;
 int advice_rm_hints = 1;
 int advice_add_embedded_repo = 1;
+int advice_ignored_hook = 1;
 
 static struct {
 	const char *name;
@@ -38,6 +39,7 @@ static struct {
 	{ "objectnamewarning", &advice_object_name_warning },
 	{ "rmhints", &advice_rm_hints },
 	{ "addembeddedrepo", &advice_add_embedded_repo },
+	{ "ignoredhook", &advice_ignored_hook},
 
 	/* make this an alias for backward compatibility */
 	{ "pushnonfastforward", &advice_push_update_rejected }
diff --git a/advice.h b/advice.h
index c84a44531c7d8..f525d6f89cb44 100644
--- a/advice.h
+++ b/advice.h
@@ -19,6 +19,7 @@ extern int advice_set_upstream_failure;
 extern int advice_object_name_warning;
 extern int advice_rm_hints;
 extern int advice_add_embedded_repo;
+extern int advice_ignored_hook;
 
 int git_default_advice_config(const char *var, const char *value);
 __attribute__((format (printf, 1, 2)))
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index d93441747523a..a331ccc556b01 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2350,6 +2350,7 @@ _git_config ()
 		advice.rmHints
 		advice.statusHints
 		advice.statusUoption
+		advice.ignoredHook
 		alias.
 		am.keepcr
 		am.threeWay
diff --git a/run-command.c b/run-command.c
index b5e6eb37c0eb3..9456355595cc1 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1169,11 +1169,21 @@ const char *find_hook(const char *name)
 	strbuf_reset(&path);
 	strbuf_git_path(&path, "hooks/%s", name);
 	if (access(path.buf, X_OK) < 0) {
+		int err = errno;
 #ifdef STRIP_EXTENSION
 		strbuf_addstr(&path, STRIP_EXTENSION);
 		if (access(path.buf, X_OK) >= 0)
 			return path.buf;
+		else if (errno == EACCES)
+			err = errno;
 #endif
+		if (err == EACCES && advice_ignored_hook) {
+			advise(_(
+				"The '%s' hook was ignored because "
+				"it's not set as executable.\n"
+				"You can disable this warning with "
+				"`git config advice.ignoredHook false`."), path.buf);
+		}
 		return NULL;
 	}
 	return path.buf;
diff --git a/t/t7519-ignored-hook-warning.sh b/t/t7519-ignored-hook-warning.sh
new file mode 100755
index 0000000000000..59052a4429111
--- /dev/null
+++ b/t/t7519-ignored-hook-warning.sh
@@ -0,0 +1,67 @@
+#!/bin/sh
+
+test_description='ignored hook warning'
+
+. ./test-lib.sh
+
+# install hook that always succeeds
+HOOKDIR="$(git rev-parse --git-dir)/hooks"
+HOOK="$HOOKDIR/pre-commit"
+mkdir -p "$HOOKDIR"
+cat > "$HOOK" <<EOF
+#!/bin/sh
+exit 0
+EOF
+
+chmod +x "$HOOK"
+
+test_expect_success 'no warning if proper hook' '
+
+    if git commit -m "more" 2>&1 >/dev/null | grep hint
+    then
+        false
+    else
+        true
+    fi
+
+'
+
+chmod -x "$HOOK"
+
+test_expect_success POSIXPERM 'warning if hook not set as executable' '
+
+    if git commit -m "more" 2>&1 >/dev/null | grep hint
+    then
+        true
+    else
+        false
+    fi
+'
+
+test_expect_success 'no warning if advice.ignoredHook set to false' '
+
+    git config advice.ignoredHook false &&
+    if git commit -m "more" 2>&1 >/dev/null | grep hint
+    then
+        false
+    else
+        true
+    fi
+'
+
+rm "$HOOK"
+
+test_expect_success 'no warning if unset advice.ignoredHook and hook removed' '
+
+    git config --unset advice.ignoredHook &&
+    if git commit -m "more" 2>&1 >/dev/null | grep hint
+    then
+        false
+    else
+        true
+    fi
+'
+git commit -m "more" 2>&1 >/dev/null
+
+
+test_done
\ No newline at end of file

--
https://github.com/git/git/pull/411

Re: [PATCH v2] run-command: add hint when a hook is ignored

[Junio C Hamano]

Damien Marié <damien@dam.io> writes:

>  	if (access(path.buf, X_OK) < 0) {
> +		int err = errno;

OK, so we remember how/why we failed in err.

>  #ifdef STRIP_EXTENSION
>  		strbuf_addstr(&path, STRIP_EXTENSION);
>  		if (access(path.buf, X_OK) >= 0)
>  			return path.buf;
> +		else if (errno == EACCES)
> +			err = errno;

I think it is easier to reason about if this were not "else if", but
just a simple "if".

 - We tried foo-hook, and failed.
 - On a platform that foo-hook.exe can also be a hook
   - We try foo-hook.exe and if it seems OK, we return with smile.
   - If not, if we know foo-hook.exe exists but we cannot execute,
     we update err (forgetting the reason why foo-hook was wrong)
     with the reason why foo-hook.exe is bad.

It is OK to forget why foo-hook was unhappy, as on a STRIP_EXTENSION
build, we would have tried to run foo-hook.exe anyway.

>  #endif

So at this point, with or without STRIP_EXTENSION, err tells us why
the file we wanted to be available as a hook did not pass our
criteria.

> +		if (err == EACCES && advice_ignored_hook) {

And we want to do the advise thing only if we know we failed due to
EACCES and for no other reason.

> +			advise(_(
> +				"The '%s' hook was ignored because "
> +				"it's not set as executable.\n"
> +				"You can disable this warning with "
> +				"`git config advice.ignoredHook false`."), path.buf);
> +		}
>  		return NULL;
>  	}
>  	return path.buf;

Overall, the logic looks correct to me.  Note that we may have
gotten EACCES not because the path lacked the executable bit, but
because the hook directory was unreadable ;-), but in such a case,
you cannot tell if "it's not set as executable" is true anyway.

> diff --git a/t/t7519-ignored-hook-warning.sh b/t/t7519-ignored-hook-warning.sh
> new file mode 100755
> index 0000000000000..59052a4429111
> --- /dev/null
> +++ b/t/t7519-ignored-hook-warning.sh
> @@ -0,0 +1,67 @@
> +#!/bin/sh
> +
> +test_description='ignored hook warning'
> +
> +. ./test-lib.sh
> +

These days, things like this...

> +# install hook that always succeeds
> +HOOKDIR="$(git rev-parse --git-dir)/hooks"
> +HOOK="$HOOKDIR/pre-commit"
> +mkdir -p "$HOOKDIR"
> +cat > "$HOOK" <<EOF
> +#!/bin/sh
> +exit 0
> +EOF
> +
> +chmod +x "$HOOK"

...should all go to test_expect_success, i.e.

	test_expect_success setup '
		...
		mkdir -p "$hookdir" &&
		write_script "$hookdir/pre-commit" <<-\EOF
		exit 0
		EOF
	'

write_script takes care of flipping +x on.

> +test_expect_success 'no warning if proper hook' '
> +
> +    if git commit -m "more" 2>&1 >/dev/null | grep hint
> +    then
> +        false
> +    else
> +        true
> +    fi
> +

 - Indents in our shell scripts are done with tab (HT).

 - We try to avoid running git command on the LHS of a pipe when we
   do not have to.

 - "git commit" may fail due to not having anything worth
   committing, even before it notices that pre-commit hook is or is
   not executable.  Avoid relying on the order of things that happen
   to be true in the current implementation when we do not have to.

 - We may see some other hint.  Avoid relying on the set of advises
   that happens to currently be defined when we do not have to.

 - Output from advise() can be localized, so grepping to expect
   something either is there or is not there would be triggered as
   an error in GETTEXT_POISON build.  We unfortunately need to use
   test_i18ngrep to work it around.

Perhaps the above should become more like so:

	git commit --allow-empty -m more 2>message &&
	test_i18ngrep ! "hook was ignored" message

> +'
> +
> +chmod -x "$HOOK"

Move this to the beginning of the next one that is protected with
POSIXPERM.

> +test_expect_success POSIXPERM 'warning if hook not set as executable' '
> +
> +    if git commit -m "more" 2>&1 >/dev/null | grep hint
> +    then
> +        true
> +    else
> +        false
> +    fi
> +'

	chmod -x "$hookdir/pre-commit" &&
	git commit --allow-empty -m "even more" 2>message &&
	test_i18ngrep "hook was ignored" message

or something like that.

Thanks.

Re: [PATCH v2] run-command: add hint when a hook is ignored

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> diff --git a/t/t7519-ignored-hook-warning.sh b/t/t7519-ignored-hook-warning.sh
>> new file mode 100755

Another thing; t7519 is taken by another topic in flight.  Let's use
t7520 instead.

Re: [PATCH v2] run-command: add hint when a hook is ignored

[Junio C Hamano]

Here is a suggested rewrite of t7519 (I used t7520 to avoid crashing
with another topic in flight).

 - use unused/unallocated 7520 to avoid crashes with bp/fsmonitor
   topic

 - use setup inside test_expect_success

 - use test_i18ngrep to avoid gettext-poison build gotchas

 - look for specific advise message to make the test more robust

 - do not run Git on LHS of a pipe when we do not have to

 - use test_config and test_unconfig


 t/t7520-ignored-hook-warning.sh | 41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100755 t/t7520-ignored-hook-warning.sh

diff --git a/t/t7520-ignored-hook-warning.sh b/t/t7520-ignored-hook-warning.sh
new file mode 100755
index 0000000000..634fb7f23a
--- /dev/null
+++ b/t/t7520-ignored-hook-warning.sh
@@ -0,0 +1,41 @@
+#!/bin/sh
+
+test_description='ignored hook warning'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	hookdir="$(git rev-parse --git-dir)/hooks" &&
+	hook="$hookdir/pre-commit" &&
+	mkdir -p "$hookdir" &&
+	write_script "$hook" <<-\EOF
+	exit 0
+	EOF
+'
+
+test_expect_success 'no warning if hook is not ignored' '
+	git commit --allow-empty -m "more" 2>message &&
+	test_i18ngrep ! -e "hook was ignored" message
+'
+
+test_expect_success POSIXPERM 'warning if hook is ignored' '
+	chmod -x "$hook" &&
+	git commit --allow-empty -m "even more" 2>message &&
+	test_i18ngrep -e "hook was ignored" message
+'
+
+test_expect_success POSIXPERM 'no warning if advice.ignoredHook set to false' '
+	test_config advice.ignoredHook false &&
+	chmod -x "$hook" &&
+	git commit --allow-empty -m "even more" 2>message &&
+	test_i18ngrep ! -e "hook was ignored" message
+'
+
+test_expect_success 'no warning if unset advice.ignoredHook and hook removed' '
+	rm -f "$hook" &&
+	test_unconfig advice.ignoredHook &&
+	git commit --allow-empty -m "even more" 2>message &&
+	test_i18ngrep ! -e "hook was ignored" message
+'
+
+test_done
-- 
2.15.0-rc0-155-g07e9c1a78d

Re: [PATCH v2] run-command: add hint when a hook is ignored

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I think it is easier to reason about if this were not "else if", but
> just a simple "if".

And here are two small suggested changes to the code portion of your
patch.

 - break if / else if cascade into two independent if / if
   statements for clarity.

 - give the "ignored hook" advice only once per <process,hook>
   pair.



 run-command.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/run-command.c b/run-command.c
index 0f8a5f7fa2..0e60dd2075 100644
--- a/run-command.c
+++ b/run-command.c
@@ -5,6 +5,7 @@
 #include "argv-array.h"
 #include "thread-utils.h"
 #include "strbuf.h"
+#include "string-list.h"
 
 void child_process_init(struct child_process *child)
 {
@@ -1170,19 +1171,25 @@ const char *find_hook(const char *name)
 	strbuf_git_path(&path, "hooks/%s", name);
 	if (access(path.buf, X_OK) < 0) {
 		int err = errno;
+
 #ifdef STRIP_EXTENSION
 		strbuf_addstr(&path, STRIP_EXTENSION);
 		if (access(path.buf, X_OK) >= 0)
 			return path.buf;
-		else if (errno == EACCES)
+		if (errno == EACCES)
 			err = errno;
 #endif
 		if (err == EACCES && advice_ignored_hook) {
-			advise(_(
-				"The '%s' hook was ignored because "
-				"it's not set as executable.\n"
-				"You can disable this warning with "
-				"`git config advice.ignoredHook false`."), path.buf);
+			static struct string_list advise_given = STRING_LIST_INIT_DUP;
+
+			if (!string_list_lookup(&advise_given, name)) {
+				string_list_insert(&advise_given, name);
+				advise(_("The '%s' hook was ignored because "
+					 "it's not set as executable.\n"
+					 "You can disable this warning with "
+					 "`git config advice.ignoredHook false`."),
+				       path.buf);
+			}
 		}
 		return NULL;
 	}
-- 
2.15.0-rc0-155-g07e9c1a78d

Re: [PATCH v2] run-command: add hint when a hook is ignored

[Damien]

Thanks for the help, a new patch is coming with those fixes applied.

On Fri, Oct 6, 2017 at 7:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I think it is easier to reason about if this were not "else if", but
>> just a simple "if".
>
> And here are two small suggested changes to the code portion of your
> patch.
>
>  - break if / else if cascade into two independent if / if
>    statements for clarity.
>
>  - give the "ignored hook" advice only once per <process,hook>
>    pair.
>
>
>
>  run-command.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index 0f8a5f7fa2..0e60dd2075 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -5,6 +5,7 @@
>  #include "argv-array.h"
>  #include "thread-utils.h"
>  #include "strbuf.h"
> +#include "string-list.h"
>
>  void child_process_init(struct child_process *child)
>  {
> @@ -1170,19 +1171,25 @@ const char *find_hook(const char *name)
>         strbuf_git_path(&path, "hooks/%s", name);
>         if (access(path.buf, X_OK) < 0) {
>                 int err = errno;
> +
>  #ifdef STRIP_EXTENSION
>                 strbuf_addstr(&path, STRIP_EXTENSION);
>                 if (access(path.buf, X_OK) >= 0)
>                         return path.buf;
> -               else if (errno == EACCES)
> +               if (errno == EACCES)
>                         err = errno;
>  #endif
>                 if (err == EACCES && advice_ignored_hook) {
> -                       advise(_(
> -                               "The '%s' hook was ignored because "
> -                               "it's not set as executable.\n"
> -                               "You can disable this warning with "
> -                               "`git config advice.ignoredHook false`."), path.buf);
> +                       static struct string_list advise_given = STRING_LIST_INIT_DUP;
> +
> +                       if (!string_list_lookup(&advise_given, name)) {
> +                               string_list_insert(&advise_given, name);
> +                               advise(_("The '%s' hook was ignored because "
> +                                        "it's not set as executable.\n"
> +                                        "You can disable this warning with "
> +                                        "`git config advice.ignoredHook false`."),
> +                                      path.buf);
> +                       }
>                 }
>                 return NULL;
>         }
> --
> 2.15.0-rc0-155-g07e9c1a78d
>

[PATCH v3] run-command: add hint when a hook is ignored

[Damien Marié]

When an hook is present but the file is not set as executable then git will
ignore the hook.
For now this is silent which can be confusing.

This commit adds this warning to improve the situation:

  hint: The 'pre-commit' hook was ignored because it's not set as executable.
  hint: You can disable this warning with `git config advice.ignoredHook false`

To allow the old use-case of enabling/disabling hooks via the executable flag a
new setting is introduced: advice.ignoredHook.

Signed-off-by: Damien Marié <damien@dam.io>
---
 Documentation/config.txt               |  3 +++
 advice.c                               |  2 ++
 advice.h                               |  1 +
 contrib/completion/git-completion.bash |  1 +
 run-command.c                          | 18 +++++++++++++++
 t/t7520-ignored-hook-warning.sh        | 41 ++++++++++++++++++++++++++++++++++
 6 files changed, 66 insertions(+)
 create mode 100755 t/t7520-ignored-hook-warning.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1ac0ae6adb046..9abca499f725c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -351,6 +351,9 @@ advice.*::
 	addEmbeddedRepo::
 		Advice on what to do when you've accidentally added one
 		git repo inside of another.
+	ignoredHook::
+		Advice shown if an hook is ignored because the it's not
+		set as executable.
 --
 
 core.fileMode::
diff --git a/advice.c b/advice.c
index d81e1cb7425b0..970bd2b71bf53 100644
--- a/advice.c
+++ b/advice.c
@@ -17,6 +17,7 @@ int advice_set_upstream_failure = 1;
 int advice_object_name_warning = 1;
 int advice_rm_hints = 1;
 int advice_add_embedded_repo = 1;
+int advice_ignored_hook = 1;
 
 static struct {
 	const char *name;
@@ -38,6 +39,7 @@ static struct {
 	{ "objectnamewarning", &advice_object_name_warning },
 	{ "rmhints", &advice_rm_hints },
 	{ "addembeddedrepo", &advice_add_embedded_repo },
+	{ "ignoredhook", &advice_ignored_hook},
 
 	/* make this an alias for backward compatibility */
 	{ "pushnonfastforward", &advice_push_update_rejected }
diff --git a/advice.h b/advice.h
index c84a44531c7d8..f525d6f89cb44 100644
--- a/advice.h
+++ b/advice.h
@@ -19,6 +19,7 @@ extern int advice_set_upstream_failure;
 extern int advice_object_name_warning;
 extern int advice_rm_hints;
 extern int advice_add_embedded_repo;
+extern int advice_ignored_hook;
 
 int git_default_advice_config(const char *var, const char *value);
 __attribute__((format (printf, 1, 2)))
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index d93441747523a..a331ccc556b01 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2350,6 +2350,7 @@ _git_config ()
 		advice.rmHints
 		advice.statusHints
 		advice.statusUoption
+		advice.ignoredHook
 		alias.
 		am.keepcr
 		am.threeWay
diff --git a/run-command.c b/run-command.c
index b5e6eb37c0eb3..288abbba5afd7 100644
--- a/run-command.c
+++ b/run-command.c
@@ -5,6 +5,7 @@
 #include "argv-array.h"
 #include "thread-utils.h"
 #include "strbuf.h"
+#include "string-list.h"
 
 void child_process_init(struct child_process *child)
 {
@@ -1169,11 +1170,28 @@ const char *find_hook(const char *name)
 	strbuf_reset(&path);
 	strbuf_git_path(&path, "hooks/%s", name);
 	if (access(path.buf, X_OK) < 0) {
+		int err = errno;
+
 #ifdef STRIP_EXTENSION
 		strbuf_addstr(&path, STRIP_EXTENSION);
 		if (access(path.buf, X_OK) >= 0)
 			return path.buf;
+		if (errno == EACCES)
+			err = errno;
 #endif
+
+		if (err == EACCES && advice_ignored_hook) {
+			static struct string_list advise_given = STRING_LIST_INIT_DUP;
+			
+			if (!string_list_lookup(&advise_given, name)) {
+				string_list_insert(&advise_given, name);
+				advise(_("The '%s' hook was ignored because "
+							"it's not set as executable.\n"
+							"You can disable this warning with "
+							"`git config advice.ignoredHook false`."),
+						path.buf);
+			}
+		}
 		return NULL;
 	}
 	return path.buf;
diff --git a/t/t7520-ignored-hook-warning.sh b/t/t7520-ignored-hook-warning.sh
new file mode 100755
index 0000000000000..634fb7f23a040
--- /dev/null
+++ b/t/t7520-ignored-hook-warning.sh
@@ -0,0 +1,41 @@
+#!/bin/sh
+
+test_description='ignored hook warning'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	hookdir="$(git rev-parse --git-dir)/hooks" &&
+	hook="$hookdir/pre-commit" &&
+	mkdir -p "$hookdir" &&
+	write_script "$hook" <<-\EOF
+	exit 0
+	EOF
+'
+
+test_expect_success 'no warning if hook is not ignored' '
+	git commit --allow-empty -m "more" 2>message &&
+	test_i18ngrep ! -e "hook was ignored" message
+'
+
+test_expect_success POSIXPERM 'warning if hook is ignored' '
+	chmod -x "$hook" &&
+	git commit --allow-empty -m "even more" 2>message &&
+	test_i18ngrep -e "hook was ignored" message
+'
+
+test_expect_success POSIXPERM 'no warning if advice.ignoredHook set to false' '
+	test_config advice.ignoredHook false &&
+	chmod -x "$hook" &&
+	git commit --allow-empty -m "even more" 2>message &&
+	test_i18ngrep ! -e "hook was ignored" message
+'
+
+test_expect_success 'no warning if unset advice.ignoredHook and hook removed' '
+	rm -f "$hook" &&
+	test_unconfig advice.ignoredHook &&
+	git commit --allow-empty -m "even more" 2>message &&
+	test_i18ngrep ! -e "hook was ignored" message
+'
+
+test_done

--
https://github.com/git/git/pull/411

Re: [PATCH v3] run-command: add hint when a hook is ignored

[Junio C Hamano]

Damien Marié <damien@dam.io> writes:

> When an hook is present but the file is not set as executable then git will
> ignore the hook.
> For now this is silent which can be confusing.

Quite honestly, I do not particulary think this is confusing, and I
expect that this change will irritate many people by forcing them to
either set the advise config or move the ones that they deliberately
left unexecutable by renaming them by adding ".disabled" at the end.

But these remedies are easy enough, so let's see how well it works
by merging it to 'next' and cooking it there for a while.

I've spotted two issues during the last-minute scan; I'll squash the
fixes in in the meantime (if you are not going to change anything
other than these two, there is no need to resend corrected patches).

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 1ac0ae6adb046..9abca499f725c 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -351,6 +351,9 @@ advice.*::
>  	addEmbeddedRepo::
>  		Advice on what to do when you've accidentally added one
>  		git repo inside of another.
> +	ignoredHook::
> +		Advice shown if an hook is ignored because the it's not
> +		set as executable.

s/the it's not/the hook is not/;

> @@ -38,6 +39,7 @@ static struct {
>  	{ "objectnamewarning", &advice_object_name_warning },
>  	{ "rmhints", &advice_rm_hints },
>  	{ "addembeddedrepo", &advice_add_embedded_repo },
> +	{ "ignoredhook", &advice_ignored_hook},

s/ignored_hook/& /;

Re: [PATCH v3] run-command: add hint when a hook is ignored

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Quite honestly, I do not particulary think this is confusing, and I
> expect that this change will irritate many people by forcing them to
> either set the advise config or move the ones that they deliberately
> left unexecutable by renaming them by adding ".disabled" at the end.
>
> But these remedies are easy enough, so let's see how well it works
> by merging it to 'next' and cooking it there for a while.

Well, it turns out that I am among those who are irritated, as all
the repositories I work with were rather old, dating back to 2005,
back when it was a norm to have these sample files installed without
executable bit, to make it easy for those who choose to use them
as-is to enable them by flipping the executable bit.

And I do not find the advice.ignoredhook is giving particularly a
good piece of advice.  I suspect that it would be a better practice
to rename a disabled foo-hook to foo-hook.disabled if the user wants
to squelch the warning.  It gives them a final chance to review what
they left disabled for all these years, and then choose to either
remove it, or rename it to foo-hook.disabled.  "ls .git/hooks/" will
then make it clear which ones are disabled without the "-F" option,
which is an additional benefit.

Anyway, I am not merging this topic to the upcoming release, so
hopefully we'll hear from others who try 'next'.

Thanks.

Re: [PATCH v3] run-command: add hint when a hook is ignored

[Jeff King]

On Wed, Oct 11, 2017 at 03:26:43PM +0900, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Quite honestly, I do not particulary think this is confusing, and I
> > expect that this change will irritate many people by forcing them to
> > either set the advise config or move the ones that they deliberately
> > left unexecutable by renaming them by adding ".disabled" at the end.
> >
> > But these remedies are easy enough, so let's see how well it works
> > by merging it to 'next' and cooking it there for a while.
> 
> Well, it turns out that I am among those who are irritated, as all
> the repositories I work with were rather old, dating back to 2005,
> back when it was a norm to have these sample files installed without
> executable bit, to make it easy for those who choose to use them
> as-is to enable them by flipping the executable bit.
> [...]
> Anyway, I am not merging this topic to the upcoming release, so
> hopefully we'll hear from others who try 'next'.

This bit me today in a funny way: t5523 started failing.

The problem is that I was bisecting an unrelated change in old code, and
I built a commit which predates f98f8cbac0 (Ship sample hooks with .sample
suffix, 2008-06-24). That wrote a bare "update" file into templates/blt
(without the ".sample" suffix). After that, the cruft is forever in my
build directory until I "git clean -x".

The t5523 script tries to run "git push --quiet" and make sure that it
produces no output, but with this setup it produces a round of "hint"
lines (actually, they come from receive-pack on the remote end but still
end up on stderr).

So that raises a few interesting questions to me:

  1. Should receive-pack generate these hints? They get propagated by
     the client, but there's a reasonable chance that the user can't
     actually fix the situation.

  2. Should these hints be suppressed with --quiet? I can see an
     argument that "--quiet" only applies to non-errors. And while these
     are not fatal errors, they're outside the realm of the usual
     chattiness.

  3. Should our tests be more careful about not looking at the
     template hooks? I think test_create_repo already disables the hooks
     directory manually, but many repos will be created by "git clone"
     during the tests.

  4. Should our build system be more clever about dropping non-existent
     files from templates/blt?

I started down the road of saying "--quiet should disable all advice",
and that patch is below. But I'm having second thoughts on it. It fixes
the case in receive-pack, but what should "commit --quiet" do? It's
documented only to suppress the status output after commit. Should it
cover this case? For that matter, the "quiet" protocol extension which
is passed to receive-pack is generally used only for progress reporting,
and is sent automatically when stderr isn't a tty (which is why the
tests below must go through some contortions with test_terminal). I'm
not sure if it should actually apply to advice.

---
 advice.c                        |  8 ++++++++
 advice.h                        |  6 ++++++
 builtin/receive-pack.c          |  4 +++-
 t/t7520-ignored-hook-warning.sh | 15 +++++++++++++++
 4 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/advice.c b/advice.c
index 406efc183b..f754af8abe 100644
--- a/advice.c
+++ b/advice.c
@@ -137,3 +137,11 @@ void detach_advice(const char *new_name)
 
 	fprintf(stderr, fmt, new_name);
 }
+
+void disable_advice(void)
+{
+	size_t i;
+
+	for (i = 0; i < ARRAY_SIZE(advice_config); i++)
+		*advice_config[i].preference = 0;
+}
diff --git a/advice.h b/advice.h
index 70568fa792..4895554ef3 100644
--- a/advice.h
+++ b/advice.h
@@ -30,4 +30,10 @@ extern void NORETURN die_resolve_conflict(const char *me);
 void NORETURN die_conclude_merge(void);
 void detach_advice(const char *new_name);
 
+/*
+ * Turn off all advice flags; this can be used to centrally enforce a --quiet
+ * option.
+ */
+void disable_advice(void);
+
 #endif /* ADVICE_H */
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index b7ce7c7f52..f257c16776 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1601,8 +1601,10 @@ static struct command *read_head_info(struct oid_array *shallow)
 				report_status = 1;
 			if (parse_feature_request(feature_list, "side-band-64k"))
 				use_sideband = LARGE_PACKET_MAX;
-			if (parse_feature_request(feature_list, "quiet"))
+			if (parse_feature_request(feature_list, "quiet")) {
 				quiet = 1;
+				disable_advice();
+			}
 			if (advertise_atomic_push
 			    && parse_feature_request(feature_list, "atomic"))
 				use_atomic = 1;
diff --git a/t/t7520-ignored-hook-warning.sh b/t/t7520-ignored-hook-warning.sh
index 634fb7f23a..1d27d3e3f0 100755
--- a/t/t7520-ignored-hook-warning.sh
+++ b/t/t7520-ignored-hook-warning.sh
@@ -3,6 +3,7 @@
 test_description='ignored hook warning'
 
 . ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-terminal.sh
 
 test_expect_success setup '
 	hookdir="$(git rev-parse --git-dir)/hooks" &&
@@ -38,4 +39,18 @@ test_expect_success 'no warning if unset advice.ignoredHook and hook removed' '
 	test_i18ngrep ! -e "hook was ignored" message
 '
 
+test_expect_success TTY,POSIXPERM 'push --quiet silences remote hook warnings' '
+	git init --bare dst.git &&
+	echo "exit 0" >dst.git/hooks/update &&
+	chmod -x dst.git/hooks/update &&
+
+	git commit --allow-empty -m one &&
+	test_terminal git push dst.git HEAD 2>message &&
+	test_i18ngrep -e "hook was ignored" message &&
+
+	git commit --allow-empty -m two &&
+	test_terminal git push --quiet dst.git HEAD 2>message &&
+	test_i18ngrep ! -e "hook was ignored" message
+'
+
 test_done
-- 
2.16.0.rc0.384.gc477e89267

Re: [PATCH v3] run-command: add hint when a hook is ignored

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The problem is that I was bisecting an unrelated change in old code, and
> I built a commit which predates f98f8cbac0 (Ship sample hooks with .sample
> suffix, 2008-06-24). That wrote a bare "update" file into templates/blt
> (without the ".sample" suffix). After that, the cruft is forever in my
> build directory until I "git clean -x".
>
> The t5523 script tries to run "git push --quiet" and make sure that it
> produces no output, but with this setup it produces a round of "hint"
> lines (actually, they come from receive-pack on the remote end but still
> end up on stderr).
>
> So that raises a few interesting questions to me:
>
>   1. Should receive-pack generate these hints? They get propagated by
>      the client, but there's a reasonable chance that the user can't
>      actually fix the situation.

True.  The user could tell the server operator to rename them or
remove them because they are not doing anything useful, but then
as long as everybody knows they are not doing anything, it is OK
to leave that sleeping dog lie, as they are not doing anything
harmful anyway.

That brings us one step further back to question if the hints are
useful in the first place, though ;-).

>   2. Should these hints be suppressed with --quiet? I can see an
>      argument that "--quiet" only applies to non-errors. And while these
>      are not fatal errors, they're outside the realm of the usual
>      chattiness.

I do not think having a non-executable file whose name happens to be
the same as a hook is an error in the first place [*1*], so I do not
think it is unreasonable for --quiet to squelch.

	side note [*1*]: we need to make sure that it is clearly
	documented that such a file is not a hook, of course.

>   3. Should our tests be more careful about not looking at the
>      template hooks? I think test_create_repo already disables the hooks
>      directory manually, but many repos will be created by "git clone"
>      during the tests.

This probably is much deeper issue and templates/hooks--* is merely
a tip of iceberg, I suspect.  In general build artifacts from
different versions of Git in the same build area could interfere the
tests in other ways (e.g. a test for "git help" output could try to
grab a deprecated command left in bin-wrappers/ from previous build
of an ancient version), and could potentially interfere things other
than the tests (e.g. the 'install' target of the Makefile may be
written loosely to install everything in a directory).

In general, I am a bit pessimistic and suspect that the best we
could say is "if you want to absolutely avoid such interference,
'make distclean' before switching to test another revision".

>   4. Should our build system be more clever about dropping non-existent
>      files from templates/blt?

It would be a reasonable mitigation that is specific to templates/,
I suspect.

Re: [PATCH v3] run-command: add hint when a hook is ignored

[Jeff King]

On Fri, Jan 05, 2018 at 11:36:39AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > The problem is that I was bisecting an unrelated change in old code, and
> > I built a commit which predates f98f8cbac0 (Ship sample hooks with .sample
> > suffix, 2008-06-24). That wrote a bare "update" file into templates/blt
> > (without the ".sample" suffix). After that, the cruft is forever in my
> > build directory until I "git clean -x".
> >
> > The t5523 script tries to run "git push --quiet" and make sure that it
> > produces no output, but with this setup it produces a round of "hint"
> > lines (actually, they come from receive-pack on the remote end but still
> > end up on stderr).
> >
> > So that raises a few interesting questions to me:
> >
> >   1. Should receive-pack generate these hints? They get propagated by
> >      the client, but there's a reasonable chance that the user can't
> >      actually fix the situation.
> 
> True.  The user could tell the server operator to rename them or
> remove them because they are not doing anything useful, but then
> as long as everybody knows they are not doing anything, it is OK
> to leave that sleeping dog lie, as they are not doing anything
> harmful anyway.
> 
> That brings us one step further back to question if the hints are
> useful in the first place, though ;-).

Yes, that last paragraph definitely crossed my mind. Do we have an
opinion on doing anything here? (E.g., declaring the hints not worth the
trouble and reverting them versus just living with it)?

-Peff

Re: [PATCH v3] run-command: add hint when a hook is ignored

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> True.  The user could tell the server operator to rename them or
>> remove them because they are not doing anything useful, but then
>> as long as everybody knows they are not doing anything, it is OK
>> to leave that sleeping dog lie, as they are not doing anything
>> harmful anyway.
>> 
>> That brings us one step further back to question if the hints are
>> useful in the first place, though ;-).
>
> Yes, that last paragraph definitely crossed my mind. Do we have an
> opinion on doing anything here? (E.g., declaring the hints not worth the
> trouble and reverting them versus just living with it)?

I am tempted to declare that the "hints" was not a good idea and
suggest reverting them.