{"thread":{"id":"50102","subject":"[PATCH] Rebase: Run post-checkout hook on checkout","startedAt":"2018-12-24T21:24:37Z","lastAt":"2018-12-29T21:36:53Z","messageCount":8,"participants":["orgads@gmail.com","Junio C Hamano","Ramsay Jones","Orgad Shaneh"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"365789","messageId":"20181224212425.16596-1-orgads@gmail.com","threadId":"50102","inReplyTo":null,"subject":"[PATCH] Rebase: Run post-checkout hook on checkout","fromName":"","fromEmail":"orgads@gmail.com","sentAt":"2018-12-24T21:24:23Z","receivedAt":"2018-12-24T21:24:37Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"From: Orgad Shaneh <orgads@gmail.com>\n\nSigned-off-by: Orgad Shaneh <orgads@gmail.com>\n---\n builtin/rebase.c              | 11 +++++++++--\n t/t5403-post-checkout-hook.sh | 20 ++++++++++++++++++++\n 2 files changed, 29 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex b5c99ec10c..7f7a2c738e 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -530,6 +530,7 @@ static int run_specific_rebase(struct rebase_options *opts)\n \n #define RESET_HEAD_DETACH (1<<0)\n #define RESET_HEAD_HARD (1<<1)\n+#define RESET_HEAD_RUN_HOOK (1<<2)\n \n static int reset_head(struct object_id *oid, const char *action,\n \t\t      const char *switch_to_branch, unsigned flags,\n@@ -537,6 +538,7 @@ static int reset_head(struct object_id *oid, const char *action,\n {\n \tunsigned detach_head = flags & RESET_HEAD_DETACH;\n \tunsigned reset_hard = flags & RESET_HEAD_HARD;\n+\tunsigned run_hook = flags & RESET_HEAD_RUN_HOOK;\n \tstruct object_id head_oid;\n \tstruct tree_desc desc[2] = { { NULL }, { NULL } };\n \tstruct lock_file lock = LOCK_INIT;\n@@ -636,6 +638,10 @@ static int reset_head(struct object_id *oid, const char *action,\n \t\t\tret = update_ref(reflog_head, \"HEAD\", oid, NULL, 0,\n \t\t\t\t\t UPDATE_REFS_MSG_ON_ERR);\n \t}\n+\tif (run_hook)\n+\t\trun_hook_le(NULL, \"post-checkout\",\n+\t\t\t    oid_to_hex(orig ? orig : &null_oid),\n+\t\t\t    oid_to_hex(oid), \"1\", NULL);\n \n leave_reset_head:\n \tstrbuf_release(&msg);\n@@ -1465,7 +1471,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),\n \t\t\t\t\t    options.switch_to);\n \t\t\t\tif (reset_head(&oid, \"checkout\",\n-\t\t\t\t\t       options.head_name, 0,\n+\t\t\t\t\t       options.head_name,\n+\t\t\t\t\t       RESET_HEAD_RUN_HOOK,\n \t\t\t\t\t       NULL, buf.buf) < 0) {\n \t\t\t\t\tret = !!error(_(\"could not switch to \"\n \t\t\t\t\t\t\t\"%s\"),\n@@ -1539,7 +1546,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tstrbuf_addf(&msg, \"%s: checkout %s\",\n \t\t    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);\n \tif (reset_head(&options.onto->object.oid, \"checkout\", NULL,\n-\t\t       RESET_HEAD_DETACH, NULL, msg.buf))\n+\t\t       RESET_HEAD_DETACH | RESET_HEAD_RUN_HOOK, NULL, msg.buf))\n \t\tdie(_(\"Could not detach HEAD\"));\n \tstrbuf_release(&msg);\n \ndiff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh\nindex 9f9a5163c5..5b4e582caa 100755\n--- a/t/t5403-post-checkout-hook.sh\n+++ b/t/t5403-post-checkout-hook.sh\n@@ -13,6 +13,8 @@ test_expect_success setup '\n \tEOF\n \ttest_commit one &&\n \ttest_commit two &&\n+\ttest_commit rebase-on-me &&\n+\tgit reset --hard HEAD^ &&\n \ttest_commit three three\n '\n \n@@ -51,6 +53,24 @@ test_expect_success 'post-checkout receives the right args when not switching br\n \trm -f .git/post-checkout.args\n '\n \n+test_expect_success 'post-checkout is triggered on rebase' '\n+\tgit checkout -b rebase-test master &&\n+\trm -f .git/post-checkout.args &&\n+\tgit rebase rebase-on-me &&\n+\tread old new flag < .git/post-checkout.args &&\n+\ttest $old != $new && test $flag = 1 &&\n+\trm -f .git/post-checkout.args\n+'\n+\n+test_expect_success 'post-checkout is triggered on rebase with fast-forward' '\n+\tgit checkout -b ff-rebase-test rebase-on-me^ &&\n+\trm -f .git/post-checkout.args &&\n+\tgit rebase rebase-on-me &&\n+\tread old new flag < .git/post-checkout.args &&\n+\ttest $old != $new && test $flag = 1 &&\n+\trm -f .git/post-checkout.args\n+'\n+\n if test \"$(git config --bool core.filemode)\" = true; then\n mkdir -p templates/hooks\n write_script templates/hooks/post-checkout <<-\\EOF\n-- \n2.20.1\n\n"},{"id":"365790","messageId":"20181224212425.16596-2-orgads@gmail.com","threadId":"50102","inReplyTo":"20181224212425.16596-1-orgads@gmail.com","subject":"[PATCH 1/2] t5403: Refactor","fromName":"","fromEmail":"orgads@gmail.com","sentAt":"2018-12-24T21:24:24Z","receivedAt":"2018-12-24T21:24:41Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"From: Orgad Shaneh <orgads@gmail.com>\n\n* Replace multiple clones and commits by test_commits.\n* Replace 3 invocations of awk by read.\n\nSigned-off-by: Orgad Shaneh <orgads@gmail.com>\n---\n t/t5403-post-checkout-hook.sh | 80 +++++++++++++----------------------\n 1 file changed, 29 insertions(+), 51 deletions(-)\n\ndiff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh\nindex fc898c9eac..9f9a5163c5 100755\n--- a/t/t5403-post-checkout-hook.sh\n+++ b/t/t5403-post-checkout-hook.sh\n@@ -7,77 +7,55 @@ test_description='Test the post-checkout hook.'\n . ./test-lib.sh\n \n test_expect_success setup '\n-\techo Data for commit0. >a &&\n-\techo Data for commit0. >b &&\n-\tgit update-index --add a &&\n-\tgit update-index --add b &&\n-\ttree0=$(git write-tree) &&\n-\tcommit0=$(echo setup | git commit-tree $tree0) &&\n-\tgit update-ref refs/heads/master $commit0 &&\n-\tgit clone ./. clone1 &&\n-\tgit clone ./. clone2 &&\n-\tGIT_DIR=clone2/.git git branch new2 &&\n-\techo Data for commit1. >clone2/b &&\n-\tGIT_DIR=clone2/.git git add clone2/b &&\n-\tGIT_DIR=clone2/.git git commit -m new2\n-'\n-\n-for clone in 1 2; do\n-    cat >clone${clone}/.git/hooks/post-checkout <<'EOF'\n-#!/bin/sh\n-echo $@ > $GIT_DIR/post-checkout.args\n-EOF\n-    chmod u+x clone${clone}/.git/hooks/post-checkout\n-done\n-\n-test_expect_success 'post-checkout runs as expected ' '\n-\tGIT_DIR=clone1/.git git checkout master &&\n-\ttest -e clone1/.git/post-checkout.args\n+\tmv .git/hooks-disabled .git/hooks &&\n+\twrite_script .git/hooks/post-checkout <<-\\EOF &&\n+\techo $@ >.git/post-checkout.args\n+\tEOF\n+\ttest_commit one &&\n+\ttest_commit two &&\n+\ttest_commit three three\n '\n \n test_expect_success 'post-checkout receives the right arguments with HEAD unchanged ' '\n-\told=$(awk \"{print \\$1}\" clone1/.git/post-checkout.args) &&\n-\tnew=$(awk \"{print \\$2}\" clone1/.git/post-checkout.args) &&\n-\tflag=$(awk \"{print \\$3}\" clone1/.git/post-checkout.args) &&\n-\ttest $old = $new && test $flag = 1\n+\tgit checkout master &&\n+\ttest -e .git/post-checkout.args &&\n+\tread old new flag <.git/post-checkout.args &&\n+\ttest $old = $new && test $flag = 1 &&\n+\trm -f .git/post-checkout.args\n '\n \n test_expect_success 'post-checkout runs as expected ' '\n-\tGIT_DIR=clone1/.git git checkout master &&\n-\ttest -e clone1/.git/post-checkout.args\n+\tgit checkout master &&\n+\ttest -e .git/post-checkout.args &&\n+\trm -f .git/post-checkout.args\n '\n \n test_expect_success 'post-checkout args are correct with git checkout -b ' '\n-\tGIT_DIR=clone1/.git git checkout -b new1 &&\n-\told=$(awk \"{print \\$1}\" clone1/.git/post-checkout.args) &&\n-\tnew=$(awk \"{print \\$2}\" clone1/.git/post-checkout.args) &&\n-\tflag=$(awk \"{print \\$3}\" clone1/.git/post-checkout.args) &&\n-\ttest $old = $new && test $flag = 1\n+\tgit checkout -b new1 &&\n+\tread old new flag <.git/post-checkout.args &&\n+\ttest $old = $new && test $flag = 1 &&\n+\trm -f .git/post-checkout.args\n '\n \n test_expect_success 'post-checkout receives the right args with HEAD changed ' '\n-\tGIT_DIR=clone2/.git git checkout new2 &&\n-\told=$(awk \"{print \\$1}\" clone2/.git/post-checkout.args) &&\n-\tnew=$(awk \"{print \\$2}\" clone2/.git/post-checkout.args) &&\n-\tflag=$(awk \"{print \\$3}\" clone2/.git/post-checkout.args) &&\n-\ttest $old != $new && test $flag = 1\n+\tgit checkout two &&\n+\tread old new flag <.git/post-checkout.args &&\n+\ttest $old != $new && test $flag = 1 &&\n+\trm -f .git/post-checkout.args\n '\n \n test_expect_success 'post-checkout receives the right args when not switching branches ' '\n-\tGIT_DIR=clone2/.git git checkout master b &&\n-\told=$(awk \"{print \\$1}\" clone2/.git/post-checkout.args) &&\n-\tnew=$(awk \"{print \\$2}\" clone2/.git/post-checkout.args) &&\n-\tflag=$(awk \"{print \\$3}\" clone2/.git/post-checkout.args) &&\n-\ttest $old = $new && test $flag = 0\n+\tgit checkout master -- three &&\n+\tread old new flag <.git/post-checkout.args &&\n+\ttest $old = $new && test $flag = 0 &&\n+\trm -f .git/post-checkout.args\n '\n \n if test \"$(git config --bool core.filemode)\" = true; then\n mkdir -p templates/hooks\n-cat >templates/hooks/post-checkout <<'EOF'\n-#!/bin/sh\n-echo $@ > $GIT_DIR/post-checkout.args\n+write_script templates/hooks/post-checkout <<-\\EOF\n+echo $@ >$GIT_DIR/post-checkout.args\n EOF\n-chmod +x templates/hooks/post-checkout\n \n test_expect_success 'post-checkout hook is triggered by clone' '\n \tgit clone --template=templates . clone3 &&\n-- \n2.20.1\n\n"},{"id":"365791","messageId":"20181224212425.16596-3-orgads@gmail.com","threadId":"50102","inReplyTo":"20181224212425.16596-1-orgads@gmail.com","subject":"[PATCH 2/2] Rebase: Run post-checkout hook on checkout","fromName":"","fromEmail":"orgads@gmail.com","sentAt":"2018-12-24T21:24:25Z","receivedAt":"2018-12-24T21:24:44Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"From: Orgad Shaneh <orgads@gmail.com>\n\nSigned-off-by: Orgad Shaneh <orgads@gmail.com>\n---\n builtin/rebase.c              | 11 +++++++++--\n t/t5403-post-checkout-hook.sh | 20 ++++++++++++++++++++\n 2 files changed, 29 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex b5c99ec10c..7f7a2c738e 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -530,6 +530,7 @@ static int run_specific_rebase(struct rebase_options *opts)\n \n #define RESET_HEAD_DETACH (1<<0)\n #define RESET_HEAD_HARD (1<<1)\n+#define RESET_HEAD_RUN_HOOK (1<<2)\n \n static int reset_head(struct object_id *oid, const char *action,\n \t\t      const char *switch_to_branch, unsigned flags,\n@@ -537,6 +538,7 @@ static int reset_head(struct object_id *oid, const char *action,\n {\n \tunsigned detach_head = flags & RESET_HEAD_DETACH;\n \tunsigned reset_hard = flags & RESET_HEAD_HARD;\n+\tunsigned run_hook = flags & RESET_HEAD_RUN_HOOK;\n \tstruct object_id head_oid;\n \tstruct tree_desc desc[2] = { { NULL }, { NULL } };\n \tstruct lock_file lock = LOCK_INIT;\n@@ -636,6 +638,10 @@ static int reset_head(struct object_id *oid, const char *action,\n \t\t\tret = update_ref(reflog_head, \"HEAD\", oid, NULL, 0,\n \t\t\t\t\t UPDATE_REFS_MSG_ON_ERR);\n \t}\n+\tif (run_hook)\n+\t\trun_hook_le(NULL, \"post-checkout\",\n+\t\t\t    oid_to_hex(orig ? orig : &null_oid),\n+\t\t\t    oid_to_hex(oid), \"1\", NULL);\n \n leave_reset_head:\n \tstrbuf_release(&msg);\n@@ -1465,7 +1471,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),\n \t\t\t\t\t    options.switch_to);\n \t\t\t\tif (reset_head(&oid, \"checkout\",\n-\t\t\t\t\t       options.head_name, 0,\n+\t\t\t\t\t       options.head_name,\n+\t\t\t\t\t       RESET_HEAD_RUN_HOOK,\n \t\t\t\t\t       NULL, buf.buf) < 0) {\n \t\t\t\t\tret = !!error(_(\"could not switch to \"\n \t\t\t\t\t\t\t\"%s\"),\n@@ -1539,7 +1546,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tstrbuf_addf(&msg, \"%s: checkout %s\",\n \t\t    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);\n \tif (reset_head(&options.onto->object.oid, \"checkout\", NULL,\n-\t\t       RESET_HEAD_DETACH, NULL, msg.buf))\n+\t\t       RESET_HEAD_DETACH | RESET_HEAD_RUN_HOOK, NULL, msg.buf))\n \t\tdie(_(\"Could not detach HEAD\"));\n \tstrbuf_release(&msg);\n \ndiff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh\nindex 9f9a5163c5..5b4e582caa 100755\n--- a/t/t5403-post-checkout-hook.sh\n+++ b/t/t5403-post-checkout-hook.sh\n@@ -13,6 +13,8 @@ test_expect_success setup '\n \tEOF\n \ttest_commit one &&\n \ttest_commit two &&\n+\ttest_commit rebase-on-me &&\n+\tgit reset --hard HEAD^ &&\n \ttest_commit three three\n '\n \n@@ -51,6 +53,24 @@ test_expect_success 'post-checkout receives the right args when not switching br\n \trm -f .git/post-checkout.args\n '\n \n+test_expect_success 'post-checkout is triggered on rebase' '\n+\tgit checkout -b rebase-test master &&\n+\trm -f .git/post-checkout.args &&\n+\tgit rebase rebase-on-me &&\n+\tread old new flag < .git/post-checkout.args &&\n+\ttest $old != $new && test $flag = 1 &&\n+\trm -f .git/post-checkout.args\n+'\n+\n+test_expect_success 'post-checkout is triggered on rebase with fast-forward' '\n+\tgit checkout -b ff-rebase-test rebase-on-me^ &&\n+\trm -f .git/post-checkout.args &&\n+\tgit rebase rebase-on-me &&\n+\tread old new flag < .git/post-checkout.args &&\n+\ttest $old != $new && test $flag = 1 &&\n+\trm -f .git/post-checkout.args\n+'\n+\n if test \"$(git config --bool core.filemode)\" = true; then\n mkdir -p templates/hooks\n write_script templates/hooks/post-checkout <<-\\EOF\n-- \n2.20.1\n\n"},{"id":"365933","messageId":"xmqqmuopl1qz.fsf@gitster-ct.c.googlers.com","threadId":"50102","inReplyTo":"20181224212425.16596-2-orgads@gmail.com","subject":"Re: [PATCH 1/2] t5403: Refactor","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-28T22:34:12Z","receivedAt":"2018-12-28T22:34:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"orgads@gmail.com writes:\n\n> Subject: Re: [PATCH 1/2] t5403: Refactor\n\nHmph.  \"Refactor\" alone leaves readers wondering \"refactor what?\",\n\"refactor for what?\" and \"refactor how?\".  Given that the overfall\ngoal this change seeks seems to be to simplify it by losing about 20\nlines, how about justifying it like so?\n\n\tSubject: t5403: simplify by using a single repository\n\n\tThere is no strong reason to use separate clones to run\n\tthese tests; just use a single test repository prepared\n\twith more modern test_commit shell helper function.\n\n\tWhile at it, replace three \"awk '{print $N}'\" on the same\n\tfile with shell built-in \"read\" into three variables.\n\n> From: Orgad Shaneh <orgads@gmail.com>\n>\n> * Replace multiple clones and commits by test_commits.\n> * Replace 3 invocations of awk by read.\n>\n> Signed-off-by: Orgad Shaneh <orgads@gmail.com>\n> ---\n>  t/t5403-post-checkout-hook.sh | 80 +++++++++++++----------------------\n>  1 file changed, 29 insertions(+), 51 deletions(-)\n>\n> diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh\n> index fc898c9eac..9f9a5163c5 100755\n> --- a/t/t5403-post-checkout-hook.sh\n> +++ b/t/t5403-post-checkout-hook.sh\n> @@ -7,77 +7,55 @@ test_description='Test the post-checkout hook.'\n>  . ./test-lib.sh\n>  \n>  test_expect_success setup '\n> -\techo Data for commit0. >a &&\n> -\techo Data for commit0. >b &&\n> -\tgit update-index --add a &&\n> -\tgit update-index --add b &&\n> -\ttree0=$(git write-tree) &&\n> -\tcommit0=$(echo setup | git commit-tree $tree0) &&\n> -\tgit update-ref refs/heads/master $commit0 &&\n> -\tgit clone ./. clone1 &&\n> -\tgit clone ./. clone2 &&\n> -\tGIT_DIR=clone2/.git git branch new2 &&\n> -\techo Data for commit1. >clone2/b &&\n> -\tGIT_DIR=clone2/.git git add clone2/b &&\n> -\tGIT_DIR=clone2/.git git commit -m new2\n> -'\n> -\n> -for clone in 1 2; do\n> -    cat >clone${clone}/.git/hooks/post-checkout <<'EOF'\n> -#!/bin/sh\n> -echo $@ > $GIT_DIR/post-checkout.args\n> -EOF\n> -    chmod u+x clone${clone}/.git/hooks/post-checkout\n> -done\n> -\n> -test_expect_success 'post-checkout runs as expected ' '\n> -\tGIT_DIR=clone1/.git git checkout master &&\n> -\ttest -e clone1/.git/post-checkout.args\n> +\tmv .git/hooks-disabled .git/hooks &&\n\nI am not sure why you want to do this---it sends a wrong signal to\nreaders saying that you want to use whatever hook that are in the\nmoved-away .git/hooks-disabled/ directory.  I am guessing that the\nonly reason why you do this is because there must be .git/hooks\ndirectory in order for write_script below to work, so a more\nreadable approach would be to \"mkdir .git/hooks\" instead, no?\n\n> +\twrite_script .git/hooks/post-checkout <<-\\EOF &&\n> +\techo $@ >.git/post-checkout.args\n\nA dollar-at inside a shell script that is not in a pair of dq always\nmakes readers wonder if the author forgot dq around it or omitted eq\naround it deliberately; avoid it.\n\nIn this case, use \"$@\" (i.e. within dq) would be more friendly to\nreaders.  The second best is to write it $*, as in this context we\nknow that $1, $2 and $3 will not contain any $IFS, that echo will\ntake these three separate arguments and write them on one line\nseparated by a space in between, which will allow \"read A B C\" read\nthem.  Or \"$*\" to feed such a single line with three arguments\nseparated by a space in between as a single string to echo for the\nsame effect.\n\n\n> +\tEOF\n> +\ttest_commit one &&\n> +\ttest_commit two &&\n> +\ttest_commit three three\n\nMakes readers wonder why the last one duplicates.  Is this because\nyou somehow do not want to use three.t as the pathname in a later\ntest?  \"test_commit X\" that creates test file X.t is a quite well\nestablished convention (see \"git grep '\\.t\\>' t/\"), by the way.\n\n\n>  '\n>  \n>  test_expect_success 'post-checkout receives the right arguments with HEAD unchanged ' '\n> -\told=$(awk \"{print \\$1}\" clone1/.git/post-checkout.args) &&\n> -\tnew=$(awk \"{print \\$2}\" clone1/.git/post-checkout.args) &&\n> -\tflag=$(awk \"{print \\$3}\" clone1/.git/post-checkout.args) &&\n> -\ttest $old = $new && test $flag = 1\n\n> +\tgit checkout master &&\n> +\ttest -e .git/post-checkout.args &&\n\nUse \"test -f\", as you do know you'd be creating a file (\"-e\"\nsucceeds as long as it _exists_, and does not care if it is a file\nor directory or whatever).\n\n> +\tread old new flag <.git/post-checkout.args &&\n\nThis indeed is much nicer.\n\n> +\ttest $old = $new && test $flag = 1 &&\n> +\trm -f .git/post-checkout.args\n\nThe last one is not a test but a clean-up.  If any of the earlier\nstep failed (e.g. $old and $new were different), the output file\nwould be left behind, resulting in confusing the next test.\n\nInstead, do it like so:\n\n\ttest_expect_success 'title of the test' '\n        \ttest_when_finished \"rm -f .git/post-checkout.args\" &&\n\t\tgit checkout master &&\n\t\ttest -f .git/post-checkout.args &&\n\t\tread old new flag <.git/post-checkout.args &&\n\t\ttest $old = $new &&\n\t\ttest $flag = 1\n\t'\n\nThat is, use test_when_finished() before the step that creates the\nfile that may be left behind to arrange that it will be cleaned at\nthe end.\n\nThis comment on clean-up applies to _all_ tests in this patch that\nhas \"rm -f .git/post-checkout.args\" at the end.\n\n>  '\n>  \n>  test_expect_success 'post-checkout runs as expected ' '\n> -\tGIT_DIR=clone1/.git git checkout master &&\n> -\ttest -e clone1/.git/post-checkout.args\n> +\tgit checkout master &&\n> +\ttest -e .git/post-checkout.args &&\n> +\trm -f .git/post-checkout.args\n>  '\n\nNow that the script got so simplified, this one looks even more\nredundant, given that the previous one already checked the same\n\"checkout 'master' when already at the tip of 'master'\" situation.\n\nDo we still need this one, in other words?\n\n>  test_expect_success 'post-checkout args are correct with git checkout -b ' '\n> -\tGIT_DIR=clone1/.git git checkout -b new1 &&\n> -\told=$(awk \"{print \\$1}\" clone1/.git/post-checkout.args) &&\n> -\tnew=$(awk \"{print \\$2}\" clone1/.git/post-checkout.args) &&\n> -\tflag=$(awk \"{print \\$3}\" clone1/.git/post-checkout.args) &&\n> -\ttest $old = $new && test $flag = 1\n> +\tgit checkout -b new1 &&\n> +\tread old new flag <.git/post-checkout.args &&\n> +\ttest $old = $new && test $flag = 1 &&\n> +\trm -f .git/post-checkout.args\n>  '\n\nThis one forgets \"did the hook run and create the file\" before\n\"read\", unlike the previous tests.  It is not strictly necessary as\n\"read\" will fail if the file is not there, but it'd be better to be\nconsistent.\n\n>  if test \"$(git config --bool core.filemode)\" = true; then\n\nThis is a tangent but this conditional came from an ancient d42ec126\n(\"disable post-checkout test on Cygwin\", 2009-03-17) that says\n\n    disable post-checkout test on Cygwin\n    \n    It is broken because of the tricks we have to play with\n    lstat to get the bearable perfomance out of the call.\n    Sadly, it disables access to Cygwin's executable attribute,\n    which Windows filesystems do not have at all.\n\nI wonder if this is still relevant these days (Cc'ed Ramsay for\ninput).  Windows port should be running enabled hooks (and X_OK is\nmade pretty much no-op in compat/mingw.c IIUC), so the above\nconditional is overly broad anyway, even if Cygwin still has issues\nwith the executable bit.\n\n>  mkdir -p templates/hooks\n> -cat >templates/hooks/post-checkout <<'EOF'\n> -#!/bin/sh\n> -echo $@ > $GIT_DIR/post-checkout.args\n> +write_script templates/hooks/post-checkout <<-\\EOF\n> +echo $@ >$GIT_DIR/post-checkout.args\n>  EOF\n> -chmod +x templates/hooks/post-checkout\n>  \n>  test_expect_success 'post-checkout hook is triggered by clone' '\n>  \tgit clone --template=templates . clone3 &&\n"},{"id":"365934","messageId":"xmqqimzdl0v4.fsf@gitster-ct.c.googlers.com","threadId":"50102","inReplyTo":"20181224212425.16596-3-orgads@gmail.com","subject":"Re: [PATCH 2/2] Rebase: Run post-checkout hook on checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-28T22:53:19Z","receivedAt":"2018-12-28T22:53:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"orgads@gmail.com writes:\n\n> From: Orgad Shaneh <orgads@gmail.com>\n\n> Re: [PATCH 2/2] Rebase: Run post-checkout hook on checkout\n\nThere is no explanation here?  \n\nIs this a regression fix (i.e. scripted version of \"rebase\" used to\nrun the hook)?  Or a new feature (i.e. no earlier version of\n\"rebase\" run the hook but you think it ought to run it)?\n\n> Signed-off-by: Orgad Shaneh <orgads@gmail.com>\n> ---\n>  builtin/rebase.c              | 11 +++++++++--\n>  t/t5403-post-checkout-hook.sh | 20 ++++++++++++++++++++\n>  2 files changed, 29 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index b5c99ec10c..7f7a2c738e 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -530,6 +530,7 @@ static int run_specific_rebase(struct rebase_options *opts)\n>  \n>  #define RESET_HEAD_DETACH (1<<0)\n>  #define RESET_HEAD_HARD (1<<1)\n> +#define RESET_HEAD_RUN_HOOK (1<<2)\n\nWould it be plausible that the only possible hook that can be run by\nreset_head() function will always be post-checkout and nothing else,\nI wonder?  Shouldn't this bit be called *_RUN_POST_CHECKOUT to make\nsure it is specific enough?\n\n> @@ -537,6 +538,7 @@ static int reset_head(struct object_id *oid, const char *action,\n>  {\n>  \tunsigned detach_head = flags & RESET_HEAD_DETACH;\n>  \tunsigned reset_hard = flags & RESET_HEAD_HARD;\n> +\tunsigned run_hook = flags & RESET_HEAD_RUN_HOOK;\n>  \tstruct object_id head_oid;\n>  \tstruct tree_desc desc[2] = { { NULL }, { NULL } };\n>  \tstruct lock_file lock = LOCK_INIT;\n> @@ -636,6 +638,10 @@ static int reset_head(struct object_id *oid, const char *action,\n>  \t\t\tret = update_ref(reflog_head, \"HEAD\", oid, NULL, 0,\n>  \t\t\t\t\t UPDATE_REFS_MSG_ON_ERR);\n>  \t}\n> +\tif (run_hook)\n> +\t\trun_hook_le(NULL, \"post-checkout\",\n> +\t\t\t    oid_to_hex(orig ? orig : &null_oid),\n> +\t\t\t    oid_to_hex(oid), \"1\", NULL);\n>  \n\nThis function is never about checking out paths from tree-ish/index\nand is always about checking out a branch, so hardcoded \"1\" is\njustified here.  Good.\n\n> @@ -1465,7 +1471,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n>  \t\t\t\t\t    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),\n>  \t\t\t\t\t    options.switch_to);\n>  \t\t\t\tif (reset_head(&oid, \"checkout\",\n> -\t\t\t\t\t       options.head_name, 0,\n> +\t\t\t\t\t       options.head_name,\n> +\t\t\t\t\t       RESET_HEAD_RUN_HOOK,\n>  \t\t\t\t\t       NULL, buf.buf) < 0) {\n>  \t\t\t\t\tret = !!error(_(\"could not switch to \"\n>  \t\t\t\t\t\t\t\"%s\"),\n> @@ -1539,7 +1546,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n>  \tstrbuf_addf(&msg, \"%s: checkout %s\",\n>  \t\t    getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name);\n>  \tif (reset_head(&options.onto->object.oid, \"checkout\", NULL,\n> -\t\t       RESET_HEAD_DETACH, NULL, msg.buf))\n> +\t\t       RESET_HEAD_DETACH | RESET_HEAD_RUN_HOOK, NULL, msg.buf))\n>  \t\tdie(_(\"Could not detach HEAD\"));\n>  \tstrbuf_release(&msg);\n\nSo... among 6 callers of reset_head(), these two whose *action is\n\"checkout\" gets the RUN_HOOK bit.  Makes sense.\n\n> diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh\n> index 9f9a5163c5..5b4e582caa 100755\n> --- a/t/t5403-post-checkout-hook.sh\n> +++ b/t/t5403-post-checkout-hook.sh\n> @@ -13,6 +13,8 @@ test_expect_success setup '\n>  \tEOF\n>  \ttest_commit one &&\n>  \ttest_commit two &&\n> +\ttest_commit rebase-on-me &&\n> +\tgit reset --hard HEAD^ &&\n>  \ttest_commit three three\n>  '\n>  \n> @@ -51,6 +53,24 @@ test_expect_success 'post-checkout receives the right args when not switching br\n>  \trm -f .git/post-checkout.args\n>  '\n>  \n> +test_expect_success 'post-checkout is triggered on rebase' '\n> +\tgit checkout -b rebase-test master &&\n> +\trm -f .git/post-checkout.args &&\n\nRead the title of this whole test script file; it should verify what\nis in the file before removing it.\n\n> +\tgit rebase rebase-on-me &&\n> +\tread old new flag < .git/post-checkout.args &&\n\nNo SP between \"<\" and \".git/post-checkout.args\".\n\n> +\ttest $old != $new && test $flag = 1 &&\n> +\trm -f .git/post-checkout.args\n> +'\n\nRegarding the clean-up of this file, see my review on the previous\none.\n\n\n> +test_expect_success 'post-checkout is triggered on rebase with fast-forward' '\n\nThe same comment as above applies to this.\n\n\nThanks.\n"},{"id":"365940","messageId":"5d3609cd-c934-fd3a-437a-30c6db930093@ramsayjones.plus.com","threadId":"50102","inReplyTo":"xmqqmuopl1qz.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] t5403: Refactor","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-12-29T03:06:09Z","receivedAt":"2018-12-29T04:42:38Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 28/12/2018 22:34, Junio C Hamano wrote:\n> orgads@gmail.com writes:\n> \n>> Subject: Re: [PATCH 1/2] t5403: Refactor\n> \n[snip]\n>>  if test \"$(git config --bool core.filemode)\" = true; then\n> \n> This is a tangent but this conditional came from an ancient d42ec126\n> (\"disable post-checkout test on Cygwin\", 2009-03-17) that says\n> \n>     disable post-checkout test on Cygwin\n>     \n>     It is broken because of the tricks we have to play with\n>     lstat to get the bearable perfomance out of the call.\n>     Sadly, it disables access to Cygwin's executable attribute,\n>     which Windows filesystems do not have at all.\n> \n> I wonder if this is still relevant these days (Cc'ed Ramsay for\n> input).  \n\nAh, no, the 'tricks we have to play with lstat' mentioned in that\ncommit message are long gone! ;-) If you remove that conditional,\nthen the test passes just fine.\n\nATB,\nRamsay Jones\n"},{"id":"365975","messageId":"CAGHpTBK+1JxhqGDNsjY4Qevd65gSYYeVMBVpHxa0=0OrKbbrEg@mail.gmail.com","threadId":"50102","inReplyTo":"xmqqimzdl0v4.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] Rebase: Run post-checkout hook on checkout","fromName":"Orgad Shaneh","fromEmail":"orgads@gmail.com","sentAt":"2018-12-29T21:31:28Z","receivedAt":"2018-12-29T21:31:42Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"On Sat, Dec 29, 2018 at 12:53 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> orgads@gmail.com writes:\n>\n> > From: Orgad Shaneh <orgads@gmail.com>\n>\n> > Re: [PATCH 2/2] Rebase: Run post-checkout hook on checkout\n>\n> There is no explanation here?\n>\n> Is this a regression fix (i.e. scripted version of \"rebase\" used to\n> run the hook)?  Or a new feature (i.e. no earlier version of\n> \"rebase\" run the hook but you think it ought to run it)?\n\nAdded an explanation.\n\n[snip]\n>\n> > +#define RESET_HEAD_RUN_HOOK (1<<2)\n>\n> Would it be plausible that the only possible hook that can be run by\n> reset_head() function will always be post-checkout and nothing else,\n> I wonder?  Shouldn't this bit be called *_RUN_POST_CHECKOUT to make\n> sure it is specific enough?\n\nDone\n\n[snip]\n>\n> > +test_expect_success 'post-checkout is triggered on rebase' '\n> > +     git checkout -b rebase-test master &&\n> > +     rm -f .git/post-checkout.args &&\n>\n> Read the title of this whole test script file; it should verify what\n> is in the file before removing it.\n\nWhy? Other tests already do it. This test is meant for rebase, not\nplain checkout.\n\n>\n> > +     git rebase rebase-on-me &&\n> > +     read old new flag < .git/post-checkout.args &&\n>\n> No SP between \"<\" and \".git/post-checkout.args\".\n\nDone\n\n> > +     test $old != $new && test $flag = 1 &&\n> > +     rm -f .git/post-checkout.args\n> > +'\n>\n> Regarding the clean-up of this file, see my review on the previous\n> one.\n\nDone\n\n> > +test_expect_success 'post-checkout is triggered on rebase with fast-forward' '\n>\n> The same comment as above applies to this.\n\nDone\n\nThanks,\n- Orgad\n"},{"id":"365976","messageId":"CAGHpTBKskY8AN0_nT5k=Syi8wnpf0EquUKMOAt2NkDq+bc=SGQ@mail.gmail.com","threadId":"50102","inReplyTo":"xmqqmuopl1qz.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/2] t5403: Refactor","fromName":"Orgad Shaneh","fromEmail":"orgads@gmail.com","sentAt":"2018-12-29T21:36:38Z","receivedAt":"2018-12-29T21:36:53Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"On Sat, Dec 29, 2018 at 12:34 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> orgads@gmail.com writes:\n>\n> > Subject: Re: [PATCH 1/2] t5403: Refactor\n>\n> Hmph.  \"Refactor\" alone leaves readers wondering \"refactor what?\",\n> \"refactor for what?\" and \"refactor how?\".  Given that the overfall\n> goal this change seeks seems to be to simplify it by losing about 20\n> lines, how about justifying it like so?\n>\n>         Subject: t5403: simplify by using a single repository\n>\n>         There is no strong reason to use separate clones to run\n>         these tests; just use a single test repository prepared\n>         with more modern test_commit shell helper function.\n>\n>         While at it, replace three \"awk '{print $N}'\" on the same\n>         file with shell built-in \"read\" into three variables.\n\nDone\n\n[snip]\n> > +     mv .git/hooks-disabled .git/hooks &&\n>\n> I am not sure why you want to do this---it sends a wrong signal to\n> readers saying that you want to use whatever hook that are in the\n> moved-away .git/hooks-disabled/ directory.  I am guessing that the\n> only reason why you do this is because there must be .git/hooks\n> directory in order for write_script below to work, so a more\n> readable approach would be to \"mkdir .git/hooks\" instead, no?\n\nAgreed. Done.\n\n> > +     write_script .git/hooks/post-checkout <<-\\EOF &&\n> > +     echo $@ >.git/post-checkout.args\n>\n> A dollar-at inside a shell script that is not in a pair of dq always\n> makes readers wonder if the author forgot dq around it or omitted eq\n> around it deliberately; avoid it.\n>\n> In this case, use \"$@\" (i.e. within dq) would be more friendly to\n> readers.\n\nDone.\n\n[snip]\n> > +     EOF\n> > +     test_commit one &&\n> > +     test_commit two &&\n> > +     test_commit three three\n>\n> Makes readers wonder why the last one duplicates.  Is this because\n> you somehow do not want to use three.t as the pathname in a later\n> test?  \"test_commit X\" that creates test file X.t is a quite well\n> established convention (see \"git grep '\\.t\\>' t/\"), by the way.\n\nDone. I wasn't aware of that.\n\n[snip]\n> > +     test -e .git/post-checkout.args &&\n>\n> Use \"test -f\", as you do know you'd be creating a file (\"-e\"\n> succeeds as long as it _exists_, and does not care if it is a file\n> or directory or whatever).\n\nJust removed these `test -e` lines. read fails anyway if the file doesn't exist.\n\n> > +     read old new flag <.git/post-checkout.args &&\n>\n> This indeed is much nicer.\n\nCredit goes to Johannes :)\n\n> > +     test $old = $new && test $flag = 1 &&\n> > +     rm -f .git/post-checkout.args\n>\n> The last one is not a test but a clean-up.  If any of the earlier\n> step failed (e.g. $old and $new were different), the output file\n> would be left behind, resulting in confusing the next test.\n>\n> Instead, do it like so:\n>\n>         test_expect_success 'title of the test' '\n>                 test_when_finished \"rm -f .git/post-checkout.args\" &&\n>                 git checkout master &&\n>                 test -f .git/post-checkout.args &&\n>                 read old new flag <.git/post-checkout.args &&\n>                 test $old = $new &&\n>                 test $flag = 1\n>         '\n>\n> That is, use test_when_finished() before the step that creates the\n> file that may be left behind to arrange that it will be cleaned at\n> the end.\n>\n> This comment on clean-up applies to _all_ tests in this patch that\n> has \"rm -f .git/post-checkout.args\" at the end.\n\nDone\n\n> >  test_expect_success 'post-checkout runs as expected ' '\n> > -     GIT_DIR=clone1/.git git checkout master &&\n> > -     test -e clone1/.git/post-checkout.args\n> > +     git checkout master &&\n> > +     test -e .git/post-checkout.args &&\n> > +     rm -f .git/post-checkout.args\n> >  '\n>\n> Now that the script got so simplified, this one looks even more\n> redundant, given that the previous one already checked the same\n> \"checkout 'master' when already at the tip of 'master'\" situation.\n>\n> Do we still need this one, in other words?\n\nI agree. Removed.\n\n> >  test_expect_success 'post-checkout args are correct with git checkout -b ' '\n> > -     GIT_DIR=clone1/.git git checkout -b new1 &&\n> > -     old=$(awk \"{print \\$1}\" clone1/.git/post-checkout.args) &&\n> > -     new=$(awk \"{print \\$2}\" clone1/.git/post-checkout.args) &&\n> > -     flag=$(awk \"{print \\$3}\" clone1/.git/post-checkout.args) &&\n> > -     test $old = $new && test $flag = 1\n> > +     git checkout -b new1 &&\n> > +     read old new flag <.git/post-checkout.args &&\n> > +     test $old = $new && test $flag = 1 &&\n> > +     rm -f .git/post-checkout.args\n> >  '\n>\n> This one forgets \"did the hook run and create the file\" before\n> \"read\", unlike the previous tests.  It is not strictly necessary as\n> \"read\" will fail if the file is not there, but it'd be better to be\n> consistent.\n\nMade consistent by removing file existence test, and left only read.\n\n> >  if test \"$(git config --bool core.filemode)\" = true; then\n>\n> This is a tangent but this conditional came from an ancient d42ec126\n> (\"disable post-checkout test on Cygwin\", 2009-03-17) that says\n>\n>     disable post-checkout test on Cygwin\n>\n>     It is broken because of the tricks we have to play with\n>     lstat to get the bearable perfomance out of the call.\n>     Sadly, it disables access to Cygwin's executable attribute,\n>     which Windows filesystems do not have at all.\n>\n> I wonder if this is still relevant these days (Cc'ed Ramsay for\n> input).  Windows port should be running enabled hooks (and X_OK is\n> made pretty much no-op in compat/mingw.c IIUC), so the above\n> conditional is overly broad anyway, even if Cygwin still has issues\n> with the executable bit.\n\nReverted.\n\nThanks,\n- Orgad\n"}]}