{"thread":{"id":"52007","subject":"[PATCH 0/1] Pass through the exit code of post-checkout","startedAt":"2019-10-10T12:01:42Z","lastAt":"2019-10-11T04:22:43Z","messageCount":3,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"383795","messageId":"pull.385.git.gitgitgadget@gmail.com","threadId":"52007","inReplyTo":null,"subject":"[PATCH 0/1] Pass through the exit code of post-checkout","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-10T12:01:38Z","receivedAt":"2019-10-10T12:01:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This is my attempt to revive an old discussion\n[https://public-inbox.org/git/20180314003816.GE147135@aiede.svl.corp.google.com/] \n(related to this StackOverflow thread\n[https://stackoverflow.com/questions/25561485/git-rebase-i-with-squash-cannot-detach-head]\n).\n\n> [...] is this the right behavior for \"git checkout\" to have? I.e. is it\nuseful for \"git checkout\" to fail when the post-checkout hook fails, or\nwould it be better for it to e.g. simply print a message and exit with\nstatus 0?\n\n\nTo answer Jonathan's question, at long last, yes, it is useful. A hook is\nnot only an opportunity to run code at given points in Git's life cycle, but\nalso an opportunity to stop Git in its tracks. Further, if you script the\noperation, it may very well be useful to discern between an exit code from\nGit's operation from an exit code produced by your hook.\n\nIf you don't want your git checkout/git switch/git restore to fail due to a \npost-checkout failure, just make sure that that hook does not fail ;-) (This\ncould easily be achieved by trap EXIT { exit 0; }, I believe.\n\nI discovered, however, that the original patch contribution missed that a \ngit checkout -b <branch> will not pass through the exit code, but instead\nreturn exit code 1. As part of my contribution, I fix this, and also\nintroduce a regression test to document the now-consistent behavior.\n\nJohannes Schindelin (1):\n  switch/restore: consistently pass through exit code of `post-checkout`\n\n Documentation/githooks.txt    | 3 ++-\n builtin/checkout.c            | 8 +++++---\n t/t5403-post-checkout-hook.sh | 9 +++++++++\n 3 files changed, 16 insertions(+), 4 deletions(-)\n\n\nbase-commit: 70bf0b755af4d1e66da25b7805cac0e481a082e4\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-385%2Fdscho%2Fpost-checkout-hook-exit-code-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-385/dscho/post-checkout-hook-exit-code-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/385\n-- \ngitgitgadget\n"},{"id":"383796","messageId":"83305732371f3f511b3f4acf2060610c80ad12fa.1570708898.git.gitgitgadget@gmail.com","threadId":"52007","inReplyTo":"pull.385.git.gitgitgadget@gmail.com","subject":"[PATCH 1/1] switch/restore: consistently pass through exit code of `post-checkout`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-10-10T12:01:39Z","receivedAt":"2019-10-10T12:01:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nCurrently, the exit code of the `post-checkout` hook becomes the exit\ncode of the `git restore` (or the equivalent `git checkout`) command,\n_iff_ no error occurred during the `restore` operation.\n\nThat allows versatile scripting where the `post-checkout` failure modes\ncan be discerned from the `git restore` failure modes.\n\nThere is a problem, though: if that `git restore` operation fails\n_before_ the `post-checkout` hook is run, -1 is returned from the\n`checkout_paths()` function, which yields the misleading exit code 127\n(which is reserved to indicate \"command not found\").\n\nAnother problem: a non-zero exit code of the `post-checkout` hook as run\nat the end of a `git switch` run will always causes that command to exit\nwith exit code 1.\n\nThis is inconsistent. Let's pass through the exit code of the\n`post-checkout` hook verbatim (unless the Git operation fails, in which\ncase we want to exit with code 1, even if the hook returns another exit\ncode).\n\nDocument this, and add a regression test for this behavior, too.\n\nInspired-by-a-patch-by: Magne Land <magne.land@gmail.com>\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n Documentation/githooks.txt    | 3 ++-\n builtin/checkout.c            | 8 +++++---\n t/t5403-post-checkout-hook.sh | 9 +++++++++\n 3 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 50365f2914..12c756a37b 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -193,7 +193,8 @@ worktree.  The hook is given three parameters: the ref of the previous HEAD,\n the ref of the new HEAD (which may or may not have changed), and a flag\n indicating whether the checkout was a branch checkout (changing branches,\n flag=1) or a file checkout (retrieving a file from the index, flag=0).\n-This hook cannot affect the outcome of `git switch` or `git checkout`.\n+If this hook exits with an exit code other than 0, it causes the calling\n+`git switch` or `git checkout` command to fail with the same exit code.\n \n It is also run after linkgit:git-clone[1], unless the `--no-checkout` (`-n`) option is\n used. The first parameter given to the hook is the null-ref, the second the\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 1283727761..56afc3c1a8 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -400,7 +400,7 @@ static int checkout_paths(const struct checkout_opts *opts,\n \tstatic char *ps_matched;\n \tstruct object_id rev;\n \tstruct commit *head;\n-\tint errs = 0;\n+\tint errs = 0, ret;\n \tstruct lock_file lock_file = LOCK_INIT;\n \tint checkout_index;\n \n@@ -548,8 +548,8 @@ static int checkout_paths(const struct checkout_opts *opts,\n \tread_ref_full(\"HEAD\", 0, &rev, NULL);\n \thead = lookup_commit_reference_gently(the_repository, &rev, 1);\n \n-\terrs |= post_checkout_hook(head, head, 0);\n-\treturn errs;\n+\tret = post_checkout_hook(head, head, 0);\n+\treturn !errs ? ret : (errs < 0 ? 1 : errs);\n }\n \n static void show_local_changes(struct object *head,\n@@ -1062,6 +1062,8 @@ static int switch_branches(const struct checkout_opts *opts,\n \n \tret = post_checkout_hook(old_branch_info.commit, new_branch_info->commit, 1);\n \tfree(path_to_free);\n+\tif (ret > 0)\n+\t\treturn ret;\n \treturn ret || writeout_error;\n }\n \ndiff --git a/t/t5403-post-checkout-hook.sh b/t/t5403-post-checkout-hook.sh\nindex a39b3b5c78..60ce81f6db 100755\n--- a/t/t5403-post-checkout-hook.sh\n+++ b/t/t5403-post-checkout-hook.sh\n@@ -73,4 +73,13 @@ test_expect_success 'post-checkout hook is triggered by clone' '\n \ttest -f clone3/.git/post-checkout.args\n '\n \n+test_expect_success 'exit code of post-checkout hook is passed through' '\n+\tmkdir -p exit-code &&\n+\techo \"exit 123\" | write_script exit-code/post-checkout &&\n+\ttest_expect_code 123 \\\n+\tgit -c core.hooksPath=\"$PWD/exit-code\" switch -c trigger-exit-code &&\n+\ttest_expect_code 123 \\\n+\tgit -c core.hooksPath=\"$PWD/exit-code\" restore .\n+'\n+\n test_done\n-- \ngitgitgadget\n"},{"id":"383860","messageId":"xmqqo8ynj4hd.fsf@gitster-ct.c.googlers.com","threadId":"52007","inReplyTo":"pull.385.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/1] Pass through the exit code of post-checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-11T04:22:38Z","receivedAt":"2019-10-11T04:22:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" <gitgitgadget@gmail.com>\nwrites:\n\n> To answer Jonathan's question, at long last, yes, it is useful. A hook is\n> not only an opportunity to run code at given points in Git's life cycle, but\n> also an opportunity to stop Git in its tracks.\n\nIn general that is correct, and especially so for pre_$git_action\nhooks, which are about interfering and vetoing an action that is\nbeing carried out.\n\nOn the other hand, post_$git_action hooks are specifically defined\nas a way to trigger an extra action and documented that they cannot\naffect the outcome of the operation (in other words, they trigger at\na point in the operation flow that is too late to affect the\noutcome).\n\nNow, it is somewhat debatable how the \"outcome\" should be defined.\nA post-rebase hook that drops the last commit in the sequence, for\nexample, should not be allowed (the rebase has rebuilt a sequence\nand that final sequence of commits should be left), but should it be\nallowed to signal back to \"git rebase\" and affect its exit status?\n\nI am not 100% sure if it is a good idea to allow post-checkout to\naffect the exit status of \"git checkout\" or its friends.  If one\ncodepath in \"git checkout\" or its friends lets post-checkout to\ninfluence the exit status while another codepath ignores, it makes\nsense to discuss if the inconsistency needs to be removed, but in\nsuch a case, I think it would make sense to unify in the direction\nof ignoring (i.e. not allowing post-* hook to affect the outcome).\n\n\n"}]}