{"thread":{"id":"50084","subject":"[PATCH 1/2] t5403: Refactor","startedAt":"2018-12-20T21:48:37Z","lastAt":"2018-12-24T21:24:18Z","messageCount":6,"participants":["orgads@gmail.com","Johannes Schindelin","Orgad Shaneh"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"365705","messageId":"20181220214823.21378-1-orgads@gmail.com","threadId":"50084","inReplyTo":null,"subject":"[PATCH 1/2] t5403: Refactor","fromName":"","fromEmail":"orgads@gmail.com","sentAt":"2018-12-20T21:48:22Z","receivedAt":"2018-12-20T21:48: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\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 | 55 ++++++++++++-----------------------\n 1 file changed, 18 insertions(+), 37 deletions(-)\n\ndiff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh\nindex fc898c9eac..7e941537f9 100755\n--- a/t/t5403-post-checkout-hook.sh\n+++ b/t/t5403-post-checkout-hook.sh\n@@ -7,67 +7,48 @@ 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+\ttest_commit one &&\n+    test_commit two &&\n+    test_commit three three &&\n+    mv .git/hooks-disabled .git/hooks\n '\n \n-for clone in 1 2; do\n-    cat >clone${clone}/.git/hooks/post-checkout <<'EOF'\n+cat >.git/hooks/post-checkout <<'EOF'\n #!/bin/sh\n-echo $@ > $GIT_DIR/post-checkout.args\n+echo $@ > .git/post-checkout.args\n EOF\n-    chmod u+x clone${clone}/.git/hooks/post-checkout\n-done\n+chmod u+x .git/hooks/post-checkout\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 '\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+\tread old new flag < .git/post-checkout.args &&\n \ttest $old = $new && test $flag = 1\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 '\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+\tgit checkout -b new1 &&\n+    read old new flag < .git/post-checkout.args &&\n \ttest $old = $new && test $flag = 1\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+\tgit checkout two &&\n+    read old new flag < .git/post-checkout.args &&\n \ttest $old != $new && test $flag = 1\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+\tgit checkout master -- three &&\n+    read old new flag < .git/post-checkout.args &&\n \ttest $old = $new && test $flag = 0\n '\n \n-- \n2.20.1\n\n"},{"id":"365706","messageId":"20181220214823.21378-2-orgads@gmail.com","threadId":"50084","inReplyTo":"20181220214823.21378-1-orgads@gmail.com","subject":"[PATCH 2/2] Rebase: Run post-checkout hook on checkout","fromName":"","fromEmail":"orgads@gmail.com","sentAt":"2018-12-20T21:48:23Z","receivedAt":"2018-12-20T21:48:40Z","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              |  8 +++++++-\n t/t5403-post-checkout-hook.sh | 18 ++++++++++++++++++\n 2 files changed, 25 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex b5c99ec10c..78a09dcda2 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@@ -1539,7 +1545,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 7e941537f9..de9c7fb871 100755\n--- a/t/t5403-post-checkout-hook.sh\n+++ b/t/t5403-post-checkout-hook.sh\n@@ -9,6 +9,8 @@ test_description='Test the post-checkout hook.'\n test_expect_success setup '\n \ttest_commit one &&\n     test_commit two &&\n+    test_commit rebase-on-me &&\n+    git reset --hard HEAD^ &&\n     test_commit three three &&\n     mv .git/hooks-disabled .git/hooks\n '\n@@ -52,6 +54,22 @@ test_expect_success 'post-checkout receives the right args when not switching br\n \ttest $old = $new && test $flag = 0\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+'\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+'\n+\n if test \"$(git config --bool core.filemode)\" = true; then\n mkdir -p templates/hooks\n cat >templates/hooks/post-checkout <<'EOF'\n-- \n2.20.1\n\n"},{"id":"365740","messageId":"nycvar.QRO.7.76.6.1812211659220.41@tvgsbejvaqbjf.bet","threadId":"50084","inReplyTo":"20181220214823.21378-1-orgads@gmail.com","subject":"Re: [PATCH 1/2] t5403: Refactor","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-12-21T16:06:40Z","receivedAt":"2018-12-21T16:06:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Orgad,\n\nOn Thu, 20 Dec 2018, orgads@gmail.com wrote:\n\n> diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh\n> index fc898c9eac..7e941537f9 100755\n> --- a/t/t5403-post-checkout-hook.sh\n> +++ b/t/t5403-post-checkout-hook.sh\n> @@ -7,67 +7,48 @@ 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> +\ttest_commit one &&\n> +    test_commit two &&\n> +    test_commit three three &&\n\nA very nice simplification (but please use tabs to indent).\n\n> +    mv .git/hooks-disabled .git/hooks\n>  '\n>  \n> -for clone in 1 2; do\n> -    cat >clone${clone}/.git/hooks/post-checkout <<'EOF'\n> +cat >.git/hooks/post-checkout <<'EOF'\n>  #!/bin/sh\n> -echo $@ > $GIT_DIR/post-checkout.args\n> +echo $@ > .git/post-checkout.args\n\nWhile at it, you could lose the space after the redirector that we seem to\nno longer prefer:\n\n> +echo $@ >.git/post-checkout.args\n\nAnd since we are already cleaning up, we could easily move use\nwrite_script instead, *and* move it into the `setup` test case (which\nmakes it easier to use something like\n\n\tsh t5403-post-checkout-hook.sh --run=1,13\n\nThe rest looks good (modulo indentation issues). I would have preferred\nthe separate concerns to be addressed in individual commits (one commit to\nreplace the `awk` calls, one to avoid the clones, one to simplify by using\n`test_commit`, etc), as that would have been easier to review. But others\nmight disagree (Junio recently made the case of smooshing separate\nconcerns into single commits, even squashing two of my patches into one\nagainst my wish), so... I guess you don't have to change this.\n\nThank you,\nJohannes\n\n>  EOF\n> -    chmod u+x clone${clone}/.git/hooks/post-checkout\n> -done\n> +chmod u+x .git/hooks/post-checkout\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>  '\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> +\tread old new flag < .git/post-checkout.args &&\n>  \ttest $old = $new && test $flag = 1\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>  '\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> +\tgit checkout -b new1 &&\n> +    read old new flag < .git/post-checkout.args &&\n>  \ttest $old = $new && test $flag = 1\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> +\tgit checkout two &&\n> +    read old new flag < .git/post-checkout.args &&\n>  \ttest $old != $new && test $flag = 1\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> +\tgit checkout master -- three &&\n> +    read old new flag < .git/post-checkout.args &&\n>  \ttest $old = $new && test $flag = 0\n>  '\n>  \n> -- \n> 2.20.1\n> \n> \n"},{"id":"365741","messageId":"nycvar.QRO.7.76.6.1812211708010.41@tvgsbejvaqbjf.bet","threadId":"50084","inReplyTo":"20181220214823.21378-2-orgads@gmail.com","subject":"Re: [PATCH 2/2] Rebase: Run post-checkout hook on checkout","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-12-21T16:12:08Z","receivedAt":"2018-12-21T16:12:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Orgad,\n\nOn Thu, 20 Dec 2018, orgads@gmail.com wrote:\n\n> From: Orgad Shaneh <orgads@gmail.com>\n> \n> Signed-off-by: Orgad Shaneh <orgads@gmail.com>\n\nFeel free to steal the PR description I added to your PR at\nhttps://github.com/git-for-windows/git/pull/1992\n\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index b5c99ec10c..78a09dcda2 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\nIIRC there was one `git checkout` in the scripted version that ran the\nhook, and one did not. The latter did not run it because it did not\nactually change the HEAD, but just switched branches.\n\nWe could imitate that here by extending the `if (run_hook)` to `if\n(run_hook && !oideq(&head_oid, oid))`, I think. Do you agree?\n\nThe rest of the patch makes sense to me (and I merged the above-mentioned\nPR already).\n\nThank you!\nJohannes\n\n>  \n>  leave_reset_head:\n>  \tstrbuf_release(&msg);\n> @@ -1539,7 +1545,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>  \n> diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh\n> index 7e941537f9..de9c7fb871 100755\n> --- a/t/t5403-post-checkout-hook.sh\n> +++ b/t/t5403-post-checkout-hook.sh\n> @@ -9,6 +9,8 @@ test_description='Test the post-checkout hook.'\n>  test_expect_success setup '\n>  \ttest_commit one &&\n>      test_commit two &&\n> +    test_commit rebase-on-me &&\n> +    git reset --hard HEAD^ &&\n>      test_commit three three &&\n>      mv .git/hooks-disabled .git/hooks\n>  '\n> @@ -52,6 +54,22 @@ test_expect_success 'post-checkout receives the right args when not switching br\n>  \ttest $old = $new && test $flag = 0\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> +'\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> +'\n> +\n>  if test \"$(git config --bool core.filemode)\" = true; then\n>  mkdir -p templates/hooks\n>  cat >templates/hooks/post-checkout <<'EOF'\n> -- \n> 2.20.1\n> \n> \n"},{"id":"365787","messageId":"CAGHpTBLP2emg5L9vT9OPneapNP_6eHRXYF-9jT20ZkD07HJKJw@mail.gmail.com","threadId":"50084","inReplyTo":"CAGHpTB+9L55Gvezhb7x6Kb49WS_nfzmKdVvpH3_=6GM7y8YQ_g@mail.gmail.com","subject":"Re: [PATCH 1/2] t5403: Refactor","fromName":"Orgad Shaneh","fromEmail":"orgads@gmail.com","sentAt":"2018-12-24T20:54:23Z","receivedAt":"2018-12-24T20:54:37Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"Hi Johannes,\n\nThanks for reviewing this.\n\nOn Fri, Dec 21, 2018 at 6:06 PM Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n>\n> Hi Orgad,\n>\n> On Thu, 20 Dec 2018, orgads@gmail.com wrote:\n>\n> > diff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh\n> > index fc898c9eac..7e941537f9 100755\n> > --- a/t/t5403-post-checkout-hook.sh\n> > +++ b/t/t5403-post-checkout-hook.sh\n> > @@ -7,67 +7,48 @@ test_description='Test the post-checkout hook.'\n> >  . ./test-lib.sh\n> >\n> >  test_expect_success setup '\n> > -     echo Data for commit0. >a &&\n> > -     echo Data for commit0. >b &&\n> > -     git update-index --add a &&\n> > -     git update-index --add b &&\n> > -     tree0=$(git write-tree) &&\n> > -     commit0=$(echo setup | git commit-tree $tree0) &&\n> > -     git update-ref refs/heads/master $commit0 &&\n> > -     git clone ./. clone1 &&\n> > -     git clone ./. clone2 &&\n> > -     GIT_DIR=clone2/.git git branch new2 &&\n> > -     echo Data for commit1. >clone2/b &&\n> > -     GIT_DIR=clone2/.git git add clone2/b &&\n> > -     GIT_DIR=clone2/.git git commit -m new2\n> > +     test_commit one &&\n> > +    test_commit two &&\n> > +    test_commit three three &&\n>\n> A very nice simplification (but please use tabs to indent).\n\nThanks. I already did. I sent these patches twice - first revision had\nspaces, and the second one had tabs.\n\n> > +    mv .git/hooks-disabled .git/hooks\n> >  '\n> >\n> > -for clone in 1 2; do\n> > -    cat >clone${clone}/.git/hooks/post-checkout <<'EOF'\n> > +cat >.git/hooks/post-checkout <<'EOF'\n> >  #!/bin/sh\n> > -echo $@ > $GIT_DIR/post-checkout.args\n> > +echo $@ > .git/post-checkout.args\n>\n> While at it, you could lose the space after the redirector that we seem to\n> no longer prefer:\n>\n> > +echo $@ >.git/post-checkout.args\n>\n> And since we are already cleaning up, we could easily move use\n> write_script instead, *and* move it into the `setup` test case (which\n> makes it easier to use something like\n>\n>         sh t5403-post-checkout-hook.sh --run=1,13\n\nDone.\n\n> The rest looks good (modulo indentation issues). I would have preferred\n> the separate concerns to be addressed in individual commits (one commit to\n> replace the `awk` calls, one to avoid the clones, one to simplify by using\n> `test_commit`, etc), as that would have been easier to review. But others\n> might disagree (Junio recently made the case of smooshing separate\n> concerns into single commits, even squashing two of my patches into one\n> against my wish), so... I guess you don't have to change this.\n\n\nI also like commit granularity, but since I'm not familiar with the\nworkflow in the mailing list, how to amend and resend commits etc. I\ntry to keep the number of commits low :)\n\nThanks,\n- Orgad\n"},{"id":"365788","messageId":"CAGHpTBJ1bs9Ob-PXQaV0rhxPnSN=x7TZj-evO85k41ERaDAfmg@mail.gmail.com","threadId":"50084","inReplyTo":"nycvar.QRO.7.76.6.1812211708010.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/2] Rebase: Run post-checkout hook on checkout","fromName":"Orgad Shaneh","fromEmail":"orgads@gmail.com","sentAt":"2018-12-24T21:24:04Z","receivedAt":"2018-12-24T21:24:18Z","isPatch":true,"sender":{"key":"orgads@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1246544?v=4"},"body":"Hi Johannes,\n\nOn Fri, Dec 21, 2018 at 6:12 PM Johannes Schindelin\n<Johannes.Schindelin@gmx.de> wrote:\n>\n> Hi Orgad,\n>\n> On Thu, 20 Dec 2018, orgads@gmail.com wrote:\n>\n> > From: Orgad Shaneh <orgads@gmail.com>\n> >\n> > Signed-off-by: Orgad Shaneh <orgads@gmail.com>\n>\n> Feel free to steal the PR description I added to your PR at\n> https://github.com/git-for-windows/git/pull/1992\n>\n> > diff --git a/builtin/rebase.c b/builtin/rebase.c\n> > index b5c99ec10c..78a09dcda2 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> >                     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> >       unsigned detach_head = flags & RESET_HEAD_DETACH;\n> >       unsigned reset_hard = flags & RESET_HEAD_HARD;\n> > +     unsigned run_hook = flags & RESET_HEAD_RUN_HOOK;\n> >       struct object_id head_oid;\n> >       struct tree_desc desc[2] = { { NULL }, { NULL } };\n> >       struct lock_file lock = LOCK_INIT;\n> > @@ -636,6 +638,10 @@ static int reset_head(struct object_id *oid, const char *action,\n> >                       ret = update_ref(reflog_head, \"HEAD\", oid, NULL, 0,\n> >                                        UPDATE_REFS_MSG_ON_ERR);\n> >       }\n> > +     if (run_hook)\n> > +             run_hook_le(NULL, \"post-checkout\",\n> > +                         oid_to_hex(orig ? orig : &null_oid),\n> > +                         oid_to_hex(oid), \"1\", NULL);\n>\n> IIRC there was one `git checkout` in the scripted version that ran the\n> hook, and one did not. The latter did not run it because it did not\n> actually change the HEAD, but just switched branches.\n>\n> We could imitate that here by extending the `if (run_hook)` to `if\n> (run_hook && !oideq(&head_oid, oid))`, I think. Do you agree?\n\nNot exactly. That's what I thought, but it looks like I was wrong.\nReturning to the branch is done in sequencer.c. The 2 calls to reset_head\nhappen on fast-forward case.\n\nOn fast-forward there is a call to reset_head with \"checkout\",\nfollowed by a call\nwith \"Fast-forwarded\". I'm not sure what exactly it does, but on my test it sent\na null oid.\n\nI did however miss the `switch-to` case, which I now added too.\n\n- Orgad\n"}]}