git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH] run-command.c: add hint when hook is not executable
@ 2017-10-03 10:59 Damien
  2017-10-04  4:40 ` Junio C Hamano
  2017-10-05 20:53 ` [PATCH v2] run-command: add hint when a hook is ignored Damien Marié
  0 siblings, 2 replies; 12+ messages in thread
From: Damien @ 2017-10-03 10:59 UTC (permalink / raw)
  To: git

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] run-command.c: add hint when hook is not executable
  2017-10-03 10:59 [PATCH] run-command.c: add hint when hook is not executable Damien
@ 2017-10-04  4:40 ` Junio C Hamano
  2017-10-05 20:48   ` Damien
  2017-10-05 20:53 ` [PATCH v2] run-command: add hint when a hook is ignored Damien Marié
  1 sibling, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2017-10-04  4:40 UTC (permalink / raw)
  To: Damien; +Cc: git

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.



^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] run-command.c: add hint when hook is not executable
  2017-10-04  4:40 ` Junio C Hamano
@ 2017-10-05 20:48   ` Damien
  0 siblings, 0 replies; 12+ messages in thread
From: Damien @ 2017-10-05 20:48 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2] run-command: add hint when a hook is ignored
  2017-10-03 10:59 [PATCH] run-command.c: add hint when hook is not executable Damien
  2017-10-04  4:40 ` Junio C Hamano
@ 2017-10-05 20:53 ` Damien Marié
  2017-10-06  4:52   ` Junio C Hamano
  1 sibling, 1 reply; 12+ messages in thread
From: Damien Marié @ 2017-10-05 20:53 UTC (permalink / raw)
  To: git

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] run-command: add hint when a hook is ignored
  2017-10-05 20:53 ` [PATCH v2] run-command: add hint when a hook is ignored Damien Marié
@ 2017-10-06  4:52   ` Junio C Hamano
  2017-10-06  5:25     ` Junio C Hamano
                       ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Junio C Hamano @ 2017-10-06  4:52 UTC (permalink / raw)
  To: Damien Marié; +Cc: git

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] run-command: add hint when a hook is ignored
  2017-10-06  4:52   ` Junio C Hamano
@ 2017-10-06  5:25     ` Junio C Hamano
  2017-10-06  5:43     ` Junio C Hamano
  2017-10-06  5:53     ` Junio C Hamano
  2 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2017-10-06  5:25 UTC (permalink / raw)
  To: Damien Marié; +Cc: git

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] run-command: add hint when a hook is ignored
  2017-10-06  4:52   ` Junio C Hamano
  2017-10-06  5:25     ` Junio C Hamano
@ 2017-10-06  5:43     ` Junio C Hamano
  2017-10-06  5:53     ` Junio C Hamano
  2 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2017-10-06  5:43 UTC (permalink / raw)
  To: Damien Marié; +Cc: git

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] run-command: add hint when a hook is ignored
  2017-10-06  4:52   ` Junio C Hamano
  2017-10-06  5:25     ` Junio C Hamano
  2017-10-06  5:43     ` Junio C Hamano
@ 2017-10-06  5:53     ` Junio C Hamano
  2017-10-06  8:04       ` Damien
  2017-10-06  8:07       ` [PATCH v3] " Damien Marié
  2 siblings, 2 replies; 12+ messages in thread
From: Junio C Hamano @ 2017-10-06  5:53 UTC (permalink / raw)
  To: Damien Marié; +Cc: git

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


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] run-command: add hint when a hook is ignored
  2017-10-06  5:53     ` Junio C Hamano
@ 2017-10-06  8:04       ` Damien
  2017-10-06  8:07       ` [PATCH v3] " Damien Marié
  1 sibling, 0 replies; 12+ messages in thread
From: Damien @ 2017-10-06  8:04 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

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
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v3] run-command: add hint when a hook is ignored
  2017-10-06  5:53     ` Junio C Hamano
  2017-10-06  8:04       ` Damien
@ 2017-10-06  8:07       ` " Damien Marié
  2017-10-10  4:21         ` Junio C Hamano
  1 sibling, 1 reply; 12+ messages in thread
From: Damien Marié @ 2017-10-06  8:07 UTC (permalink / raw)
  To: git

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] run-command: add hint when a hook is ignored
  2017-10-06  8:07       ` [PATCH v3] " Damien Marié
@ 2017-10-10  4:21         ` Junio C Hamano
  2017-10-11  6:26           ` Junio C Hamano
  0 siblings, 1 reply; 12+ messages in thread
From: Junio C Hamano @ 2017-10-10  4:21 UTC (permalink / raw)
  To: git; +Cc: Damien Marié

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/& /;


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v3] run-command: add hint when a hook is ignored
  2017-10-10  4:21         ` Junio C Hamano
@ 2017-10-11  6:26           ` Junio C Hamano
  0 siblings, 0 replies; 12+ messages in thread
From: Junio C Hamano @ 2017-10-11  6:26 UTC (permalink / raw)
  To: git; +Cc: Damien Marié

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, back to index

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-03 10:59 [PATCH] run-command.c: add hint when hook is not executable Damien
2017-10-04  4:40 ` Junio C Hamano
2017-10-05 20:48   ` Damien
2017-10-05 20:53 ` [PATCH v2] run-command: add hint when a hook is ignored Damien Marié
2017-10-06  4:52   ` Junio C Hamano
2017-10-06  5:25     ` Junio C Hamano
2017-10-06  5:43     ` Junio C Hamano
2017-10-06  5:53     ` Junio C Hamano
2017-10-06  8:04       ` Damien
2017-10-06  8:07       ` [PATCH v3] " Damien Marié
2017-10-10  4:21         ` Junio C Hamano
2017-10-11  6:26           ` Junio C Hamano

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox