Re: [PATCH v2 3/4] hook: add list command

From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Emily Shaffer <emilyshaffer@google.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v2 3/4] hook: add list command
Date: Mon, 25 May 2020 01:00:03 +0200 (CEST)	[thread overview]
Message-ID: <nycvar.QRO.7.76.6.2005250055180.56@tvgsbejvaqbjf.bet> (raw)
In-Reply-To: <20200521185414.43760-4-emilyshaffer@google.com>

Hi Emily,

On Thu, 21 May 2020, Emily Shaffer wrote:

> diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh
> index 34b0df5216..4e46d7dd4e 100755
> --- a/t/t1360-config-based-hooks.sh
> +++ b/t/t1360-config-based-hooks.sh
> @@ -4,8 +4,55 @@ test_description='config-managed multihooks, including git-hook command'
>
>  . ./test-lib.sh
>
> -test_expect_success 'git hook command does not crash' '
> -	git hook
> +test_expect_success 'git hook rejects commands without a mode' '
> +	test_must_fail git hook pre-commit
> +'
> +
> +
> +test_expect_success 'git hook rejects commands without a hookname' '
> +	test_must_fail git hook list
> +'
> +
> +test_expect_success 'setup hooks in global, and local' '
> +	git config --add --local hook.pre-commit.command "/path/ghi" &&
> +	git config --add --global hook.pre-commit.command "/path/def"
> +'
> +
> +test_expect_success 'git hook list orders by config order' '
> +	cat >expected <<-\EOF &&
> +	global:	/path/def
> +	local:	/path/ghi
> +	EOF
> +
> +	git hook list pre-commit >actual &&
> +	test_cmp expected actual

This, as well as the next two test cases, won't work on Windows, as you
almost certainly realized from looking at the failed GitHub workflow run
of your branch.

The reason is that Unix-like absolute paths like `/path/def` do _not_ do
what you think on Windows: they are relative to the MSYS2 root (because
the shell script runs in an MSYS2 Bash). The Git executable, however, has
not the slightest idea about MSYS2 and does not handle those. To remedy
that, the MSYS2 Bash prefixes those paths with the absolute
_Windows-style_ path when passing them to `git.exe` (in your case,
actually in the `setup hooks` test case above).

So you will need to squash this (or an equivalent fix) into your patch:

-- snip --
From f2568d47509130a9c35590d907797d2eb813ac0d Mon Sep 17 00:00:00 2001
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mon, 25 May 2020 15:03:16 +0200
Subject: [PATCH] fixup??? hook: add list command

This is needed to make the tests pass on Windows, where Unix-like
absolute paths are not what you think they are.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t1360-config-based-hooks.sh | 39 +++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/t/t1360-config-based-hooks.sh b/t/t1360-config-based-hooks.sh
index 3296d8af4587..c862655fd4d9 100755
--- a/t/t1360-config-based-hooks.sh
+++ b/t/t1360-config-based-hooks.sh
@@ -18,10 +18,19 @@ test_expect_success 'setup hooks in global, and local' '
 	git config --add --global hook.pre-commit.command "/path/def"
 '

+ROOT=
+if test_have_prereq MINGW
+then
+	# In Git for Windows, Unix-like paths work only in shell scripts;
+	# `git.exe`, however, will prefix them with the pseudo root directory
+	# (of the Unix shell). Let's accommodate for that.
+	ROOT="$(cd / && pwd)"
+fi
+
 test_expect_success 'git hook list orders by config order' '
-	cat >expected <<-\EOF &&
-	global:	/path/def
-	local:	/path/ghi
+	cat >expected <<-EOF &&
+	global:	$ROOT/path/def
+	local:	$ROOT/path/ghi
 	EOF

 	git hook list pre-commit >actual &&
@@ -32,10 +41,10 @@ test_expect_success 'git hook list dereferences a hookcmd' '
 	git config --add --local hook.pre-commit.command "abc" &&
 	git config --add --global hookcmd.abc.command "/path/abc" &&

-	cat >expected <<-\EOF &&
-	global:	/path/def
-	local:	/path/ghi
-	local:	/path/abc
+	cat >expected <<-EOF &&
+	global:	$ROOT/path/def
+	local:	$ROOT/path/ghi
+	local:	$ROOT/path/abc
 	EOF

 	git hook list pre-commit >actual &&
@@ -45,10 +54,10 @@ test_expect_success 'git hook list dereferences a hookcmd' '
 test_expect_success 'git hook list reorders on duplicate commands' '
 	git config --add --local hook.pre-commit.command "/path/def" &&

-	cat >expected <<-\EOF &&
-	local:	/path/ghi
-	local:	/path/abc
-	local:	/path/def
+	cat >expected <<-EOF &&
+	local:	$ROOT/path/ghi
+	local:	$ROOT/path/abc
+	local:	$ROOT/path/def
 	EOF

 	git hook list pre-commit >actual &&
@@ -56,10 +65,10 @@ test_expect_success 'git hook list reorders on duplicate commands' '
 '

 test_expect_success 'git hook list --porcelain prints just the command' '
-	cat >expected <<-\EOF &&
-	/path/ghi
-	/path/abc
-	/path/def
+	cat >expected <<-EOF &&
+	$ROOT/path/ghi
+	$ROOT/path/abc
+	$ROOT/path/def
 	EOF

 	git hook list --porcelain pre-commit >actual &&
--
2.27.0.rc1.windows.1

-- snap --

Ciao,
Dscho

> +'
> +
> +test_expect_success 'git hook list dereferences a hookcmd' '
> +	git config --add --local hook.pre-commit.command "abc" &&
> +	git config --add --global hookcmd.abc.command "/path/abc" &&
> +
> +	cat >expected <<-\EOF &&
> +	global:	/path/def
> +	local:	/path/ghi
> +	local:	/path/abc
> +	EOF
> +
> +	git hook list pre-commit >actual &&
> +	test_cmp expected actual
> +'
> +
> +test_expect_success 'git hook list reorders on duplicate commands' '
> +	git config --add --local hook.pre-commit.command "/path/def" &&
> +
> +	cat >expected <<-\EOF &&
> +	local:	/path/ghi
> +	local:	/path/abc
> +	local:	/path/def
> +	EOF
> +
> +	git hook list pre-commit >actual &&
> +	test_cmp expected actual
>  '
>
>  test_done
> --
> 2.27.0.rc0.183.gde8f92d652-goog
>
>
>
