[PATCH 0/2] Documentation/githooks: mention pwd, $GIT_PREFIX

[PATCH 0/2] Documentation/githooks: mention pwd, $GIT_PREFIX

[Richard Hansen]

A couple of patches to document and test that hooks are run from the
top-level directory and that GIT_PREFIX is set to the subdirectory
that Git was run from (like !aliases).

The new documentation is mostly lifted from the documentation for
alias.*.  I don't think the new documentation is perfectly clear, but
it's a start.  In particular, what does Git do for hook pwd and
GIT_PREFIX when it is run from within a bare repository?  Or from
within .git?  Or if GIT_WORK_TREE (--work-tree) and/or GIT_DIR
(--git-dir) are set?  Many of these same questions apply to !aliases,
so the documentation for alias.* should also be shored up.

-Richard


Richard Hansen (2):
  Documentation/githooks: mention pwd, $GIT_PREFIX
  t1020-subdirectory.sh: check hook pwd, $GIT_PREFIX

 Documentation/githooks.txt |  6 ++++++
 t/t1020-subdirectory.sh    | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

-- 
2.2.1

[PATCH 1/2] Documentation/githooks: mention pwd, $GIT_PREFIX

[Richard Hansen]

Document that hooks are run from the top-level directory and that
GIT_PREFIX is set to the name of the original subdirectory (relative
to the top-level directory).

Signed-off-by: Richard Hansen <rhansen@bbn.com>
---
 Documentation/githooks.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 9ef2469..c08f4fd 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -26,6 +26,12 @@ executable by default.
 
 This document describes the currently defined hooks.
 
+Hooks are executed from the top-level directory of a repository, which
+may not necessarily be the current directory.
+The 'GIT_PREFIX' environment variable is set as returned by running
+'git rev-parse --show-prefix' from the original current directory.
+See linkgit:git-rev-parse[1].
+
 HOOKS
 -----
 
-- 
2.2.1

[PATCH 2/2] t1020-subdirectory.sh: check hook pwd, $GIT_PREFIX

[Richard Hansen]

Make sure hooks are executed at the top-level directory and that
GIT_PREFIX is set (as documented).

Signed-off-by: Richard Hansen <rhansen@bbn.com>
---
 t/t1020-subdirectory.sh | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh
index 2edb4f2..03bb0a2 100755
--- a/t/t1020-subdirectory.sh
+++ b/t/t1020-subdirectory.sh
@@ -128,6 +128,23 @@ test_expect_success !MINGW '!alias expansion' '
 	test_cmp expect actual
 '
 
