{"thread":{"id":"54137","subject":"post-checkout hook aborts rebase","startedAt":"2020-08-26T23:10:58Z","lastAt":"2020-09-12T00:52:10Z","messageCount":16,"participants":["Tom Rutherford","Junio C Hamano","Chris Torek","Elijah Newren","Phillip Wood","Philippe Blain"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"404560","messageId":"CAHr-Uu_KeAJZrd+GzymNP47iFi+dZkvVYsWQPtzT_FQrVnWTDg@mail.gmail.com","threadId":"54137","inReplyTo":null,"subject":"post-checkout hook aborts rebase","fromName":"Tom Rutherford","fromEmail":"tmrutherford@gmail.com","sentAt":"2020-08-26T23:10:42Z","receivedAt":"2020-08-26T23:10:58Z","isPatch":false,"sender":{"key":"tmrutherford@gmail.com","avatar":"https://gravatar.com/avatar/0b30de20956a5811282a6647fdb876d7db29f5c689466ca73826fd3385e1f458?d=mp&s=160"},"body":"Hi Git community,\n\nMy team encountered an issue today which I believe is a bug, and I'm\ninterested to know if those more familiar with the codebase and\ndocumentation agree with me. Here's a bug report. Thanks for your\nattention.\n\nTom\n\n===\n\nThank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n\n1. Have a post-checkout hook that returns a nonzero exit status.\n2. `git pull --rebase` where there is at least one local commit to be\nrebased onto an incoming commit. (I assume any rebase would exhibit\nsimilar behavior, but this is how I encountered the issue).\n\nWhat did you expect to happen? (Expected behavior)\n\nThe rebase succeeds.\n\nI expect this because the documentation for the post-checkout hook\nstates, \"This hook cannot affect the outcome of git switch or git\ncheckout\" https://www.git-scm.com/docs/githooks#_post_checkout\n\nWhat happened instead? (Actual behavior)\n\n`error: could not detach HEAD` is displayed, and I end up in a\ndetached head state.\n\nWhat's different between what you expected and what actually happened?\n\nI do not expect the post-checkout hook to prevent rebase from\nsucceeding. The documentation suggests this should succeed. It seems\neither the documentation is wrong, or git's use of the hook is in this\ncase.\n\nAnything else you want to add:\n\nThe issue is elegantly explained in this stack overflow answer:\nhttps://stackoverflow.com/a/25562688/1286571\n\nIt's not clear to me if this is a bug, or an omission from the\ndocumentation. I reproduced it on 2.28, but I haven't tried building\nfrom source.\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.28.0\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 5.4.0-42-generic #46~18.04.1-Ubuntu SMP Fri Jul 10\n07:21:24 UTC 2020 x86_64\ncompiler info: gnuc: 7.5\nlibc info: glibc: 2.27\n$SHELL (typically, interactive shell): /usr/bin/fish\n\n\n[Enabled Hooks]\npre-commit\npost-checkout\n"},{"id":"404562","messageId":"xmqq7dtlt080.fsf@gitster.c.googlers.com","threadId":"54137","inReplyTo":"CAHr-Uu_KeAJZrd+GzymNP47iFi+dZkvVYsWQPtzT_FQrVnWTDg@mail.gmail.com","subject":"Re: post-checkout hook aborts rebase","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-27T00:13:19Z","receivedAt":"2020-08-27T00:13:29Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tom Rutherford writes:\n\n> What did you expect to happen? (Expected behavior)\n>\n> The rebase succeeds.\n>\n> I expect this because the documentation for the post-checkout hook\n> states, \"This hook cannot affect the outcome of git switch or git\n> checkout\" https://www.git-scm.com/docs/githooks#_post_checkout\n\nIn general, pre- hook is run before subcommand\nis executed, and by exiting with non-zero status, the hook can tell\nthe subcommand not to proceed. post- hook, on\nthe other hand, runs _after_ has done its thing already,\nso by definition, it cannot say \"Hey, , don't continue\".\n\nThat is the primary meaning of \"cannot affect\" in that description. \n\nWithout looking at the contents of the actual hook is, nobody can\nsay anything definite, but it also means that \"Your hook is NOT\nALLOWED TO do any extra modification to files and the index 'git\nswitch' or 'git checkout' made\". If \"git rebase\" or whatever\ncommand wanted to place files and the index into some state by using\n\"git checkout\" command, and if the post-checkout hook mucked with\nthe state in such a way that contradicts with what the \"git rebase\"\ncommand wanted them to be in, it is not surprising the hook's behavior\nbroke \"git rebase\"'s operation.\n"},{"id":"404563","messageId":"xmqq3649szs8.fsf@gitster.c.googlers.com","threadId":"54137","inReplyTo":"xmqq7dtlt080.fsf@gitster.c.googlers.com","subject":"Re: post-checkout hook aborts rebase","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-27T00:22:47Z","receivedAt":"2020-08-27T00:22:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> ... If \"git rebase\" or whatever\n> command wanted to place files and the index into some state by using\n> \"git checkout\" command, and if the post-checkout hook mucked with\n> the state in such a way that contradicts with what the \"git rebase\"\n> command wanted them to be in, it is not surprising the hook's behavior\n> broke \"git rebase\"'s operation.\n\nHaving said all that, I actually think that \"rebase\" shouldn't be\ninvoking \"git checkout\" (and its equivalent) internally when\nswitching to a specific version, in such a way that it would trigger\nany end-user specified hooks and allow them to muck with the working\ntree and the index state.\n\nI haven't checked the actual implementation of \"git rebase\" for\nquite some time to be sure, but we have lower-level plumbing\ncommands that are not affected by the end-user hooks for exactly\nthat kind of \"build higher-level commands by synthesis of\nlower-level machinery\", and it is very possible that what we are\nlooking at is actually a bug that needs to be fixed. I dunno.\n\nThanks.\n"},{"id":"404564","messageId":"CAHr-Uu8umDQJ=LORaBNJX+wnmaeM1hHxxpG7xROPgCqgEPrwdw@mail.gmail.com","threadId":"54137","inReplyTo":"xmqq3649szs8.fsf@gitster.c.googlers.com","subject":"Re: post-checkout hook aborts rebase","fromName":"Tom Rutherford","fromEmail":"tmrutherford@gmail.com","sentAt":"2020-08-27T00:44:16Z","receivedAt":"2020-08-27T00:44:30Z","isPatch":false,"sender":{"key":"tmrutherford@gmail.com","avatar":"https://gravatar.com/avatar/0b30de20956a5811282a6647fdb876d7db29f5c689466ca73826fd3385e1f458?d=mp&s=160"},"body":"Thank you for the response Junio.\n\nFor what it's worth, my hook does not make changes to the repo. It's\nrunning a command to check that the installed version of our\ndependencies match the version specified in the commit being checked\nout, and merely warns if the two don't match (then exits with a\nnonzero return code).\n\nFor this reason it's been convenient that the hook runs during\nrebases, but I find it surprising that the nonzero return code would\nimpact the rebase.\n\n\nTom\n\nOn Wed, Aug 26, 2020 at 5:22 PM Junio C Hamano wrote:\n>\n> Junio C Hamano writes:\n>\n> > ... If \"git rebase\" or whatever\n> > command wanted to place files and the index into some state by using\n> > \"git checkout\" command, and if the post-checkout hook mucked with\n> > the state in such a way that contradicts with what the \"git rebase\"\n> > command wanted them to be in, it is not surprising the hook's behavior\n> > broke \"git rebase\"'s operation.\n>\n> Having said all that, I actually think that \"rebase\" shouldn't be\n> invoking \"git checkout\" (and its equivalent) internally when\n> switching to a specific version, in such a way that it would trigger\n> any end-user specified hooks and allow them to muck with the working\n> tree and the index state.\n>\n> I haven't checked the actual implementation of \"git rebase\" for\n> quite some time to be sure, but we have lower-level plumbing\n> commands that are not affected by the end-user hooks for exactly\n> that kind of \"build higher-level commands by synthesis of\n> lower-level machinery\", and it is very possible that what we are\n> looking at is actually a bug that needs to be fixed. I dunno.\n>\n> Thanks.\n"},{"id":"404584","messageId":"CAPx1GvfSt=s5VP9_+ZtndHWaBZ5W7nFxAf8bTF2tXnJkS95Dfg@mail.gmail.com","threadId":"54137","inReplyTo":"CAHr-Uu8umDQJ=LORaBNJX+wnmaeM1hHxxpG7xROPgCqgEPrwdw@mail.gmail.com","subject":"Re: post-checkout hook aborts rebase","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2020-08-27T05:44:35Z","receivedAt":"2020-08-27T05:44:50Z","isPatch":false,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Wed, Aug 26, 2020 at 5:48 PM Tom Rutherford wrote:\n>\n> Thank you for the response Junio.\n>\n> For what it's worth, my hook does not make changes to the repo. It's\n> running a command to check that the installed version of our\n> dependencies match the version specified in the commit being checked\n> out, and merely warns if the two don't match (then exits with a\n> nonzero return code).\n\nI've run into this before myself. The core of the bug is that when\n`git checkout` runs the post-checkout hook, whatever exit status\nthat hook has, `git checkout` has the same exit status.\n\nThis might be intended as a feature, but if so, the documentation\nneeds a tweak: the githooks docs say in part\n\n This hook cannot affect the outcome of git checkout.\n\nIf \"outcome\" includes exit status -- to me, it does -- either the docs\nare wrong or the code is wrong.\n\n> For this reason it's been convenient that the hook runs during\n> rebases, but I find it surprising that the nonzero return code would\n> impact the rebase.\n\nI have to agree with this, too.\n\n(The simplest fix would be to have `git checkout` ignore the status\nfrom the post-checkout hook, of course, and just exit 0 for success.)\n\nChris\n"},{"id":"404621","messageId":"xmqqzh6gqe7i.fsf@gitster.c.googlers.com","threadId":"54137","inReplyTo":"CAPx1GvfSt=s5VP9_+ZtndHWaBZ5W7nFxAf8bTF2tXnJkS95Dfg@mail.gmail.com","subject":"Re: post-checkout hook aborts rebase","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-27T15:51:45Z","receivedAt":"2020-08-27T15:51:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chris Torek writes:\n\n> This might be intended as a feature, but if so, the documentation\n> needs a tweak: the githooks docs say in part\n>\n> This hook cannot affect the outcome of git checkout.\n>\n> If \"outcome\" includes exit status -- to me, it does -- either the docs\n> are wrong or the code is wrong.\n\nI suspected that this was a case of adding the hook that just runs\nwithout affecting the exit code and then somebody later changed the\nbehaviour (either deliberately or by mistake) and forgot to update\nthe documentation. But it seems that ever since the hook support\nwas introduced at 1abbe475 (post-checkout hook, tests, and docs,\n2007-09-26), the command was made to exit with the same status from\nthe hook.\n\nSo I agree that depending on reader's view on \"the outcome\", this is\ndocumented incorrectly (if the exit code is part of \"the outcome\")\nor just fine (otherwise). Apparently, the author of the patch and\nthe reviewers back then thought the latter.\n\nI still suspect that the checkout run, merely as an implementation\ndetail of rebase (or any other git subcommand), should not trigger\nany hook, but before any such code change, at least let's update the\ndocumentation to clarify what we mean by \"the outcome\".\n\nHopefully something like the below may be a good starting point?\n\n Documentation/githooks.txt | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 31b601e4bc..cf95d6d02b 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -193,7 +193,9 @@ 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+This hook cannot affect the outcome of `git switch` or `git checkout`,\n+other than that the hook's exit status becomes the exit status of\n+these two commands.\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\n\n\n\n"},{"id":"404627","messageId":"CABPp-BF=ydfHE2XqN4L9qfeAg3AZL9yNJhs4rykGj0baT1Eh6g@mail.gmail.com","threadId":"54137","inReplyTo":"xmqq3649szs8.fsf@gitster.c.googlers.com","subject":"Re: post-checkout hook aborts rebase","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-08-27T16:27:52Z","receivedAt":"2020-08-27T16:28:06Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Aug 26, 2020 at 5:24 PM Junio C Hamano wrote:\n>\n> Junio C Hamano writes:\n>\n> > ... If \"git rebase\" or whatever\n> > command wanted to place files and the index into some state by using\n> > \"git checkout\" command, and if the post-checkout hook mucked with\n> > the state in such a way that contradicts with what the \"git rebase\"\n> > command wanted them to be in, it is not surprising the hook's behavior\n> > broke \"git rebase\"'s operation.\n>\n> Having said all that, I actually think that \"rebase\" shouldn't be\n> invoking \"git checkout\" (and its equivalent) internally when\n> switching to a specific version, in such a way that it would trigger\n> any end-user specified hooks and allow them to muck with the working\n> tree and the index state.\n>\n> I haven't checked the actual implementation of \"git rebase\" for\n> quite some time to be sure, but we have lower-level plumbing\n> commands that are not affected by the end-user hooks for exactly\n> that kind of \"build higher-level commands by synthesis of\n> lower-level machinery\", and it is very possible that what we are\n> looking at is actually a bug that needs to be fixed. I dunno.\n>\n> Thanks.\n\nYes, and I think we should also make rebase stop invoking \"git commit\" too.\n"},{"id":"404631","messageId":"xmqqd03cq9r5.fsf@gitster.c.googlers.com","threadId":"54137","inReplyTo":"CABPp-BF=ydfHE2XqN4L9qfeAg3AZL9yNJhs4rykGj0baT1Eh6g@mail.gmail.com","subject":"Re: post-checkout hook aborts rebase","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-27T17:27:58Z","receivedAt":"2020-08-27T17:28:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> On Wed, Aug 26, 2020 at 5:24 PM Junio C Hamano wrote:\n>>\n>> Junio C Hamano writes:\n>>\n>> > ... If \"git rebase\" or whatever\n>> > command wanted to place files and the index into some state by using\n>> > \"git checkout\" command, and if the post-checkout hook mucked with\n>> > the state in such a way that contradicts with what the \"git rebase\"\n>> > command wanted them to be in, it is not surprising the hook's behavior\n>> > broke \"git rebase\"'s operation.\n>>\n>> Having said all that, I actually think that \"rebase\" shouldn't be\n>> invoking \"git checkout\" (and its equivalent) internally when\n>> switching to a specific version, in such a way that it would trigger\n>> any end-user specified hooks and allow them to muck with the working\n>> tree and the index state.\n>>\n>> I haven't checked the actual implementation of \"git rebase\" for\n>> quite some time to be sure, but we have lower-level plumbing\n>> commands that are not affected by the end-user hooks for exactly\n>> that kind of \"build higher-level commands by synthesis of\n>> lower-level machinery\", and it is very possible that what we are\n>> looking at is actually a bug that needs to be fixed. I dunno.\n>>\n>> Thanks.\n>\n> Yes, and I think we should also make rebase stop invoking \"git commit\" too.\n\nNote that I didn't say we should make it stop invoking \"git\ncheckout\".\n\nWe could invent a mechanism that disables all the hook invocations\nand other customizations [*1*] (done e.g. via the configuration\nvariables) for internal use of the Porcelain commands, and use it\nwhen \"rebase\" invokes Porcelains like \"checkout\", \"commit\" as its\nimplementation detail, for example (some \"invocations\" I think\nbypass the run_command() inteface and instead done by directly\ncalling the implementation detail of \"checkout\" and \"commit\", but\nthe principles are the same).\n\n[Footnote]\n\n*1* of course, it becomes a balancing act to decide what kind of\n customizations are OK to honor under such a mode.\n"},{"id":"404637","messageId":"CABPp-BEmjKR1HcZ_C0es9M7f6rMy3BLcZRdK0phm2J3VwtV26Q@mail.gmail.com","threadId":"54137","inReplyTo":"xmqqd03cq9r5.fsf@gitster.c.googlers.com","subject":"Re: post-checkout hook aborts rebase","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-08-27T17:47:08Z","receivedAt":"2020-08-27T17:47:23Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Aug 27, 2020 at 10:28 AM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> > On Wed, Aug 26, 2020 at 5:24 PM Junio C Hamano wrote:\n> >>\n> >> Junio C Hamano writes:\n> >>\n> >> > ... If \"git rebase\" or whatever\n> >> > command wanted to place files and the index into some state by using\n> >> > \"git checkout\" command, and if the post-checkout hook mucked with\n> >> > the state in such a way that contradicts with what the \"git rebase\"\n> >> > command wanted them to be in, it is not surprising the hook's behavior\n> >> > broke \"git rebase\"'s operation.\n> >>\n> >> Having said all that, I actually think that \"rebase\" shouldn't be\n> >> invoking \"git checkout\" (and its equivalent) internally when\n> >> switching to a specific version, in such a way that it would trigger\n> >> any end-user specified hooks and allow them to muck with the working\n> >> tree and the index state.\n> >>\n> >> I haven't checked the actual implementation of \"git rebase\" for\n> >> quite some time to be sure, but we have lower-level plumbing\n> >> commands that are not affected by the end-user hooks for exactly\n> >> that kind of \"build higher-level commands by synthesis of\n> >> lower-level machinery\", and it is very possible that what we are\n> >> looking at is actually a bug that needs to be fixed. I dunno.\n> >>\n> >> Thanks.\n> >\n> > Yes, and I think we should also make rebase stop invoking \"git commit\" too.\n>\n> Note that I didn't say we should make it stop invoking \"git\n> checkout\".\n\nUnderstood that you didn't say that, but I am of the opinion that we\nshould do that.\n\nInvoking \"git checkout\" and \"git commit\" were convenient\nimplementation details when rebase was written as a script. When it\nwas rewritten in C, forking out to these processes made for an easy\nconversion path (even if slightly ugly). But forking other processes\nis costly, it has given us multiple reports of unwanted side-effects\nfrom hooks[1], it makes the code more difficult to debug in a\ndebugger, etc. I think these are all problems we could avoid by no\nlonger calling these external commands.\n\n> We could invent a mechanism that disables all the hook invocations\n> and other customizations [*1*] (done e.g. via the configuration\n> variables) for internal use of the Porcelain commands, and use it\n> when \"rebase\" invokes Porcelains like \"checkout\", \"commit\" as its\n> implementation detail, for example (some \"invocations\" I think\n> bypass the run_command() inteface and instead done by directly\n> calling the implementation detail of \"checkout\" and \"commit\", but\n> the principles are the same).\n>\n> [Footnote]\n>\n> *1* of course, it becomes a balancing act to decide what kind of\n> customizations are OK to honor under such a mode.\n\nThat'd be one way to solve it, but it feels like it'd push maintenance\nburden onto future folks that people touching the commit and checkout\nbuiltins have to be aware of what other commands are using them under\nthe covers and tweak them appropriately. I'd rather checkout and\ncommit just shared the relevant bits of important code with sequencer,\nand then if people want to add configuration bits (be that hooks or\nconfig settings or whatever), then it only gets added to the commands\nthat people explicitly add them to, rather than them getting added to\nrebase via accident.\n"},{"id":"404640","messageId":"CAPx1GvfXOMFDsXE74LiG_BG5Bqb+seHDWX69xGe49C240ORi6A@mail.gmail.com","threadId":"54137","inReplyTo":"xmqqzh6gqe7i.fsf@gitster.c.googlers.com","subject":"Re: post-checkout hook aborts rebase","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2020-08-27T19:04:52Z","receivedAt":"2020-08-27T19:05:10Z","isPatch":false,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Thu, Aug 27, 2020 at 8:51 AM Junio C Hamano wrote:\n> I still suspect that the checkout run, merely as an implementation\n> detail of rebase (or any other git subcommand), should not trigger\n> any hook ...\n\nThe *last* checkout from the finished rebase perhaps *should*, but\nother than that one, that seems logically correct.\n\n> but before any such code change, at least let's update the\n> documentation to clarify what we mean by \"the outcome\".\n>\n> Hopefully something like the below may be a good starting point?\n>\n> Documentation/githooks.txt | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\n> index 31b601e4bc..cf95d6d02b 100644\n> --- a/Documentation/githooks.txt\n> +++ b/Documentation/githooks.txt\n> @@ -193,7 +193,9 @@ 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> +This hook cannot affect the outcome of `git switch` or `git checkout`,\n> +other than that the hook's exit status becomes the exit status of\n> +these two commands.\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\n\nThis looks good to me, and can either be independent of, or the first part of,\nany rebase update.\n\nChris\n"},{"id":"404654","messageId":"CABPp-BEEmqdt2_OuNQFxyf6pkppBWptkcvuAYhWX8r+_Wr8-2g@mail.gmail.com","threadId":"54137","inReplyTo":"CAPx1GvfXOMFDsXE74LiG_BG5Bqb+seHDWX69xGe49C240ORi6A@mail.gmail.com","subject":"Re: post-checkout hook aborts rebase","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-08-27T20:11:12Z","receivedAt":"2020-08-27T20:11:27Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Aug 27, 2020 at 12:07 PM Chris Torek wrote:\n>\n> On Thu, Aug 27, 2020 at 8:51 AM Junio C Hamano wrote:\n> > I still suspect that the checkout run, merely as an implementation\n> > detail of rebase (or any other git subcommand), should not trigger\n> > any hook ...\n>\n> The *last* checkout from the finished rebase perhaps *should*, but\n> other than that one, that seems logically correct.\n\nWhat do you mean by the \"last checkout\"? I believe a typical\nnon-interactive rebase of N patches has only one checkout, and I don't\nsee why running hooks for the starting point is relevant.\n\nIf hooks are wanted for rebase, I'd still want to have rebase-specific\nones, because most people who think of \"checkout hooks\" or \"commit\nhooks\" probably aren't going to think of them the way rebase or\ncherry-pick happen to use them. (And that might even be more true for\n--interactive and --rebase-merges cases.)\n\n> > but before any such code change, at least let's update the\n> > documentation to clarify what we mean by \"the outcome\".\n> >\n> > Hopefully something like the below may be a good starting point?\n> >\n> > Documentation/githooks.txt | 4 +++-\n> > 1 file changed, 3 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\n> > index 31b601e4bc..cf95d6d02b 100644\n> > --- a/Documentation/githooks.txt\n> > +++ b/Documentation/githooks.txt\n> > @@ -193,7 +193,9 @@ 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> > +This hook cannot affect the outcome of `git switch` or `git checkout`,\n> > +other than that the hook's exit status becomes the exit status of\n> > +these two commands.\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\n>\n> This looks good to me, and can either be independent of, or the first part of,\n> any rebase update.\n\nPatch looks good to me too.\n"},{"id":"404655","messageId":"CAPx1GvdQKw-4yjNMfqZtDQUBg3-dGbkPpV+0FyjJx-a3bQaQ-Q@mail.gmail.com","threadId":"54137","inReplyTo":"CABPp-BEEmqdt2_OuNQFxyf6pkppBWptkcvuAYhWX8r+_Wr8-2g@mail.gmail.com","subject":"Re: post-checkout hook aborts rebase","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2020-08-27T20:32:50Z","receivedAt":"2020-08-27T20:33:05Z","isPatch":false,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"> On Thu, Aug 27, 2020 at 12:07 PM Chris Torek wrote:\n> > The *last* checkout from the finished rebase perhaps *should* [invoke\na hook]\n\nOn Thu, Aug 27, 2020 at 1:11 PM Elijah Newren wrote:\n> What do you mean by the \"last checkout\"? I believe a typical\n> non-interactive rebase of N patches has only one checkout, and I don't\n> see why running hooks for the starting point is relevant.\n\nI meant for the final state after rewriting the commits -- but that's\nactually the post-rewrite hook, not the post-checkout hook. I had them\nconflated mentally when I wrote the above.\n\n> If hooks are wanted for rebase, I'd still want to have rebase-specific\n> ones, because most people who think of \"checkout hooks\" or \"commit\n> hooks\" probably aren't going to think of them the way rebase or\n> cherry-pick happen to use them. (And that might even be more true for\n> --interactive and --rebase-merges cases.)\n\nI agree with this.\n\nChris\n"},{"id":"405239","messageId":"dc9b58c4-96f1-2270-406b-781f8d0a4f22@gmail.com","threadId":"54137","inReplyTo":"CAHr-Uu8umDQJ=LORaBNJX+wnmaeM1hHxxpG7xROPgCqgEPrwdw@mail.gmail.com","subject":"Re: post-checkout hook aborts rebase","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-09-09T09:43:59Z","receivedAt":"2020-09-09T09:44:14Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Tom\n\nOn 27/08/2020 01:44, Tom Rutherford wrote:\n> Thank you for the response Junio.\n> \n> For what it's worth, my hook does not make changes to the repo. It's\n> running a command to check that the installed version of our\n> dependencies match the version specified in the commit being checked\n> out, and merely warns if the two don't match (then exits with a\n> nonzero return code).\n> \n> For this reason it's been convenient that the hook runs during\n> rebases, but I find it surprising that the nonzero return code would\n> impact the rebase.\n\nIf the checkout succeeds that rebase does not print any of checkout's\noutput so unfortunately you wouldn't see the message from your hook.\n\nI tend to agree with Junio that we shouldn't be running the \npost-checkout hook when rebasing.\n\nBest Wishes\n\nPhillip\n\n> \n> Tom\n> \n> On Wed, Aug 26, 2020 at 5:22 PM Junio C Hamano wrote:\n>>\n>> Junio C Hamano writes:\n>>\n>>> ... If \"git rebase\" or whatever\n>>> command wanted to place files and the index into some state by using\n>>> \"git checkout\" command, and if the post-checkout hook mucked with\n>>> the state in such a way that contradicts with what the \"git rebase\"\n>>> command wanted them to be in, it is not surprising the hook's behavior\n>>> broke \"git rebase\"'s operation.\n>>\n>> Having said all that, I actually think that \"rebase\" shouldn't be\n>> invoking \"git checkout\" (and its equivalent) internally when\n>> switching to a specific version, in such a way that it would trigger\n>> any end-user specified hooks and allow them to muck with the working\n>> tree and the index state.\n>>\n>> I haven't checked the actual implementation of \"git rebase\" for\n>> quite some time to be sure, but we have lower-level plumbing\n>> commands that are not affected by the end-user hooks for exactly\n>> that kind of \"build higher-level commands by synthesis of\n>> lower-level machinery\", and it is very possible that what we are\n>> looking at is actually a bug that needs to be fixed. I dunno.\n>>\n>> Thanks.\n\n"},{"id":"405250","messageId":"CAHr-Uu_XhLXDhGOcHGG6wiLSg=nUjAU55gtB08DfKbi5bebL4g@mail.gmail.com","threadId":"54137","inReplyTo":"dc9b58c4-96f1-2270-406b-781f8d0a4f22@gmail.com","subject":"Re: post-checkout hook aborts rebase","fromName":"Tom Rutherford","fromEmail":"tmrutherford@gmail.com","sentAt":"2020-09-09T16:07:37Z","receivedAt":"2020-09-09T16:08:39Z","isPatch":false,"sender":{"key":"tmrutherford@gmail.com","avatar":"https://gravatar.com/avatar/0b30de20956a5811282a6647fdb876d7db29f5c689466ca73826fd3385e1f458?d=mp&s=160"},"body":"Thanks Phillip,\n\nYes, I've now noticed that's the case. :)\n\nI also didn't intend to suggest that post-checkout *should* run during\nrebases, just that it would be convenient for this particular use case\nif it did.\nI think I'll need to use multiple hooks to accomplish what I want to do.\n\n\nTom\n\nOn Wed, Sep 9, 2020 at 2:44 AM Phillip Wood wrote:\n>\n> Hi Tom\n>\n> On 27/08/2020 01:44, Tom Rutherford wrote:\n> > Thank you for the response Junio.\n> >\n> > For what it's worth, my hook does not make changes to the repo. It's\n> > running a command to check that the installed version of our\n> > dependencies match the version specified in the commit being checked\n> > out, and merely warns if the two don't match (then exits with a\n> > nonzero return code).\n> >\n> > For this reason it's been convenient that the hook runs during\n> > rebases, but I find it surprising that the nonzero return code would\n> > impact the rebase.\n>\n> If the checkout succeeds that rebase does not print any of checkout's\n> output so unfortunately you wouldn't see the message from your hook.\n>\n> I tend to agree with Junio that we shouldn't be running the\n> post-checkout hook when rebasing.\n>\n> Best Wishes\n>\n> Phillip\n>\n> >\n> > Tom\n> >\n> > On Wed, Aug 26, 2020 at 5:22 PM Junio C Hamano wrote:\n> >>\n> >> Junio C Hamano writes:\n> >>\n> >>> ... If \"git rebase\" or whatever\n> >>> command wanted to place files and the index into some state by using\n> >>> \"git checkout\" command, and if the post-checkout hook mucked with\n> >>> the state in such a way that contradicts with what the \"git rebase\"\n> >>> command wanted them to be in, it is not surprising the hook's behavior\n> >>> broke \"git rebase\"'s operation.\n> >>\n> >> Having said all that, I actually think that \"rebase\" shouldn't be\n> >> invoking \"git checkout\" (and its equivalent) internally when\n> >> switching to a specific version, in such a way that it would trigger\n> >> any end-user specified hooks and allow them to muck with the working\n> >> tree and the index state.\n> >>\n> >> I haven't checked the actual implementation of \"git rebase\" for\n> >> quite some time to be sure, but we have lower-level plumbing\n> >> commands that are not affected by the end-user hooks for exactly\n> >> that kind of \"build higher-level commands by synthesis of\n> >> lower-level machinery\", and it is very possible that what we are\n> >> looking at is actually a bug that needs to be fixed. I dunno.\n> >>\n> >> Thanks.\n>\n"},{"id":"405452","messageId":"99BE0DDB-C248-4781-A715-2517A977372B@gmail.com","threadId":"54137","inReplyTo":"dc9b58c4-96f1-2270-406b-781f8d0a4f22@gmail.com","subject":"Re: post-checkout hook aborts rebase","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-09-11T20:25:28Z","receivedAt":"2020-09-11T20:25:40Z","isPatch":false,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hello everyone,\n\n> Le 9 sept. 2020 à 05:43, Phillip Wood a écrit :\n> \n> Hi Tom\n> \n> On 27/08/2020 01:44, Tom Rutherford wrote:\n>> Thank you for the response Junio.\n>> \n>> For what it's worth, my hook does not make changes to the repo. It's\n>> running a command to check that the installed version of our\n>> dependencies match the version specified in the commit being checked\n>> out, and merely warns if the two don't match (then exits with a\n>> nonzero return code).\n>> \n>> For this reason it's been convenient that the hook runs during\n>> rebases, but I find it surprising that the nonzero return code would\n>> impact the rebase.\n> \n> If the checkout succeeds that rebase does not print any of checkout's\n> output so unfortunately you wouldn't see the message from your hook.\n\nAccording to git-rebase's doc [1], that's true only for the new \"merge\" backend,\nthough, (which would be in use here in 2.28 unless 'rebase.backend' is set to 'apply').\n\nOn a slightly related note, I noticed that that part of the documentation is not 100% exact. I states:\n\"Further, both backends only call the post-checkout hook with the starting point commit of the rebase, \nnot the intermediate commits nor the final commit.\"\n\nbut I noticed that in an interactive rebase, the post-checkout hook *is* called for commits\nmarked as \"edit\", when the rebase stops to edit them (I haven't checked if it's called when the rebase stops\nfor other reasons, like conflicts.)\n\nMaybe we should tweak the wording of the doc ?\n\nCheers,\n\nPhilippe.\n\n[1] https://git-scm.com/docs/git-rebase#_hooks"},{"id":"405462","messageId":"xmqqeen7n7f6.fsf@gitster.c.googlers.com","threadId":"54137","inReplyTo":"99BE0DDB-C248-4781-A715-2517A977372B@gmail.com","subject":"Re: post-checkout hook aborts rebase","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-12T00:51:57Z","receivedAt":"2020-09-12T00:52:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philippe Blain writes:\n\n> On a slightly related note, I noticed that that part of the documentation is not 100% exact. I states:\n> \"Further, both backends only call the post-checkout hook with the starting point commit of the rebase, \n> not the intermediate commits nor the final commit.\"\n>\n> but I noticed that in an interactive rebase, the post-checkout hook *is* called for commits\n> marked as \"edit\", when the rebase stops to edit them (I haven't checked if it's called when the rebase stops\n> for other reasons, like conflicts.)\n>\n> Maybe we should tweak the wording of the doc ?\n\nYeah, we should do either that, or fix the code and make it not to\ncall the post-checkout hook so aggressively, as suggested during the\ndiscussion.\n\nThanks.\n\n\n\n"}]}