{"thread":{"id":"53159","subject":"git rebase backend change: post-checkout hook is not ran by 'merge' backend","startedAt":"2020-04-02T15:57:37Z","lastAt":"2020-04-03T09:08:27Z","messageCount":4,"participants":["Philippe Blain","Elijah Newren","Phillip Wood"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"394591","messageId":"9945492E-1D97-4741-B999-F30CA561D4DE@gmail.com","threadId":"53159","inReplyTo":null,"subject":"git rebase backend change: post-checkout hook is not ran by 'merge' backend","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-04-02T15:57:28Z","receivedAt":"2020-04-02T15:57:37Z","isPatch":false,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hello,\n\nI'm not sure if this has been reported yet (couldn't find it), \nbut I noticed a behavioral difference between the merge \nand apply backends that is not mentioned in the documentation:\nThe 'apply' backend will run the post-checkout hook just after \nmoving HEAD to the commit we are rebasing onto; \nthis does not happen with the merge backend:\n\n $ echo \"echo \\\"Running post-checkout hook\\\"\" > .git/hooks/post-checkout\n $ git checkout -b \n $ git rebase upstream/master --apply\n First, rewinding head to replay your work on top of it...\n Running post-checkout hook\n Applying: \n Applying: \n $ git checkout -b \n Successfully rebased and updated refs/heads/\n\nCheers,\nPhilippe.\n"},{"id":"394592","messageId":"FB0DFFB4-0E6B-4C1E-96AE-9869752BB50B@gmail.com","threadId":"53159","inReplyTo":"9945492E-1D97-4741-B999-F30CA561D4DE@gmail.com","subject":"Re: git rebase backend change: post-checkout hook is not ran by 'merge' backend","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2020-04-02T16:00:28Z","receivedAt":"2020-04-02T16:00:37Z","isPatch":false,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Sorry, the last rebase command is missing:\n\n $ echo \"echo \\\"Running post-checkout hook\\\"\" > .git/hooks/post-checkout\n $ git checkout -b \n $ git rebase upstream/master --apply\n First, rewinding head to replay your work on top of it...\n Running post-checkout hook\n Applying: \n Applying: \n $ git checkout -b \n $ git rebase upstream/master --merge\n Successfully rebased and updated refs/heads/\n\n> Le 2 avr. 2020 à 11:57, Philippe Blain a écrit :\n> \n> $ echo \"echo \\\"Running post-checkout hook\\\"\" > .git/hooks/post-checkout\n> $ git checkout -b \n> $ git rebase upstream/master --apply\n> First, rewinding head to replay your work on top of it...\n> Running post-checkout hook\n> Applying: \n> Applying: \n> $ git checkout -b \n> Successfully rebased and updated refs/heads/\n\n"},{"id":"394605","messageId":"CABPp-BG0bFKUage5cN_2yr2DkmS04W2Z9Pg5WcROqHznV3XBdw@mail.gmail.com","threadId":"53159","inReplyTo":"9945492E-1D97-4741-B999-F30CA561D4DE@gmail.com","subject":"Re: git rebase backend change: post-checkout hook is not ran by 'merge' backend","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2020-04-02T16:58:14Z","receivedAt":"2020-04-02T16:58:28Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Thu, Apr 2, 2020 at 8:57 AM Philippe Blain\n wrote:\n>\n> Hello,\n>\n> I'm not sure if this has been reported yet (couldn't find it),\n> but I noticed a behavioral difference between the merge\n> and apply backends that is not mentioned in the documentation:\n> The 'apply' backend will run the post-checkout hook just after\n> moving HEAD to the commit we are rebasing onto;\n> this does not happen with the merge backend:\n>\n> $ echo \"echo \\\"Running post-checkout hook\\\"\" > .git/hooks/post-checkout\n> $ git checkout -b \n> $ git rebase upstream/master --apply\n> First, rewinding head to replay your work on top of it...\n> Running post-checkout hook\n> Applying: \n> Applying: \n> $ git checkout -b \n> Successfully rebased and updated refs/heads/\n>\n> Cheers,\n> Philippe.\n\nInteresting. We had a report about the post-commit hook, but not\nabout post-checkout. From the description in the githooks manpage\nit's not clear to me that either rebase backend is correct or\nincorrect here, but they should at least be consistent. Also, if\nrebase should call post-checkout, should cherry-pick? If cherry-pick\nshould, should commit or revert call it? What about reset or merge?\nThis hook to me seems to be rather badly defined because If any\noperations other than checkout/switch should call it, then you've got\nvarious angles of weirdness if you don't include all the others too.\n(Also, digging through the history, the only reason that the apply\nbackend called the post-checkout hook was because it was written as a\nscript and invoked 'git checkout'. But then someone noticed that the\nscripted version called the hook and thus ported it to the builtin.)\nFrom another angle, maybe you could make the cutoff be only when HEAD\nchanges which symref it points to (meaning rebase would only call it\nif you asked it to rebase a different branch than you were on), but\nthat has its own weirdness too. I think this hook is really weird,\nunless we restrict it to only the things already explicitly mentioned\nin the githooks manpage.\n\nIn any event, I guess that means we need to update this section of the\nrebase manpage:\n\n\"\"\"\nHooks\n~~~~~\n\nThe apply backend has not traditionally called the post-commit hook,\nwhile the merge backend has. However, this was by accident of\nimplementation rather than by design. Both backends should have the\nsame behavior, though it is not clear which one is correct.\n\"\"\"\n\nto also mention post-checkout, note that the situation about which\nbackend calls this hook is reversed relative to post-commit, and again\nmention it's not clear which backend is correct.\n\nMy very rough opinion of the moment is:\n * post-checkout should probably do as the manpage says, and only run\nfor checkout/switch plus new worktree situations (worktree add/clone).\nrebase --apply should stop calling it.\n * post-commit is horribly broken; it hardcodes an assumption of\nexactly one commit. Also, it's a huge performance problem when things\nwant to create multiple commits. So, let's do as the githooks manpage\nsuggest and call this hook from a direct \"git commit\" -- and _only_\nfrom there. Remove it from rebase, cherry-pick, revert, etc.\nNormally, for consistency, I would say that merge should start calling\nthat hook (it only creates one commit, and \"git commit\" is used to\ncomplete an interrupted merge anyway so why not uninterrupted ones),\nbut merge can be called by rebase --rebase-merges right now so that\npushes us back to the get it out of rebase side. The hook is kinda\nbroken anyway, so maybe limit exposure?\n * Make a new post-batch-commit hook called by things that create new\ncommits, but only call it once per operation (e.g. rebase and\ncherry-pick call it once rather than once per commit). Have commit,\nmerge, and revert all call this too. Not sure what to do about things\nthat change multiple refs simultaneously, e.g. fast-import (or things\nlike it, e.g. filter-branch).\n\nCC'ing Jonathan and Emily since they've thought a lot about hooks, and\nwhom I thought might be making changes in this area[1][2].\n\n\nElijah\n\n[1] https://lore.kernel.org/git/20200116203521.GA71299@google.com/\n[2] https://lore.kernel.org/git/20200115215922.GI181522@google.com/\n"},{"id":"394646","messageId":"6c413abc-f48d-5a02-ac8b-09a0fa80b98d@gmail.com","threadId":"53159","inReplyTo":"CABPp-BG0bFKUage5cN_2yr2DkmS04W2Z9Pg5WcROqHznV3XBdw@mail.gmail.com","subject":"Re: git rebase backend change: post-checkout hook is not ran by 'merge' backend","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-04-03T09:08:20Z","receivedAt":"2020-04-03T09:08:27Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi\n\nOn 02/04/2020 17:58, Elijah Newren wrote:\n> Hi,\n> \n> On Thu, Apr 2, 2020 at 8:57 AM Philippe Blain\n> wrote:\n>>\n>> Hello,\n>>\n>> I'm not sure if this has been reported yet (couldn't find it),\n>> but I noticed a behavioral difference between the merge\n>> and apply backends that is not mentioned in the documentation:\n>> The 'apply' backend will run the post-checkout hook just after\n>> moving HEAD to the commit we are rebasing onto;\n>> this does not happen with the merge backend:\n>>\n>> $ echo \"echo \\\"Running post-checkout hook\\\"\" > .git/hooks/post-checkout\n>> $ git checkout -b \n>> $ git rebase upstream/master --apply\n>> First, rewinding head to replay your work on top of it...\n>> Running post-checkout hook\n>> Applying: \n>> Applying: \n>> $ git checkout -b \n>> Successfully rebased and updated refs/heads/\n\nThe merge backend runs `git checkout` to do the checkout and so does \nactually run the post-checkout hook. However the invocation is wrapped \nin run_command_silent_on_success() which means if the checkout is \nsuccessful you don't see the output from the hook (and if the checkout \nfails you don't see the output from the hook because it is not run). You \ncan verify this by changing the first line of your example to\n\n$ echo \"echo \\\"Running post-checkout hook\\\" >>/tmp/post-checkout\" > \n.git/hooks/post-checkout\n\nand examining /tmp/post-checkout\n\n>> Cheers,\n>> Philippe.\n> \n> Interesting. We had a report about the post-commit hook, but not\n> about post-checkout. From the description in the githooks manpage\n> it's not clear to me that either rebase backend is correct or\n> incorrect here, but they should at least be consistent. Also, if\n> rebase should call post-checkout, should cherry-pick? If cherry-pick\n> should, should commit or revert call it?\n\ncherry-pick/revert don't check anything out\n\n> What about reset or merge?\n\nagain they are not checking out an existing commit.\n\nHowever the 'reset' command for rebase -r is checking out an existing \ncommit so if we decide to keep running the hook for the initial checkout \nthen I think we should do it after those commands as well as there's \nnothing special about the initial checkout.\n\nAs for whether we should be running the hook I think it depends on what \npeople are using it for. If they have a pair of pre-commit and \npost-checkout hooks to handle file metadata such as permissions and acls \nthey might want to run the post-checkout hook so the file metadata gets \nupdated but then there is no support for this when picking the rebased \ncommits so it's arguably pointless to do it on the initial checkout.\n\nIf they are using it to be notified of branch switches then maybe we \ndon't need to run it when checking out the onto commit (unless they want \nto know that we've switched to a detached HEAD). Though we would still \nwant to do it for the branch switch in 'git rebase '\n\nAs you can see I'm waffling and have no idea what's the right thing to do!\n\nBest Wishes\n\nPhillip\n\n\n> This hook to me seems to be rather badly defined because If any\n> operations other than checkout/switch should call it, then you've got\n> various angles of weirdness if you don't include all the others too.\n> (Also, digging through the history, the only reason that the apply\n> backend called the post-checkout hook was because it was written as a\n> script and invoked 'git checkout'. But then someone noticed that the\n> scripted version called the hook and thus ported it to the builtin.)\n> From another angle, maybe you could make the cutoff be only when HEAD\n> changes which symref it points to (meaning rebase would only call it\n> if you asked it to rebase a different branch than you were on), but\n> that has its own weirdness too. I think this hook is really weird,\n> unless we restrict it to only the things already explicitly mentioned\n> in the githooks manpage.\n> \n> In any event, I guess that means we need to update this section of the\n> rebase manpage:\n> \n> \"\"\"\n> Hooks\n> ~~~~~\n> \n> The apply backend has not traditionally called the post-commit hook,\n> while the merge backend has. However, this was by accident of\n> implementation rather than by design. Both backends should have the\n> same behavior, though it is not clear which one is correct.\n> \"\"\"\n> \n> to also mention post-checkout, note that the situation about which\n> backend calls this hook is reversed relative to post-commit, and again\n> mention it's not clear which backend is correct.\n> \n> My very rough opinion of the moment is:\n> * post-checkout should probably do as the manpage says, and only run\n> for checkout/switch plus new worktree situations (worktree add/clone).\n> rebase --apply should stop calling it.\n> * post-commit is horribly broken; it hardcodes an assumption of\n> exactly one commit. Also, it's a huge performance problem when things\n> want to create multiple commits. So, let's do as the githooks manpage\n> suggest and call this hook from a direct \"git commit\" -- and _only_\n> from there. Remove it from rebase, cherry-pick, revert, etc.\n> Normally, for consistency, I would say that merge should start calling\n> that hook (it only creates one commit, and \"git commit\" is used to\n> complete an interrupted merge anyway so why not uninterrupted ones),\n> but merge can be called by rebase --rebase-merges right now so that\n> pushes us back to the get it out of rebase side. The hook is kinda\n> broken anyway, so maybe limit exposure?\n> * Make a new post-batch-commit hook called by things that create new\n> commits, but only call it once per operation (e.g. rebase and\n> cherry-pick call it once rather than once per commit). Have commit,\n> merge, and revert all call this too. Not sure what to do about things\n> that change multiple refs simultaneously, e.g. fast-import (or things\n> like it, e.g. filter-branch).\n> \n> CC'ing Jonathan and Emily since they've thought a lot about hooks, and\n> whom I thought might be making changes in this area[1][2].\n> \n> \n> Elijah\n> \n> [1] https://lore.kernel.org/git/20200116203521.GA71299@google.com/\n> [2] https://lore.kernel.org/git/20200115215922.GI181522@google.com/\n> \n"}]}