+test_expect_success 'hook pwd' '
+	pwd >expect &&
+	(
+		rm -f actual &&
+		mkdir -p .git/hooks &&
+		! test -e .git/hooks/post-checkout &&
+		cat <<-\EOF >.git/hooks/post-checkout &&
+			#!/bin/sh
+			pwd >actual
+		EOF
+		chmod +x .git/hooks/post-checkout &&
+		(cd dir && git checkout -- two) &&
+		rm -f .git/hooks/post-checkout
+	) &&
+	test_cmp expect actual
+'
+
 test_expect_success 'GIT_PREFIX for !alias' '
 	printf "dir/" >expect &&
 	(
@@ -154,6 +171,23 @@ test_expect_success 'GIT_PREFIX for built-ins' '
 	test_cmp expect actual
 '
 
+test_expect_success 'GIT_PREFIX for hooks' '
+	printf "dir/" >expect &&
+	(
+		rm -f actual &&
+		mkdir -p .git/hooks &&
+		! test -e .git/hooks/post-checkout &&
+		cat <<-\EOF >.git/hooks/post-checkout &&
+			#!/bin/sh
+			printf %s "$GIT_PREFIX" >actual
+		EOF
+		chmod +x .git/hooks/post-checkout &&
+		(cd dir && git checkout -- two) &&
+		rm -f .git/hooks/post-checkout
+	)  &&
+	test_cmp expect actual
+'
+
 test_expect_success 'no file/rev ambiguity check inside .git' '
 	git commit -a -m 1 &&
 	(
-- 
2.2.1

Re: [PATCH 2/2] t1020-subdirectory.sh: check hook pwd, $GIT_PREFIX

[Johannes Sixt]

Am 10.01.2015 um 07:49 schrieb Richard Hansen:
> Make sure hooks are executed at the top-level directory and that
> GIT_PREFIX is set (as documented).
> 
> Signed-off-by: Richard Hansen <rhansen@bbn.com>
> ---
>  t/t1020-subdirectory.sh | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh
> index 2edb4f2..03bb0a2 100755
> --- a/t/t1020-subdirectory.sh
> +++ b/t/t1020-subdirectory.sh
> @@ -128,6 +128,23 @@ test_expect_success !MINGW '!alias expansion' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'hook pwd' '
> +	pwd >expect &&
> +	(
> +		rm -f actual &&
> +		mkdir -p .git/hooks &&
> +		! test -e .git/hooks/post-checkout &&

What is the purpose of this test?

> +		cat <<-\EOF >.git/hooks/post-checkout &&
> +			#!/bin/sh
> +			pwd >actual
> +		EOF
> +		chmod +x .git/hooks/post-checkout &&

Use write_script() to construct a shell script.

> +		(cd dir && git checkout -- two) &&
> +		rm -f .git/hooks/post-checkout

This cleanup would be skipped if the checkout fails for some reason. Use
test_when_finished.

> +	) &&

The outer sub-shell us unnecessary, isn't it?

> +	test_cmp expect actual

If 'git checkout' runs the hook from the wrong directory, there would
not exist a file 'actual' at this point because it was rm -f'd earlier,
and the test would fail. Perhaps it would make sense to document this
failure case by inserting

	test_path_is_file actual &&

before the test_cmp?

Which makes me think: Would the test for existence of 'actual' be
sufficient? Then the test_cmp could be omitted. The advantage is that we
do not depend on how the `pwd` is formatted: With or without symbolic
links in any leading path or c:/foo vs. /c/foo on Windows. (I anticipate
that the test as written fails on Windows because 'expect' is in c:/foo
form and 'actual' is in /c/foo form.)

> +'
> +
>  test_expect_success 'GIT_PREFIX for !alias' '
>  	printf "dir/" >expect &&
>  	(
> @@ -154,6 +171,23 @@ test_expect_success 'GIT_PREFIX for built-ins' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'GIT_PREFIX for hooks' '
> +	printf "dir/" >expect &&
> +	(
> +		rm -f actual &&
> +		mkdir -p .git/hooks &&
> +		! test -e .git/hooks/post-checkout &&
> +		cat <<-\EOF >.git/hooks/post-checkout &&
> +			#!/bin/sh
> +			printf %s "$GIT_PREFIX" >actual
> +		EOF
> +		chmod +x .git/hooks/post-checkout &&
> +		(cd dir && git checkout -- two) &&
> +		rm -f .git/hooks/post-checkout
> +	)  &&

The comments about the sub-shell, write_script, and clean-up apply here,
too.

> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'no file/rev ambiguity check inside .git' '
>  	git commit -a -m 1 &&
>  	(
> 

-- Hannes

[PATCH v2 0/2] Documentation/githooks: mention pwd, $GIT_PREFIX

[Richard Hansen]

patch 1/2 is the same as v1
patch 2/2 has been reworked to incorporate Hannes's feedback (thank
you!)

-Richard


Richard Hansen (2):
  Documentation/githooks: mention pwd, $GIT_PREFIX
  t1020-subdirectory.sh: check hook pwd, $GIT_PREFIX

 Documentation/githooks.txt |  6 ++++++
 t/t1020-subdirectory.sh    | 23 +++++++++++++++++++++++
 2 files changed, 29 insertions(+)

-- 
2.2.1

[PATCH v2 1/2] Documentation/githooks: mention pwd, $GIT_PREFIX

[Richard Hansen]

Document that hooks are run from the top-level directory and that
GIT_PREFIX is set to the name of the original subdirectory (relative
to the top-level directory).

Signed-off-by: Richard Hansen <rhansen@bbn.com>
---
 Documentation/githooks.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 9ef2469..c08f4fd 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -26,6 +26,12 @@ executable by default.
 
 This document describes the currently defined hooks.
 
+Hooks are executed from the top-level directory of a repository, which
+may not necessarily be the current directory.
+The 'GIT_PREFIX' environment variable is set as returned by running
+'git rev-parse --show-prefix' from the original current directory.
+See linkgit:git-rev-parse[1].
+
 HOOKS
 -----
 
-- 
2.2.1

Re: [PATCH v2 1/2] Documentation/githooks: mention pwd, $GIT_PREFIX

[Junio C Hamano]

Richard Hansen <rhansen@bbn.com> writes:

> Document that hooks are run from the top-level directory and that
> GIT_PREFIX is set to the name of the original subdirectory (relative
> to the top-level directory).
>
> Signed-off-by: Richard Hansen <rhansen@bbn.com>
> ---
>  Documentation/githooks.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index 9ef2469..c08f4fd 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -26,6 +26,12 @@ executable by default.
>  
>  This document describes the currently defined hooks.
>  
> +Hooks are executed from the top-level directory of a repository, which
> +may not necessarily be the current directory.

I agree that it is a good idea to describe how the hook writers can
go to the top-level directory and how the hook writers can discover
where the hooked operation started, but these two lines cannot be
the whole story---what happens when there is no top-level directory
(i.e. a bare repository)?

Is this universal to all hooks, or just the ones you examined?  I
ask this because I know we do not go through a single interface to
call out to hooks that says "cd to the root and then run the hook
given as an argument".

> +The 'GIT_PREFIX' environment variable is set as returned by running
> +'git rev-parse --show-prefix' from the original current directory.

Is this also universal, or is it set only for some but not all
hooks?  What happens in a bare repository?  What is given if you are
in a non-bare repository and are already at the root level?

> +See linkgit:git-rev-parse[1].
> +
>  HOOKS
>  -----

[PATCH v2 2/2] t1020-subdirectory.sh: check hook pwd, $GIT_PREFIX

[Richard Hansen]

Make sure hooks are executed at the top-level directory and that
GIT_PREFIX is set (as documented).

Signed-off-by: Richard Hansen <rhansen@bbn.com>
---
 t/t1020-subdirectory.sh | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh
index 2edb4f2..0ccbb7e 100755
--- a/t/t1020-subdirectory.sh
+++ b/t/t1020-subdirectory.sh
@@ -128,6 +128,17 @@ test_expect_success !MINGW '!alias expansion' '
 	test_cmp expect actual
 '
 
+test_expect_success 'hook pwd' '
+	rm -f actual &&
+	mkdir -p .git/hooks &&
+	write_script .git/hooks/post-checkout <<-\EOF &&
+		pwd >actual
+	EOF
+	test_when_finished "rm -f .git/hooks/post-checkout actual" &&
+	(cd dir && git checkout -- two) &&
+	test_path_is_file actual
+'
+
 test_expect_success 'GIT_PREFIX for !alias' '
 	printf "dir/" >expect &&
 	(
@@ -154,6 +165,18 @@ test_expect_success 'GIT_PREFIX for built-ins' '
 	test_cmp expect actual
 '
 
+test_expect_success 'GIT_PREFIX for hooks' '
+	printf "dir/" >expect &&
+	rm -f actual &&
+	mkdir -p .git/hooks &&
+	write_script .git/hooks/post-checkout <<-\EOF &&
+		printf %s "$GIT_PREFIX" >actual
+	EOF
+	test_when_finished "rm -f .git/hooks/post-checkout expect actual" &&
+	(cd dir && git checkout -- two) &&
+	test_cmp expect actual
+'
+
 test_expect_success 'no file/rev ambiguity check inside .git' '
 	git commit -a -m 1 &&
 	(
-- 
2.2.1

Re: [PATCH v2 2/2] t1020-subdirectory.sh: check hook pwd, $GIT_PREFIX

[Junio C Hamano]

Richard Hansen <rhansen@bbn.com> writes:

> Make sure hooks are executed at the top-level directory and that
> GIT_PREFIX is set (as documented).

The same comment as the one for 1/2 applies here.  If we substitute
'hook' everywhere with 'post-checkout hook' in this patch, it makes
perfect sense to me, but otherwise this is far from "check _hook_"
in general.

> Signed-off-by: Richard Hansen <rhansen@bbn.com>
> ---
>  t/t1020-subdirectory.sh | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh
> index 2edb4f2..0ccbb7e 100755
> --- a/t/t1020-subdirectory.sh
> +++ b/t/t1020-subdirectory.sh
> @@ -128,6 +128,17 @@ test_expect_success !MINGW '!alias expansion' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'hook pwd' '
> +	rm -f actual &&
> +	mkdir -p .git/hooks &&
> +	write_script .git/hooks/post-checkout <<-\EOF &&
> +		pwd >actual
> +	EOF
> +	test_when_finished "rm -f .git/hooks/post-checkout actual" &&
> +	(cd dir && git checkout -- two) &&
> +	test_path_is_file actual

Cute, but it is misleading to use "pwd" there, because the contents
of the file does not matter for this test, even though the test is
about the current directory.  It forces the reader to look for the
place where you are comparing the contents of that file with
expected path to the current directory, and no such code exists.

"date >actual", "echo >actual", or even just a redirection without
command, i.e. ">actual", woudl have been easier to see what is going
on (I would have used the last form if I were doing this patch).

> +'
> +
>  test_expect_success 'GIT_PREFIX for !alias' '
>  	printf "dir/" >expect &&
>  	(
> @@ -154,6 +165,18 @@ test_expect_success 'GIT_PREFIX for built-ins' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'GIT_PREFIX for hooks' '
> +	printf "dir/" >expect &&
> +	rm -f actual &&
> +	mkdir -p .git/hooks &&
> +	write_script .git/hooks/post-checkout <<-\EOF &&
> +		printf %s "$GIT_PREFIX" >actual
> +	EOF
> +	test_when_finished "rm -f .git/hooks/post-checkout expect actual" &&
> +	(cd dir && git checkout -- two) &&
> +	test_cmp expect actual
> +'

It is not wrong per-se, but the same cute trick could have been
used, i.e.

	write_script ... post-checkout <<-\EOF &&
        >"$GIT_PREFIX/actual"
        EOF
        ...
        test_path_is_file dir/actual




> +
>  test_expect_success 'no file/rev ambiguity check inside .git' '
>  	git commit -a -m 1 &&
>  	(