{"thread":{"id":"38327","subject":"[PATCH 0/2] Documentation/githooks: mention pwd, $GIT_PREFIX","startedAt":"2015-01-10T06:49:56Z","lastAt":"2015-01-12T22:38:23Z","messageCount":9,"participants":["Richard Hansen","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"254524","messageId":"1420872598-9609-1-git-send-email-rhansen@bbn.com","threadId":"38327","inReplyTo":null,"subject":"[PATCH 0/2] Documentation/githooks: mention pwd, $GIT_PREFIX","fromName":"Richard Hansen","fromEmail":"rhansen@bbn.com","sentAt":"2015-01-10T06:49:56Z","receivedAt":"2015-01-10T06:49:56Z","isPatch":true,"sender":{"key":"rhansen@rhansen.org","avatar":null},"body":"A couple of patches to document and test that hooks are run from the\ntop-level directory and that GIT_PREFIX is set to the subdirectory\nthat Git was run from (like !aliases).\n\nThe new documentation is mostly lifted from the documentation for\nalias.*.  I don't think the new documentation is perfectly clear, but\nit's a start.  In particular, what does Git do for hook pwd and\nGIT_PREFIX when it is run from within a bare repository?  Or from\nwithin .git?  Or if GIT_WORK_TREE (--work-tree) and/or GIT_DIR\n(--git-dir) are set?  Many of these same questions apply to !aliases,\nso the documentation for alias.* should also be shored up.\n\n-Richard\n\n\nRichard Hansen (2):\n  Documentation/githooks: mention pwd, $GIT_PREFIX\n  t1020-subdirectory.sh: check hook pwd, $GIT_PREFIX\n\n Documentation/githooks.txt |  6 ++++++\n t/t1020-subdirectory.sh    | 34 ++++++++++++++++++++++++++++++++++\n 2 files changed, 40 insertions(+)\n\n-- \n2.2.1\n"},{"id":"254526","messageId":"1420872598-9609-2-git-send-email-rhansen@bbn.com","threadId":"38327","inReplyTo":"1420872598-9609-1-git-send-email-rhansen@bbn.com","subject":"[PATCH 1/2] Documentation/githooks: mention pwd, $GIT_PREFIX","fromName":"Richard Hansen","fromEmail":"rhansen@bbn.com","sentAt":"2015-01-10T06:49:57Z","receivedAt":"2015-01-10T06:49:57Z","isPatch":true,"sender":{"key":"rhansen@rhansen.org","avatar":null},"body":"Document that hooks are run from the top-level directory and that\nGIT_PREFIX is set to the name of the original subdirectory (relative\nto the top-level directory).\n\nSigned-off-by: Richard Hansen <rhansen@bbn.com>\n---\n Documentation/githooks.txt | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 9ef2469..c08f4fd 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -26,6 +26,12 @@ executable by default.\n \n This document describes the currently defined hooks.\n \n+Hooks are executed from the top-level directory of a repository, which\n+may not necessarily be the current directory.\n+The 'GIT_PREFIX' environment variable is set as returned by running\n+'git rev-parse --show-prefix' from the original current directory.\n+See linkgit:git-rev-parse[1].\n+\n HOOKS\n -----\n \n-- \n2.2.1\n"},{"id":"254525","messageId":"1420872598-9609-3-git-send-email-rhansen@bbn.com","threadId":"38327","inReplyTo":"1420872598-9609-1-git-send-email-rhansen@bbn.com","subject":"[PATCH 2/2] t1020-subdirectory.sh: check hook pwd, $GIT_PREFIX","fromName":"Richard Hansen","fromEmail":"rhansen@bbn.com","sentAt":"2015-01-10T06:49:58Z","receivedAt":"2015-01-10T06:49:58Z","isPatch":true,"sender":{"key":"rhansen@rhansen.org","avatar":null},"body":"Make sure hooks are executed at the top-level directory and that\nGIT_PREFIX is set (as documented).\n\nSigned-off-by: Richard Hansen <rhansen@bbn.com>\n---\n t/t1020-subdirectory.sh | 34 ++++++++++++++++++++++++++++++++++\n 1 file changed, 34 insertions(+)\n\ndiff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh\nindex 2edb4f2..03bb0a2 100755\n--- a/t/t1020-subdirectory.sh\n+++ b/t/t1020-subdirectory.sh\n@@ -128,6 +128,23 @@ test_expect_success !MINGW '!alias expansion' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'hook pwd' '\n+\tpwd >expect &&\n+\t(\n+\t\trm -f actual &&\n+\t\tmkdir -p .git/hooks &&\n+\t\t! test -e .git/hooks/post-checkout &&\n+\t\tcat <<-\\EOF >.git/hooks/post-checkout &&\n+\t\t\t#!/bin/sh\n+\t\t\tpwd >actual\n+\t\tEOF\n+\t\tchmod +x .git/hooks/post-checkout &&\n+\t\t(cd dir && git checkout -- two) &&\n+\t\trm -f .git/hooks/post-checkout\n+\t) &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'GIT_PREFIX for !alias' '\n \tprintf \"dir/\" >expect &&\n \t(\n@@ -154,6 +171,23 @@ test_expect_success 'GIT_PREFIX for built-ins' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'GIT_PREFIX for hooks' '\n+\tprintf \"dir/\" >expect &&\n+\t(\n+\t\trm -f actual &&\n+\t\tmkdir -p .git/hooks &&\n+\t\t! test -e .git/hooks/post-checkout &&\n+\t\tcat <<-\\EOF >.git/hooks/post-checkout &&\n+\t\t\t#!/bin/sh\n+\t\t\tprintf %s \"$GIT_PREFIX\" >actual\n+\t\tEOF\n+\t\tchmod +x .git/hooks/post-checkout &&\n+\t\t(cd dir && git checkout -- two) &&\n+\t\trm -f .git/hooks/post-checkout\n+\t)  &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'no file/rev ambiguity check inside .git' '\n \tgit commit -a -m 1 &&\n \t(\n-- \n2.2.1\n"},{"id":"254527","messageId":"54B0E1EE.2020301@kdbg.org","threadId":"38327","inReplyTo":"1420872598-9609-3-git-send-email-rhansen@bbn.com","subject":"Re: [PATCH 2/2] t1020-subdirectory.sh: check hook pwd, $GIT_PREFIX","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-01-10T08:25:18Z","receivedAt":"2015-01-10T08:25:18Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10.01.2015 um 07:49 schrieb Richard Hansen:\n> Make sure hooks are executed at the top-level directory and that\n> GIT_PREFIX is set (as documented).\n> \n> Signed-off-by: Richard Hansen <rhansen@bbn.com>\n> ---\n>  t/t1020-subdirectory.sh | 34 ++++++++++++++++++++++++++++++++++\n>  1 file changed, 34 insertions(+)\n> \n> diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh\n> index 2edb4f2..03bb0a2 100755\n> --- a/t/t1020-subdirectory.sh\n> +++ b/t/t1020-subdirectory.sh\n> @@ -128,6 +128,23 @@ test_expect_success !MINGW '!alias expansion' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'hook pwd' '\n> +\tpwd >expect &&\n> +\t(\n> +\t\trm -f actual &&\n> +\t\tmkdir -p .git/hooks &&\n> +\t\t! test -e .git/hooks/post-checkout &&\n\nWhat is the purpose of this test?\n\n> +\t\tcat <<-\\EOF >.git/hooks/post-checkout &&\n> +\t\t\t#!/bin/sh\n> +\t\t\tpwd >actual\n> +\t\tEOF\n> +\t\tchmod +x .git/hooks/post-checkout &&\n\nUse write_script() to construct a shell script.\n\n> +\t\t(cd dir && git checkout -- two) &&\n> +\t\trm -f .git/hooks/post-checkout\n\nThis cleanup would be skipped if the checkout fails for some reason. Use\ntest_when_finished.\n\n> +\t) &&\n\nThe outer sub-shell us unnecessary, isn't it?\n\n> +\ttest_cmp expect actual\n\nIf 'git checkout' runs the hook from the wrong directory, there would\nnot exist a file 'actual' at this point because it was rm -f'd earlier,\nand the test would fail. Perhaps it would make sense to document this\nfailure case by inserting\n\n\ttest_path_is_file actual &&\n\nbefore the test_cmp?\n\nWhich makes me think: Would the test for existence of 'actual' be\nsufficient? Then the test_cmp could be omitted. The advantage is that we\ndo not depend on how the `pwd` is formatted: With or without symbolic\nlinks in any leading path or c:/foo vs. /c/foo on Windows. (I anticipate\nthat the test as written fails on Windows because 'expect' is in c:/foo\nform and 'actual' is in /c/foo form.)\n\n> +'\n> +\n>  test_expect_success 'GIT_PREFIX for !alias' '\n>  \tprintf \"dir/\" >expect &&\n>  \t(\n> @@ -154,6 +171,23 @@ test_expect_success 'GIT_PREFIX for built-ins' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'GIT_PREFIX for hooks' '\n> +\tprintf \"dir/\" >expect &&\n> +\t(\n> +\t\trm -f actual &&\n> +\t\tmkdir -p .git/hooks &&\n> +\t\t! test -e .git/hooks/post-checkout &&\n> +\t\tcat <<-\\EOF >.git/hooks/post-checkout &&\n> +\t\t\t#!/bin/sh\n> +\t\t\tprintf %s \"$GIT_PREFIX\" >actual\n> +\t\tEOF\n> +\t\tchmod +x .git/hooks/post-checkout &&\n> +\t\t(cd dir && git checkout -- two) &&\n> +\t\trm -f .git/hooks/post-checkout\n> +\t)  &&\n\nThe comments about the sub-shell, write_script, and clean-up apply here,\ntoo.\n\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_expect_success 'no file/rev ambiguity check inside .git' '\n>  \tgit commit -a -m 1 &&\n>  \t(\n> \n\n-- Hannes\n"},{"id":"254549","messageId":"1420931503-22857-1-git-send-email-rhansen@bbn.com","threadId":"38327","inReplyTo":"54B0E1EE.2020301@kdbg.org","subject":"[PATCH v2 0/2] Documentation/githooks: mention pwd, $GIT_PREFIX","fromName":"Richard Hansen","fromEmail":"rhansen@bbn.com","sentAt":"2015-01-10T23:11:41Z","receivedAt":"2015-01-10T23:11:41Z","isPatch":true,"sender":{"key":"rhansen@rhansen.org","avatar":null},"body":"patch 1/2 is the same as v1\npatch 2/2 has been reworked to incorporate Hannes's feedback (thank\nyou!)\n\n-Richard\n\n\nRichard Hansen (2):\n  Documentation/githooks: mention pwd, $GIT_PREFIX\n  t1020-subdirectory.sh: check hook pwd, $GIT_PREFIX\n\n Documentation/githooks.txt |  6 ++++++\n t/t1020-subdirectory.sh    | 23 +++++++++++++++++++++++\n 2 files changed, 29 insertions(+)\n\n-- \n2.2.1\n"},{"id":"254548","messageId":"1420931503-22857-2-git-send-email-rhansen@bbn.com","threadId":"38327","inReplyTo":"1420931503-22857-1-git-send-email-rhansen@bbn.com","subject":"[PATCH v2 1/2] Documentation/githooks: mention pwd, $GIT_PREFIX","fromName":"Richard Hansen","fromEmail":"rhansen@bbn.com","sentAt":"2015-01-10T23:11:42Z","receivedAt":"2015-01-10T23:11:42Z","isPatch":true,"sender":{"key":"rhansen@rhansen.org","avatar":null},"body":"Document that hooks are run from the top-level directory and that\nGIT_PREFIX is set to the name of the original subdirectory (relative\nto the top-level directory).\n\nSigned-off-by: Richard Hansen <rhansen@bbn.com>\n---\n Documentation/githooks.txt | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 9ef2469..c08f4fd 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -26,6 +26,12 @@ executable by default.\n \n This document describes the currently defined hooks.\n \n+Hooks are executed from the top-level directory of a repository, which\n+may not necessarily be the current directory.\n+The 'GIT_PREFIX' environment variable is set as returned by running\n+'git rev-parse --show-prefix' from the original current directory.\n+See linkgit:git-rev-parse[1].\n+\n HOOKS\n -----\n \n-- \n2.2.1\n"},{"id":"254547","messageId":"1420931503-22857-3-git-send-email-rhansen@bbn.com","threadId":"38327","inReplyTo":"1420931503-22857-1-git-send-email-rhansen@bbn.com","subject":"[PATCH v2 2/2] t1020-subdirectory.sh: check hook pwd, $GIT_PREFIX","fromName":"Richard Hansen","fromEmail":"rhansen@bbn.com","sentAt":"2015-01-10T23:11:43Z","receivedAt":"2015-01-10T23:11:43Z","isPatch":true,"sender":{"key":"rhansen@rhansen.org","avatar":null},"body":"Make sure hooks are executed at the top-level directory and that\nGIT_PREFIX is set (as documented).\n\nSigned-off-by: Richard Hansen <rhansen@bbn.com>\n---\n t/t1020-subdirectory.sh | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n\ndiff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh\nindex 2edb4f2..0ccbb7e 100755\n--- a/t/t1020-subdirectory.sh\n+++ b/t/t1020-subdirectory.sh\n@@ -128,6 +128,17 @@ test_expect_success !MINGW '!alias expansion' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'hook pwd' '\n+\trm -f actual &&\n+\tmkdir -p .git/hooks &&\n+\twrite_script .git/hooks/post-checkout <<-\\EOF &&\n+\t\tpwd >actual\n+\tEOF\n+\ttest_when_finished \"rm -f .git/hooks/post-checkout actual\" &&\n+\t(cd dir && git checkout -- two) &&\n+\ttest_path_is_file actual\n+'\n+\n test_expect_success 'GIT_PREFIX for !alias' '\n \tprintf \"dir/\" >expect &&\n \t(\n@@ -154,6 +165,18 @@ test_expect_success 'GIT_PREFIX for built-ins' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'GIT_PREFIX for hooks' '\n+\tprintf \"dir/\" >expect &&\n+\trm -f actual &&\n+\tmkdir -p .git/hooks &&\n+\twrite_script .git/hooks/post-checkout <<-\\EOF &&\n+\t\tprintf %s \"$GIT_PREFIX\" >actual\n+\tEOF\n+\ttest_when_finished \"rm -f .git/hooks/post-checkout expect actual\" &&\n+\t(cd dir && git checkout -- two) &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'no file/rev ambiguity check inside .git' '\n \tgit commit -a -m 1 &&\n \t(\n-- \n2.2.1\n"},{"id":"254557","messageId":"xmqqzj9n7oxp.fsf@gitster.dls.corp.google.com","threadId":"38327","inReplyTo":"1420931503-22857-2-git-send-email-rhansen@bbn.com","subject":"Re: [PATCH v2 1/2] Documentation/githooks: mention pwd, $GIT_PREFIX","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-12T19:56:50Z","receivedAt":"2015-01-12T19:56:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Richard Hansen <rhansen@bbn.com> writes:\n\n> Document that hooks are run from the top-level directory and that\n> GIT_PREFIX is set to the name of the original subdirectory (relative\n> to the top-level directory).\n>\n> Signed-off-by: Richard Hansen <rhansen@bbn.com>\n> ---\n>  Documentation/githooks.txt | 6 ++++++\n>  1 file changed, 6 insertions(+)\n>\n> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\n> index 9ef2469..c08f4fd 100644\n> --- a/Documentation/githooks.txt\n> +++ b/Documentation/githooks.txt\n> @@ -26,6 +26,12 @@ executable by default.\n>  \n>  This document describes the currently defined hooks.\n>  \n> +Hooks are executed from the top-level directory of a repository, which\n> +may not necessarily be the current directory.\n\nI agree that it is a good idea to describe how the hook writers can\ngo to the top-level directory and how the hook writers can discover\nwhere the hooked operation started, but these two lines cannot be\nthe whole story---what happens when there is no top-level directory\n(i.e. a bare repository)?\n\nIs this universal to all hooks, or just the ones you examined?  I\nask this because I know we do not go through a single interface to\ncall out to hooks that says \"cd to the root and then run the hook\ngiven as an argument\".\n\n> +The 'GIT_PREFIX' environment variable is set as returned by running\n> +'git rev-parse --show-prefix' from the original current directory.\n\nIs this also universal, or is it set only for some but not all\nhooks?  What happens in a bare repository?  What is given if you are\nin a non-bare repository and are already at the root level?\n\n> +See linkgit:git-rev-parse[1].\n> +\n>  HOOKS\n>  -----\n"},{"id":"254565","messageId":"xmqq4mrv7hgg.fsf@gitster.dls.corp.google.com","threadId":"38327","inReplyTo":"1420931503-22857-3-git-send-email-rhansen@bbn.com","subject":"Re: [PATCH v2 2/2] t1020-subdirectory.sh: check hook pwd, $GIT_PREFIX","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-12T22:38:23Z","receivedAt":"2015-01-12T22:38:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Richard Hansen <rhansen@bbn.com> writes:\n\n> Make sure hooks are executed at the top-level directory and that\n> GIT_PREFIX is set (as documented).\n\nThe same comment as the one for 1/2 applies here.  If we substitute\n'hook' everywhere with 'post-checkout hook' in this patch, it makes\nperfect sense to me, but otherwise this is far from \"check _hook_\"\nin general.\n\n> Signed-off-by: Richard Hansen <rhansen@bbn.com>\n> ---\n>  t/t1020-subdirectory.sh | 23 +++++++++++++++++++++++\n>  1 file changed, 23 insertions(+)\n>\n> diff --git a/t/t1020-subdirectory.sh b/t/t1020-subdirectory.sh\n> index 2edb4f2..0ccbb7e 100755\n> --- a/t/t1020-subdirectory.sh\n> +++ b/t/t1020-subdirectory.sh\n> @@ -128,6 +128,17 @@ test_expect_success !MINGW '!alias expansion' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'hook pwd' '\n> +\trm -f actual &&\n> +\tmkdir -p .git/hooks &&\n> +\twrite_script .git/hooks/post-checkout <<-\\EOF &&\n> +\t\tpwd >actual\n> +\tEOF\n> +\ttest_when_finished \"rm -f .git/hooks/post-checkout actual\" &&\n> +\t(cd dir && git checkout -- two) &&\n> +\ttest_path_is_file actual\n\nCute, but it is misleading to use \"pwd\" there, because the contents\nof the file does not matter for this test, even though the test is\nabout the current directory.  It forces the reader to look for the\nplace where you are comparing the contents of that file with\nexpected path to the current directory, and no such code exists.\n\n\"date >actual\", \"echo >actual\", or even just a redirection without\ncommand, i.e. \">actual\", woudl have been easier to see what is going\non (I would have used the last form if I were doing this patch).\n\n> +'\n> +\n>  test_expect_success 'GIT_PREFIX for !alias' '\n>  \tprintf \"dir/\" >expect &&\n>  \t(\n> @@ -154,6 +165,18 @@ test_expect_success 'GIT_PREFIX for built-ins' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success 'GIT_PREFIX for hooks' '\n> +\tprintf \"dir/\" >expect &&\n> +\trm -f actual &&\n> +\tmkdir -p .git/hooks &&\n> +\twrite_script .git/hooks/post-checkout <<-\\EOF &&\n> +\t\tprintf %s \"$GIT_PREFIX\" >actual\n> +\tEOF\n> +\ttest_when_finished \"rm -f .git/hooks/post-checkout expect actual\" &&\n> +\t(cd dir && git checkout -- two) &&\n> +\ttest_cmp expect actual\n> +'\n\nIt is not wrong per-se, but the same cute trick could have been\nused, i.e.\n\n\twrite_script ... post-checkout <<-\\EOF &&\n        >\"$GIT_PREFIX/actual\"\n        EOF\n        ...\n        test_path_is_file dir/actual\n\n\n\n\n> +\n>  test_expect_success 'no file/rev ambiguity check inside .git' '\n>  \tgit commit -a -m 1 &&\n>  \t(\n"}]}