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  wrote:
> Junio C Hamano  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 
>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(, "hooks/%s", name);
> if (access(path.buf, X_OK) < 0) {
> int err = errno;
> +
>  #ifdef STRIP_EXTENSION
> strbuf_addstr(, 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(_given, name)) {
> +   string_list_insert(_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

[Junio C Hamano]

Junio C Hamano  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 
   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(, "hooks/%s", name);
if (access(path.buf, X_OK) < 0) {
int err = errno;
+
 #ifdef STRIP_EXTENSION
strbuf_addstr(, 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(_given, name)) {
+   string_list_insert(_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

[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 00..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  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]

Damien Marié  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(, 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 0..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" < +#!/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.