* post-checkout hook aborts rebase @ 2020-08-26 23:10 Tom Rutherford 2020-08-27 0:13 ` Junio C Hamano 0 siblings, 1 reply; 16+ messages in thread From: Tom Rutherford @ 2020-08-26 23:10 UTC (permalink / raw) To: git Hi Git community, My team encountered an issue today which I believe is a bug, and I'm interested to know if those more familiar with the codebase and documentation agree with me. Here's a bug report. Thanks for your attention. Tom === Thank you for filling out a Git bug report! Please answer the following questions to help us understand your issue. What did you do before the bug happened? (Steps to reproduce your issue) 1. Have a post-checkout hook that returns a nonzero exit status. 2. `git pull --rebase` where there is at least one local commit to be rebased onto an incoming commit. (I assume any rebase would exhibit similar behavior, but this is how I encountered the issue). What did you expect to happen? (Expected behavior) The rebase succeeds. I expect this because the documentation for the post-checkout hook states, "This hook cannot affect the outcome of git switch or git checkout" https://www.git-scm.com/docs/githooks#_post_checkout What happened instead? (Actual behavior) `error: could not detach HEAD` is displayed, and I end up in a detached head state. What's different between what you expected and what actually happened? I do not expect the post-checkout hook to prevent rebase from succeeding. The documentation suggests this should succeed. It seems either the documentation is wrong, or git's use of the hook is in this case. Anything else you want to add: The issue is elegantly explained in this stack overflow answer: https://stackoverflow.com/a/25562688/1286571 It's not clear to me if this is a bug, or an omission from the documentation. I reproduced it on 2.28, but I haven't tried building from source. Please review the rest of the bug report below. You can delete any lines you don't wish to share. [System Info] git version: git version 2.28.0 cpu: x86_64 no commit associated with this build sizeof-long: 8 sizeof-size_t: 8 shell-path: /bin/sh uname: Linux 5.4.0-42-generic #46~18.04.1-Ubuntu SMP Fri Jul 10 07:21:24 UTC 2020 x86_64 compiler info: gnuc: 7.5 libc info: glibc: 2.27 $SHELL (typically, interactive shell): /usr/bin/fish [Enabled Hooks] pre-commit post-checkout ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: post-checkout hook aborts rebase 2020-08-26 23:10 post-checkout hook aborts rebase Tom Rutherford @ 2020-08-27 0:13 ` Junio C Hamano 2020-08-27 0:22 ` Junio C Hamano 0 siblings, 1 reply; 16+ messages in thread From: Junio C Hamano @ 2020-08-27 0:13 UTC (permalink / raw) To: Tom Rutherford; +Cc: git Tom Rutherford <tmrutherford@gmail.com> writes: > What did you expect to happen? (Expected behavior) > > The rebase succeeds. > > I expect this because the documentation for the post-checkout hook > states, "This hook cannot affect the outcome of git switch or git > checkout" https://www.git-scm.com/docs/githooks#_post_checkout In general, pre-<anything> hook is run before <anything> subcommand is executed, and by exiting with non-zero status, the hook can tell the <anything> subcommand not to proceed. post-<anything> hook, on the other hand, runs _after_ <anything> has done its thing already, so by definition, it cannot say "Hey, <anything>, don't continue". That is the primary meaning of "cannot affect" in that description. Without looking at the contents of the actual hook is, nobody can say anything definite, but it also means that "Your hook is NOT ALLOWED TO do any extra modification to files and the index 'git switch' or 'git checkout' made". If "git rebase" or whatever command wanted to place files and the index into some state by using "git checkout" command, and if the post-checkout hook mucked with the state in such a way that contradicts with what the "git rebase" command wanted them to be in, it is not surprising the hook's behavior broke "git rebase"'s operation. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: post-checkout hook aborts rebase 2020-08-27 0:13 ` Junio C Hamano @ 2020-08-27 0:22 ` Junio C Hamano 2020-08-27 0:44 ` Tom Rutherford 2020-08-27 16:27 ` Elijah Newren 0 siblings, 2 replies; 16+ messages in thread From: Junio C Hamano @ 2020-08-27 0:22 UTC (permalink / raw) To: Tom Rutherford; +Cc: git Junio C Hamano <gitster@pobox.com> writes: > ... If "git rebase" or whatever > command wanted to place files and the index into some state by using > "git checkout" command, and if the post-checkout hook mucked with > the state in such a way that contradicts with what the "git rebase" > command wanted them to be in, it is not surprising the hook's behavior > broke "git rebase"'s operation. Having said all that, I actually think that "rebase" shouldn't be invoking "git checkout" (and its equivalent) internally when switching to a specific version, in such a way that it would trigger any end-user specified hooks and allow them to muck with the working tree and the index state. I haven't checked the actual implementation of "git rebase" for quite some time to be sure, but we have lower-level plumbing commands that are not affected by the end-user hooks for exactly that kind of "build higher-level commands by synthesis of lower-level machinery", and it is very possible that what we are looking at is actually a bug that needs to be fixed. I dunno. Thanks. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: post-checkout hook aborts rebase 2020-08-27 0:22 ` Junio C Hamano @ 2020-08-27 0:44 ` Tom Rutherford 2020-08-27 5:44 ` Chris Torek 2020-09-09 9:43 ` Phillip Wood 2020-08-27 16:27 ` Elijah Newren 1 sibling, 2 replies; 16+ messages in thread From: Tom Rutherford @ 2020-08-27 0:44 UTC (permalink / raw) To: Junio C Hamano; +Cc: git Thank you for the response Junio. For what it's worth, my hook does not make changes to the repo. It's running a command to check that the installed version of our dependencies match the version specified in the commit being checked out, and merely warns if the two don't match (then exits with a nonzero return code). For this reason it's been convenient that the hook runs during rebases, but I find it surprising that the nonzero return code would impact the rebase. Tom On Wed, Aug 26, 2020 at 5:22 PM Junio C Hamano <gitster@pobox.com> wrote: > > Junio C Hamano <gitster@pobox.com> writes: > > > ... If "git rebase" or whatever > > command wanted to place files and the index into some state by using > > "git checkout" command, and if the post-checkout hook mucked with > > the state in such a way that contradicts with what the "git rebase" > > command wanted them to be in, it is not surprising the hook's behavior > > broke "git rebase"'s operation. > > Having said all that, I actually think that "rebase" shouldn't be > invoking "git checkout" (and its equivalent) internally when > switching to a specific version, in such a way that it would trigger > any end-user specified hooks and allow them to muck with the working > tree and the index state. > > I haven't checked the actual implementation of "git rebase" for > quite some time to be sure, but we have lower-level plumbing > commands that are not affected by the end-user hooks for exactly > that kind of "build higher-level commands by synthesis of > lower-level machinery", and it is very possible that what we are > looking at is actually a bug that needs to be fixed. I dunno. > > Thanks. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: post-checkout hook aborts rebase 2020-08-27 0:44 ` Tom Rutherford @ 2020-08-27 5:44 ` Chris Torek 2020-08-27 15:51 ` Junio C Hamano 2020-09-09 9:43 ` Phillip Wood 1 sibling, 1 reply; 16+ messages in thread From: Chris Torek @ 2020-08-27 5:44 UTC (permalink / raw) To: Tom Rutherford; +Cc: Junio C Hamano, Git List On Wed, Aug 26, 2020 at 5:48 PM Tom Rutherford <tmrutherford@gmail.com> wrote: > > Thank you for the response Junio. > > For what it's worth, my hook does not make changes to the repo. It's > running a command to check that the installed version of our > dependencies match the version specified in the commit being checked > out, and merely warns if the two don't match (then exits with a > nonzero return code). I've run into this before myself. The core of the bug is that when `git checkout` runs the post-checkout hook, whatever exit status that hook has, `git checkout` has the same exit status. This might be intended as a feature, but if so, the documentation needs a tweak: the githooks docs say in part This hook cannot affect the outcome of git checkout. If "outcome" includes exit status -- to me, it does -- either the docs are wrong or the code is wrong. > For this reason it's been convenient that the hook runs during > rebases, but I find it surprising that the nonzero return code would > impact the rebase. I have to agree with this, too. (The simplest fix would be to have `git checkout` ignore the status from the post-checkout hook, of course, and just exit 0 for success.) Chris ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: post-checkout hook aborts rebase 2020-08-27 5:44 ` Chris Torek @ 2020-08-27 15:51 ` Junio C Hamano 2020-08-27 19:04 ` Chris Torek 0 siblings, 1 reply; 16+ messages in thread From: Junio C Hamano @ 2020-08-27 15:51 UTC (permalink / raw) To: Chris Torek; +Cc: Tom Rutherford, Git List Chris Torek <chris.torek@gmail.com> writes: > This might be intended as a feature, but if so, the documentation > needs a tweak: the githooks docs say in part > > This hook cannot affect the outcome of git checkout. > > If "outcome" includes exit status -- to me, it does -- either the docs > are wrong or the code is wrong. I suspected that this was a case of adding the hook that just runs without affecting the exit code and then somebody later changed the behaviour (either deliberately or by mistake) and forgot to update the documentation. But it seems that ever since the hook support was introduced at 1abbe475 (post-checkout hook, tests, and docs, 2007-09-26), the command was made to exit with the same status from the hook. So I agree that depending on reader's view on "the outcome", this is documented incorrectly (if the exit code is part of "the outcome") or just fine (otherwise). Apparently, the author of the patch and the reviewers back then thought the latter. I still suspect that the checkout run, merely as an implementation detail of rebase (or any other git subcommand), should not trigger any hook, but before any such code change, at least let's update the documentation to clarify what we mean by "the outcome". Hopefully something like the below may be a good starting point? Documentation/githooks.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt index 31b601e4bc..cf95d6d02b 100644 --- a/Documentation/githooks.txt +++ b/Documentation/githooks.txt @@ -193,7 +193,9 @@ worktree. The hook is given three parameters: the ref of the previous HEAD, the ref of the new HEAD (which may or may not have changed), and a flag indicating whether the checkout was a branch checkout (changing branches, flag=1) or a file checkout (retrieving a file from the index, flag=0). -This hook cannot affect the outcome of `git switch` or `git checkout`. +This hook cannot affect the outcome of `git switch` or `git checkout`, +other than that the hook's exit status becomes the exit status of +these two commands. It is also run after linkgit:git-clone[1], unless the `--no-checkout` (`-n`) option is used. The first parameter given to the hook is the null-ref, the second the ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: post-checkout hook aborts rebase 2020-08-27 15:51 ` Junio C Hamano @ 2020-08-27 19:04 ` Chris Torek 2020-08-27 20:11 ` Elijah Newren 0 siblings, 1 reply; 16+ messages in thread From: Chris Torek @ 2020-08-27 19:04 UTC (permalink / raw) To: Junio C Hamano; +Cc: Tom Rutherford, Git List On Thu, Aug 27, 2020 at 8:51 AM Junio C Hamano <gitster@pobox.com> wrote: > I still suspect that the checkout run, merely as an implementation > detail of rebase (or any other git subcommand), should not trigger > any hook ... The *last* checkout from the finished rebase perhaps *should*, but other than that one, that seems logically correct. > but before any such code change, at least let's update the > documentation to clarify what we mean by "the outcome". > > Hopefully something like the below may be a good starting point? > > Documentation/githooks.txt | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt > index 31b601e4bc..cf95d6d02b 100644 > --- a/Documentation/githooks.txt > +++ b/Documentation/githooks.txt > @@ -193,7 +193,9 @@ worktree. The hook is given three parameters: the ref of the previous HEAD, > the ref of the new HEAD (which may or may not have changed), and a flag > indicating whether the checkout was a branch checkout (changing branches, > flag=1) or a file checkout (retrieving a file from the index, flag=0). > -This hook cannot affect the outcome of `git switch` or `git checkout`. > +This hook cannot affect the outcome of `git switch` or `git checkout`, > +other than that the hook's exit status becomes the exit status of > +these two commands. > > It is also run after linkgit:git-clone[1], unless the `--no-checkout` (`-n`) option is > used. The first parameter given to the hook is the null-ref, the second the This looks good to me, and can either be independent of, or the first part of, any rebase update. Chris ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: post-checkout hook aborts rebase 2020-08-27 19:04 ` Chris Torek @ 2020-08-27 20:11 ` Elijah Newren 2020-08-27 20:32 ` Chris Torek 0 siblings, 1 reply; 16+ messages in thread From: Elijah Newren @ 2020-08-27 20:11 UTC (permalink / raw) To: Chris Torek; +Cc: Junio C Hamano, Tom Rutherford, Git List On Thu, Aug 27, 2020 at 12:07 PM Chris Torek <chris.torek@gmail.com> wrote: > > On Thu, Aug 27, 2020 at 8:51 AM Junio C Hamano <gitster@pobox.com> wrote: > > I still suspect that the checkout run, merely as an implementation > > detail of rebase (or any other git subcommand), should not trigger > > any hook ... > > The *last* checkout from the finished rebase perhaps *should*, but > other than that one, that seems logically correct. What do you mean by the "last checkout"? I believe a typical non-interactive rebase of N patches has only one checkout, and I don't see why running hooks for the starting point is relevant. If hooks are wanted for rebase, I'd still want to have rebase-specific ones, because most people who think of "checkout hooks" or "commit hooks" probably aren't going to think of them the way rebase or cherry-pick happen to use them. (And that might even be more true for --interactive and --rebase-merges cases.) > > but before any such code change, at least let's update the > > documentation to clarify what we mean by "the outcome". > > > > Hopefully something like the below may be a good starting point? > > > > Documentation/githooks.txt | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt > > index 31b601e4bc..cf95d6d02b 100644 > > --- a/Documentation/githooks.txt > > +++ b/Documentation/githooks.txt > > @@ -193,7 +193,9 @@ worktree. The hook is given three parameters: the ref of the previous HEAD, > > the ref of the new HEAD (which may or may not have changed), and a flag > > indicating whether the checkout was a branch checkout (changing branches, > > flag=1) or a file checkout (retrieving a file from the index, flag=0). > > -This hook cannot affect the outcome of `git switch` or `git checkout`. > > +This hook cannot affect the outcome of `git switch` or `git checkout`, > > +other than that the hook's exit status becomes the exit status of > > +these two commands. > > > > It is also run after linkgit:git-clone[1], unless the `--no-checkout` (`-n`) option is > > used. The first parameter given to the hook is the null-ref, the second the > > This looks good to me, and can either be independent of, or the first part of, > any rebase update. Patch looks good to me too. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: post-checkout hook aborts rebase 2020-08-27 20:11 ` Elijah Newren @ 2020-08-27 20:32 ` Chris Torek 0 siblings, 0 replies; 16+ messages in thread From: Chris Torek @ 2020-08-27 20:32 UTC (permalink / raw) To: Elijah Newren; +Cc: Junio C Hamano, Tom Rutherford, Git List > On Thu, Aug 27, 2020 at 12:07 PM Chris Torek <chris.torek@gmail.com> wrote: > > The *last* checkout from the finished rebase perhaps *should* [invoke a hook] On Thu, Aug 27, 2020 at 1:11 PM Elijah Newren <newren@gmail.com> wrote: > What do you mean by the "last checkout"? I believe a typical > non-interactive rebase of N patches has only one checkout, and I don't > see why running hooks for the starting point is relevant. I meant for the final state after rewriting the commits -- but that's actually the post-rewrite hook, not the post-checkout hook. I had them conflated mentally when I wrote the above. > If hooks are wanted for rebase, I'd still want to have rebase-specific > ones, because most people who think of "checkout hooks" or "commit > hooks" probably aren't going to think of them the way rebase or > cherry-pick happen to use them. (And that might even be more true for > --interactive and --rebase-merges cases.) I agree with this. Chris ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: post-checkout hook aborts rebase 2020-08-27 0:44 ` Tom Rutherford 2020-08-27 5:44 ` Chris Torek @ 2020-09-09 9:43 ` Phillip Wood 2020-09-09 16:07 ` Tom Rutherford 2020-09-11 20:25 ` Philippe Blain 1 sibling, 2 replies; 16+ messages in thread From: Phillip Wood @ 2020-09-09 9:43 UTC (permalink / raw) To: Tom Rutherford, Junio C Hamano; +Cc: git Hi Tom On 27/08/2020 01:44, Tom Rutherford wrote: > Thank you for the response Junio. > > For what it's worth, my hook does not make changes to the repo. It's > running a command to check that the installed version of our > dependencies match the version specified in the commit being checked > out, and merely warns if the two don't match (then exits with a > nonzero return code). > > For this reason it's been convenient that the hook runs during > rebases, but I find it surprising that the nonzero return code would > impact the rebase. If the checkout succeeds that rebase does not print any of checkout's output so unfortunately you wouldn't see the message from your hook. I tend to agree with Junio that we shouldn't be running the post-checkout hook when rebasing. Best Wishes Phillip > > Tom > > On Wed, Aug 26, 2020 at 5:22 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> Junio C Hamano <gitster@pobox.com> writes: >> >>> ... If "git rebase" or whatever >>> command wanted to place files and the index into some state by using >>> "git checkout" command, and if the post-checkout hook mucked with >>> the state in such a way that contradicts with what the "git rebase" >>> command wanted them to be in, it is not surprising the hook's behavior >>> broke "git rebase"'s operation. >> >> Having said all that, I actually think that "rebase" shouldn't be >> invoking "git checkout" (and its equivalent) internally when >> switching to a specific version, in such a way that it would trigger >> any end-user specified hooks and allow them to muck with the working >> tree and the index state. >> >> I haven't checked the actual implementation of "git rebase" for >> quite some time to be sure, but we have lower-level plumbing >> commands that are not affected by the end-user hooks for exactly >> that kind of "build higher-level commands by synthesis of >> lower-level machinery", and it is very possible that what we are >> looking at is actually a bug that needs to be fixed. I dunno. >> >> Thanks. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: post-checkout hook aborts rebase 2020-09-09 9:43 ` Phillip Wood @ 2020-09-09 16:07 ` Tom Rutherford 2020-09-11 20:25 ` Philippe Blain 1 sibling, 0 replies; 16+ messages in thread From: Tom Rutherford @ 2020-09-09 16:07 UTC (permalink / raw) To: Phillip Wood; +Cc: Junio C Hamano, git Thanks Phillip, Yes, I've now noticed that's the case. :) I also didn't intend to suggest that post-checkout *should* run during rebases, just that it would be convenient for this particular use case if it did. I think I'll need to use multiple hooks to accomplish what I want to do. Tom On Wed, Sep 9, 2020 at 2:44 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > > Hi Tom > > On 27/08/2020 01:44, Tom Rutherford wrote: > > Thank you for the response Junio. > > > > For what it's worth, my hook does not make changes to the repo. It's > > running a command to check that the installed version of our > > dependencies match the version specified in the commit being checked > > out, and merely warns if the two don't match (then exits with a > > nonzero return code). > > > > For this reason it's been convenient that the hook runs during > > rebases, but I find it surprising that the nonzero return code would > > impact the rebase. > > If the checkout succeeds that rebase does not print any of checkout's > output so unfortunately you wouldn't see the message from your hook. > > I tend to agree with Junio that we shouldn't be running the > post-checkout hook when rebasing. > > Best Wishes > > Phillip > > > > > Tom > > > > On Wed, Aug 26, 2020 at 5:22 PM Junio C Hamano <gitster@pobox.com> wrote: > >> > >> Junio C Hamano <gitster@pobox.com> writes: > >> > >>> ... If "git rebase" or whatever > >>> command wanted to place files and the index into some state by using > >>> "git checkout" command, and if the post-checkout hook mucked with > >>> the state in such a way that contradicts with what the "git rebase" > >>> command wanted them to be in, it is not surprising the hook's behavior > >>> broke "git rebase"'s operation. > >> > >> Having said all that, I actually think that "rebase" shouldn't be > >> invoking "git checkout" (and its equivalent) internally when > >> switching to a specific version, in such a way that it would trigger > >> any end-user specified hooks and allow them to muck with the working > >> tree and the index state. > >> > >> I haven't checked the actual implementation of "git rebase" for > >> quite some time to be sure, but we have lower-level plumbing > >> commands that are not affected by the end-user hooks for exactly > >> that kind of "build higher-level commands by synthesis of > >> lower-level machinery", and it is very possible that what we are > >> looking at is actually a bug that needs to be fixed. I dunno. > >> > >> Thanks. > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: post-checkout hook aborts rebase 2020-09-09 9:43 ` Phillip Wood 2020-09-09 16:07 ` Tom Rutherford @ 2020-09-11 20:25 ` Philippe Blain 2020-09-12 0:51 ` Junio C Hamano 1 sibling, 1 reply; 16+ messages in thread From: Philippe Blain @ 2020-09-11 20:25 UTC (permalink / raw) To: Phillip Wood Cc: Tom Rutherford, Junio C Hamano, Git mailing list, Elijah Newren, chris.torek Hello everyone, > Le 9 sept. 2020 à 05:43, Phillip Wood <phillip.wood123@gmail.com> a écrit : > > Hi Tom > > On 27/08/2020 01:44, Tom Rutherford wrote: >> Thank you for the response Junio. >> >> For what it's worth, my hook does not make changes to the repo. It's >> running a command to check that the installed version of our >> dependencies match the version specified in the commit being checked >> out, and merely warns if the two don't match (then exits with a >> nonzero return code). >> >> For this reason it's been convenient that the hook runs during >> rebases, but I find it surprising that the nonzero return code would >> impact the rebase. > > If the checkout succeeds that rebase does not print any of checkout's > output so unfortunately you wouldn't see the message from your hook. According to git-rebase's doc [1], that's true only for the new "merge" backend, though, (which would be in use here in 2.28 unless 'rebase.backend' is set to 'apply'). On a slightly related note, I noticed that that part of the documentation is not 100% exact. I states: "Further, both backends only call the post-checkout hook with the starting point commit of the rebase, not the intermediate commits nor the final commit." but I noticed that in an interactive rebase, the post-checkout hook *is* called for commits marked as "edit", when the rebase stops to edit them (I haven't checked if it's called when the rebase stops for other reasons, like conflicts.) Maybe we should tweak the wording of the doc ? Cheers, Philippe. [1] https://git-scm.com/docs/git-rebase#_hooks ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: post-checkout hook aborts rebase 2020-09-11 20:25 ` Philippe Blain @ 2020-09-12 0:51 ` Junio C Hamano 0 siblings, 0 replies; 16+ messages in thread From: Junio C Hamano @ 2020-09-12 0:51 UTC (permalink / raw) To: Philippe Blain Cc: Phillip Wood, Tom Rutherford, Git mailing list, Elijah Newren, chris.torek Philippe Blain <levraiphilippeblain@gmail.com> writes: > On a slightly related note, I noticed that that part of the documentation is not 100% exact. I states: > "Further, both backends only call the post-checkout hook with the starting point commit of the rebase, > not the intermediate commits nor the final commit." > > but I noticed that in an interactive rebase, the post-checkout hook *is* called for commits > marked as "edit", when the rebase stops to edit them (I haven't checked if it's called when the rebase stops > for other reasons, like conflicts.) > > Maybe we should tweak the wording of the doc ? Yeah, we should do either that, or fix the code and make it not to call the post-checkout hook so aggressively, as suggested during the discussion. Thanks. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: post-checkout hook aborts rebase 2020-08-27 0:22 ` Junio C Hamano 2020-08-27 0:44 ` Tom Rutherford @ 2020-08-27 16:27 ` Elijah Newren 2020-08-27 17:27 ` Junio C Hamano 1 sibling, 1 reply; 16+ messages in thread From: Elijah Newren @ 2020-08-27 16:27 UTC (permalink / raw) To: Junio C Hamano; +Cc: Tom Rutherford, Git Mailing List On Wed, Aug 26, 2020 at 5:24 PM Junio C Hamano <gitster@pobox.com> wrote: > > Junio C Hamano <gitster@pobox.com> writes: > > > ... If "git rebase" or whatever > > command wanted to place files and the index into some state by using > > "git checkout" command, and if the post-checkout hook mucked with > > the state in such a way that contradicts with what the "git rebase" > > command wanted them to be in, it is not surprising the hook's behavior > > broke "git rebase"'s operation. > > Having said all that, I actually think that "rebase" shouldn't be > invoking "git checkout" (and its equivalent) internally when > switching to a specific version, in such a way that it would trigger > any end-user specified hooks and allow them to muck with the working > tree and the index state. > > I haven't checked the actual implementation of "git rebase" for > quite some time to be sure, but we have lower-level plumbing > commands that are not affected by the end-user hooks for exactly > that kind of "build higher-level commands by synthesis of > lower-level machinery", and it is very possible that what we are > looking at is actually a bug that needs to be fixed. I dunno. > > Thanks. Yes, and I think we should also make rebase stop invoking "git commit" too. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: post-checkout hook aborts rebase 2020-08-27 16:27 ` Elijah Newren @ 2020-08-27 17:27 ` Junio C Hamano 2020-08-27 17:47 ` Elijah Newren 0 siblings, 1 reply; 16+ messages in thread From: Junio C Hamano @ 2020-08-27 17:27 UTC (permalink / raw) To: Elijah Newren; +Cc: Tom Rutherford, Git Mailing List Elijah Newren <newren@gmail.com> writes: > On Wed, Aug 26, 2020 at 5:24 PM Junio C Hamano <gitster@pobox.com> wrote: >> >> Junio C Hamano <gitster@pobox.com> writes: >> >> > ... If "git rebase" or whatever >> > command wanted to place files and the index into some state by using >> > "git checkout" command, and if the post-checkout hook mucked with >> > the state in such a way that contradicts with what the "git rebase" >> > command wanted them to be in, it is not surprising the hook's behavior >> > broke "git rebase"'s operation. >> >> Having said all that, I actually think that "rebase" shouldn't be >> invoking "git checkout" (and its equivalent) internally when >> switching to a specific version, in such a way that it would trigger >> any end-user specified hooks and allow them to muck with the working >> tree and the index state. >> >> I haven't checked the actual implementation of "git rebase" for >> quite some time to be sure, but we have lower-level plumbing >> commands that are not affected by the end-user hooks for exactly >> that kind of "build higher-level commands by synthesis of >> lower-level machinery", and it is very possible that what we are >> looking at is actually a bug that needs to be fixed. I dunno. >> >> Thanks. > > Yes, and I think we should also make rebase stop invoking "git commit" too. Note that I didn't say we should make it stop invoking "git checkout". We could invent a mechanism that disables all the hook invocations and other customizations [*1*] (done e.g. via the configuration variables) for internal use of the Porcelain commands, and use it when "rebase" invokes Porcelains like "checkout", "commit" as its implementation detail, for example (some "invocations" I think bypass the run_command() inteface and instead done by directly calling the implementation detail of "checkout" and "commit", but the principles are the same). [Footnote] *1* of course, it becomes a balancing act to decide what kind of customizations are OK to honor under such a mode. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: post-checkout hook aborts rebase 2020-08-27 17:27 ` Junio C Hamano @ 2020-08-27 17:47 ` Elijah Newren 0 siblings, 0 replies; 16+ messages in thread From: Elijah Newren @ 2020-08-27 17:47 UTC (permalink / raw) To: Junio C Hamano; +Cc: Tom Rutherford, Git Mailing List On Thu, Aug 27, 2020 at 10:28 AM Junio C Hamano <gitster@pobox.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > > On Wed, Aug 26, 2020 at 5:24 PM Junio C Hamano <gitster@pobox.com> wrote: > >> > >> Junio C Hamano <gitster@pobox.com> writes: > >> > >> > ... If "git rebase" or whatever > >> > command wanted to place files and the index into some state by using > >> > "git checkout" command, and if the post-checkout hook mucked with > >> > the state in such a way that contradicts with what the "git rebase" > >> > command wanted them to be in, it is not surprising the hook's behavior > >> > broke "git rebase"'s operation. > >> > >> Having said all that, I actually think that "rebase" shouldn't be > >> invoking "git checkout" (and its equivalent) internally when > >> switching to a specific version, in such a way that it would trigger > >> any end-user specified hooks and allow them to muck with the working > >> tree and the index state. > >> > >> I haven't checked the actual implementation of "git rebase" for > >> quite some time to be sure, but we have lower-level plumbing > >> commands that are not affected by the end-user hooks for exactly > >> that kind of "build higher-level commands by synthesis of > >> lower-level machinery", and it is very possible that what we are > >> looking at is actually a bug that needs to be fixed. I dunno. > >> > >> Thanks. > > > > Yes, and I think we should also make rebase stop invoking "git commit" too. > > Note that I didn't say we should make it stop invoking "git > checkout". Understood that you didn't say that, but I am of the opinion that we should do that. Invoking "git checkout" and "git commit" were convenient implementation details when rebase was written as a script. When it was rewritten in C, forking out to these processes made for an easy conversion path (even if slightly ugly). But forking other processes is costly, it has given us multiple reports of unwanted side-effects from hooks[1], it makes the code more difficult to debug in a debugger, etc. I think these are all problems we could avoid by no longer calling these external commands. > We could invent a mechanism that disables all the hook invocations > and other customizations [*1*] (done e.g. via the configuration > variables) for internal use of the Porcelain commands, and use it > when "rebase" invokes Porcelains like "checkout", "commit" as its > implementation detail, for example (some "invocations" I think > bypass the run_command() inteface and instead done by directly > calling the implementation detail of "checkout" and "commit", but > the principles are the same). > > [Footnote] > > *1* of course, it becomes a balancing act to decide what kind of > customizations are OK to honor under such a mode. That'd be one way to solve it, but it feels like it'd push maintenance burden onto future folks that people touching the commit and checkout builtins have to be aware of what other commands are using them under the covers and tweak them appropriately. I'd rather checkout and commit just shared the relevant bits of important code with sequencer, and then if people want to add configuration bits (be that hooks or config settings or whatever), then it only gets added to the commands that people explicitly add them to, rather than them getting added to rebase via accident. ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-09-12 0:52 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-26 23:10 post-checkout hook aborts rebase Tom Rutherford 2020-08-27 0:13 ` Junio C Hamano 2020-08-27 0:22 ` Junio C Hamano 2020-08-27 0:44 ` Tom Rutherford 2020-08-27 5:44 ` Chris Torek 2020-08-27 15:51 ` Junio C Hamano 2020-08-27 19:04 ` Chris Torek 2020-08-27 20:11 ` Elijah Newren 2020-08-27 20:32 ` Chris Torek 2020-09-09 9:43 ` Phillip Wood 2020-09-09 16:07 ` Tom Rutherford 2020-09-11 20:25 ` Philippe Blain 2020-09-12 0:51 ` Junio C Hamano 2020-08-27 16:27 ` Elijah Newren 2020-08-27 17:27 ` Junio C Hamano 2020-08-27 17:47 ` Elijah Newren
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).