Re: master d3ec68f5e43: ; Fix post-commit and pre-push hooks in worktrees again

Re: master d3ec68f5e43: ; Fix post-commit and pre-push hooks in worktrees again

[Ruijie Yu via Emacs development discussions.]

Jim Porter <jporterbugs@gmail.com> writes:

> branch: master
> commit d3ec68f5e433e5792c1c63672c7b437bb29c5759
> Author: Jim Porter <jporterbugs@gmail.com>
> Commit: Jim Porter <jporterbugs@gmail.com>
>
>     ; Fix post-commit and pre-push hooks in worktrees again
>     
>     * build-aux/git-hooks/post-commit:
>     * build-aux/git-hooks/pre-push: Use "$(dirname $0)" to get the hooks
>     directory.
> ---
>  build-aux/git-hooks/post-commit | 4 +++-
>  build-aux/git-hooks/pre-push    | 4 +++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/build-aux/git-hooks/post-commit b/build-aux/git-hooks/post-commit
> index 05f2d778b5c..10f43b539ac 100755
> --- a/build-aux/git-hooks/post-commit
> +++ b/build-aux/git-hooks/post-commit
> @@ -34,6 +34,8 @@
>  
>  ### Code:
>  
> +HOOKS_DIR=$(dirname $0)
> +

From the first look, it seems that we assume the worktree root dir name
should never contains spaces.  Is this intended?

I guess the previous versions also assumed this (according to the
${GIT_DIR:-.git} in the quoted portion below), but I just wanted to be
sure about it.

Also, while looking into the build system (because I know make is always
unhappy about spaces), I noticed these lines in Makefile (which is
auto-generated from Makefile.in):

--8<---------------cut here---------------start------------->8---
$ grep --line-number -e abs_ Makefile
192:abs_srcdir=/opt/src/emacs/base/emacs-30
266:buildlisppath=${abs_srcdir}/lisp
461:	@for dir in '$(abs_srcdir)' '$(lispdir)' '$(archlibdir)'; do \
497:	@(w32srcdir=`${srcdir}/build-aux/msys-to-w32 "${abs_srcdir}"`; \
--8<---------------cut here---------------end--------------->8---

Notice that lines 461 and 497 protects against spaces (presumably),
whereas line 266 doesn't.  If we do expect spaces in git root, line 266
might be an issue.

>  # Prefer gawk if available, as it handles NUL bytes properly.
>  if type gawk >/dev/null 2>&1; then
>    awk="gawk"
> @@ -42,4 +44,4 @@ else
>  fi
>  
>  git rev-parse HEAD | $awk -v reason=post-commit \
> -                          -f ${GIT_DIR:-.git}/hooks/commit-msg-files.awk
> +                          -f $HOOKS_DIR/commit-msg-files.awk
> diff --git a/build-aux/git-hooks/pre-push b/build-aux/git-hooks/pre-push
> index 6ff59102fd7..8d5dde2bbaf 100755
> --- a/build-aux/git-hooks/pre-push
> +++ b/build-aux/git-hooks/pre-push
> @@ -31,6 +31,8 @@
>  
>  ### Code:
>  
> +HOOKS_DIR=$(dirname $0)
> +
>  # Prefer gawk if available, as it handles NUL bytes properly.
>  if type gawk >/dev/null 2>&1; then
>    awk="gawk"
> @@ -83,4 +85,4 @@ $awk -v origin_name="$1" '
>      # Print every SHA after oldref, up to (and including) newref.
>      system("git rev-list --first-parent --reverse " oldref ".." newref)
>    }
> -' | $awk -v reason=pre-push -f ${GIT_DIR:-.git}/hooks/commit-msg-files.awk
> +' | $awk -v reason=pre-push -f $HOOKS_DIR/commit-msg-files.awk

-- 
Best,


RY

[Please note that this mail might go to spam due to some
misconfiguration in my mail server -- still investigating.]

Re: master d3ec68f5e43: ; Fix post-commit and pre-push hooks in worktrees again

[Jim Porter]

On 5/4/2023 8:28 PM, Ruijie Yu via Emacs development discussions. wrote:
> Jim Porter <jporterbugs@gmail.com> writes:
> 
>> +HOOKS_DIR=$(dirname $0)
>> +
> 
>  From the first look, it seems that we assume the worktree root dir name
> should never contains spaces.  Is this intended?

Nope, just a mistake. Fixed in eb3a90619fe. Thanks for noticing.

Re: master d3ec68f5e43: ; Fix post-commit and pre-push hooks in worktrees again

[Eli Zaretskii]

> Cc: Emacs developers <emacs-devel@gnu.org>
> Date: Fri, 05 May 2023 11:28:43 +0800
> From:  Ruijie Yu via "Emacs development discussions." <emacs-devel@gnu.org>
> 
> $ grep --line-number -e abs_ Makefile
> 192:abs_srcdir=/opt/src/emacs/base/emacs-30
> 266:buildlisppath=${abs_srcdir}/lisp
> 461:	@for dir in '$(abs_srcdir)' '$(lispdir)' '$(archlibdir)'; do \
> 497:	@(w32srcdir=`${srcdir}/build-aux/msys-to-w32 "${abs_srcdir}"`; \
> --8<---------------cut here---------------end--------------->8---
> 
> Notice that lines 461 and 497 protects against spaces (presumably),
> whereas line 266 doesn't.  If we do expect spaces in git root, line 266
> might be an issue.

Quoting of file names is needed in command lines, not in assignments
of values to Make variables.  Make doesn't have a problem with
variable values that include whitespace.  It's the shell that needs
quoting.

So line 266 cannot be the problem, but any command line that uses
$buildlisppath might be.

Re: master d3ec68f5e43: ; Fix post-commit and pre-push hooks in worktrees again

[Po Lu]

Jim Porter <jporterbugs@gmail.com> writes:

>>> +HOOKS_DIR=$(dirname $0)

Please write:

  HOOKS_DIR=`dirname $0`

as the Bourne shell does not support POSIX command substitutions.

Re: master d3ec68f5e43: ; Fix post-commit and pre-push hooks in worktrees again

[Jim Porter]

On 5/4/2023 10:23 PM, Po Lu wrote:
> Jim Porter <jporterbugs@gmail.com> writes:
> 
>>>> +HOOKS_DIR=$(dirname $0)
> 
> Please write:
> 
>    HOOKS_DIR=`dirname $0`
> 
> as the Bourne shell does not support POSIX command substitutions.

Done in f204c4a6cfa.

Re: master d3ec68f5e43: ; Fix post-commit and pre-push hooks in worktrees again

[Po Lu]

Jim Porter <jporterbugs@gmail.com> writes:

> On 5/4/2023 10:23 PM, Po Lu wrote:
>> Jim Porter <jporterbugs@gmail.com> writes:
>> 
>>>>> +HOOKS_DIR=$(dirname $0)
>> Please write:
>>    HOOKS_DIR=`dirname $0`
>> as the Bourne shell does not support POSIX command substitutions.
>
> Done in f204c4a6cfa.

Thank you.