git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Damien <damien@dam.io>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] run-command.c: add hint when hook is not executable
Date: Wed, 04 Oct 2017 13:40:12 +0900
Message-ID: <xmqqo9pnedar.fsf@gitster.mtv.corp.google.com> (raw)
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).

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



  reply index

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-10-03 10:59 Damien
2017-10-04  4:40 ` Junio C Hamano [this message]
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
2018-01-03  8:31             ` Jeff King
2018-01-05 19:36               ` Junio C Hamano
2018-03-03  7:16                 ` Jeff King
2018-03-05 21:19                   ` Junio C Hamano

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqo9pnedar.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=damien@dam.io \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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