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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 4, 2017, 04:40 UTC
Message-ID: <xmqqo9pnedar.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <0102015ee1e41f17-927a8da1-ac14-4399-8424-fee8a82c2b0a-000000@eu-west-1.amazonses.com>

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

Show 18 quoted lines

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

Show 13 quoted lines

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