{"thread":{"id":"60647","subject":"rebase invoking pre-commit","startedAt":"2023-12-21T20:59:05Z","lastAt":"2024-01-05T16:26:09Z","messageCount":8,"participants":["Sean Allred","Phillip Wood","Elijah Newren","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"485953","messageId":"m0sf3vi86g.fsf@epic96565.epic.com","threadId":"60647","inReplyTo":null,"subject":"rebase invoking pre-commit","fromName":"Sean Allred","fromEmail":"allred.sean@gmail.com","sentAt":"2023-12-21T20:58:35Z","receivedAt":"2023-12-21T20:59:05Z","isPatch":false,"sender":{"key":"allred.sean@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2082195?v=4"},"body":"Is there a current reason why pre-commit shouldn't be invoked during\nrebase, or is this just waiting for a reviewable patch?\n\nThis was brought up before at [1] in 2015, but that thread so old at\nthis point that it seemed prudent to double-check before investing time\nin a developing and testing a patch.\n\n[1]: https://lore.kernel.org/git/1m55i3m.1fum4zo1fpnhncM%25lists@haller-berlin.de/\n\n--\nSean Allred\n"},{"id":"485971","messageId":"bf1ce173-50d7-405f-88c1-7edb7ec5a55a@gmail.com","threadId":"60647","inReplyTo":"m0sf3vi86g.fsf@epic96565.epic.com","subject":"Re: rebase invoking pre-commit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-12-22T10:05:56Z","receivedAt":"2023-12-22T10:06:02Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Sean\n\nOn 21/12/2023 20:58, Sean Allred wrote:\n> Is there a current reason why pre-commit shouldn't be invoked during\n> rebase, or is this just waiting for a reviewable patch?\n\nThe reason that we don't run the pre-commit hook is that the commit \nbeing rebased may have been created with \"git commit --no-verify\" and so \nrunning the pre-commit hook would stop it from being rebased - see \ne637122ef2 (rebase -m: do not trigger pre-commit verification, 2008-03-16).\n\nI think that most of the time it would be valuable to run the pre-commit \nhook when committing a conflict resolution but we'd need to add \nsomething like \"git rebase --continue --no-verify\" as a way to bypass it \nwhen resolving conflicts in commits that were created with \"git commit \n--no-verify\".\n\nBest Wishes\n\nPhillip\n\n> This was brought up before at [1] in 2015, but that thread so old at\n> this point that it seemed prudent to double-check before investing time\n> in a developing and testing a patch.\n> \n> [1]: https://lore.kernel.org/git/1m55i3m.1fum4zo1fpnhncM%25lists@haller-berlin.de/\n> \n> --\n> Sean Allred\n> \n"},{"id":"485985","messageId":"m01qbdx561.fsf@epic96565.epic.com","threadId":"60647","inReplyTo":"bf1ce173-50d7-405f-88c1-7edb7ec5a55a@gmail.com","subject":"Re: rebase invoking pre-commit","fromName":"Sean Allred","fromEmail":"allred.sean@gmail.com","sentAt":"2023-12-22T22:05:20Z","receivedAt":"2023-12-22T22:07:21Z","isPatch":false,"sender":{"key":"allred.sean@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2082195?v=4"},"body":"\nPhillip Wood <phillip.wood123@gmail.com> writes:\n> [...], we'd need to add something like \"git rebase --continue\n> --no-verify\" as a way to bypass it when resolving conflicts in commits\n> that were created with \"git commit --no-verify\".\n\nLooks like such a flag already exists to skip the 'pre-rebase' hook.\nPresumably it can pull double-duty to skip pre-commit as well.\n\n(It's a little messy to me to not be able to pick/choose which hooks to\nskip, but looks like this is already status quo with other hooks.)\n\n--\nSean Allred\n"},{"id":"486004","messageId":"CABPp-BFbvRDCbMp9Gs9PuV7WfgoVNwyOOn1rB7fe_8UvEEdehA@mail.gmail.com","threadId":"60647","inReplyTo":"m0sf3vi86g.fsf@epic96565.epic.com","subject":"Re: rebase invoking pre-commit","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-12-23T21:37:40Z","receivedAt":"2023-12-23T21:37:52Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Dec 21, 2023 at 12:59 PM Sean Allred <allred.sean@gmail.com> wrote:\n>\n> Is there a current reason why pre-commit shouldn't be invoked during\n> rebase, or is this just waiting for a reviewable patch?\n>\n> This was brought up before at [1] in 2015, but that thread so old at\n> this point that it seemed prudent to double-check before investing time\n> in a developing and testing a patch.\n>\n> [1]: https://lore.kernel.org/git/1m55i3m.1fum4zo1fpnhncM%25lists@haller-berlin.de/\n\nI'm very opinionated here.  I'm just one person, so definitely take\nthis with a grain of salt, but in my view...\n\nPersonally, I think implementing any per-commit hook in rebase by\ndefault is a mistake.  It enforces a\nmust-be-in-a-worktree-and-the-worktree-must-be-updated-with-every-replayed-commit\nmindset, which I find highly problematic[2], even if that's \"what we\nalways used to do\".  Because of that, I would prefer to see this at\nmost be a command line flag.  However, we've already got a command\nline flag that while not identical, is essentially equivalent: \"--exec\n$MY_SCRIPT\" (it's not the same because it's a post-commit check, but\nyou get notification of any problematic commits, and an immediate stop\nof the rebase for you to fix up the problematic commit; fixing up the\ncommit shouldn't be problematic since you are, after all, already\nrebasing).\n\nI see Phillip already responded and suggested not running the\npre-commit hook with every commit, but only upon the first commit\nafter a \"git rebase --continue\".  That seems far more reasonable to me\nthan running on every commit...though even that worries me in regards\nto what assumptions that entails about what is present in the working\ntree.  (For example, what about folks with large repositories, so\nlarge that a branch switch or full checkout is extremely costly, and\nwhich could benefit from resolving conflicts in a separate\nsparse-checkout worktree, potentially much more sparse than their main\ncheckout?  And what if people like that really fast rebase resolution\n(namely, done in a separate very sparse checkout which also has the\nadvantage of not polluting your current working tree) so much that\nthey use it on smaller repositories as well?  Can I not even\nexperiment with this idea because of the historical\nper-commit-at-least-as-full-as-main-worktree-checkout assumptions\nwe've baked into rebase?)\n\nWhile at it, I should also mention that I'm not a fan of the broken\npre-rebase hook; its design ties us to doing a single branch at a\ntime.  Maybe that hook is not quite as bad, though, since we already\nbroke that hook and no one seemed to care (in particular, when\n--update-refs was implemented).  But if no one seems to care about\nbroken hooks, I think the proper answer is to either get rid of the\nhook or fix it.\n\nAnyway, as I mentioned, I'm quite opinionated here.  To the point that\nI deemed git-rebase in its current form to possibly be unfixable\n(after first putting a lot of work into improving it over the past\nyears) and eventually introduced a new \"git-replay\" command which I\nhope to make into a competent replacement for it.  Given that I do\nhave another command to do my experiments, others on the list may\nthink it's fine to send rebase further down this route, but I was\nhoping to avoid further drift so that there might be some way of\nre-implementing rebase on top of my \"git-replay\" ideas/design.\n\nJust my $0.02,\nElijah\n\n[2] https://lore.kernel.org/git/20231124111044.3426007-1-christian.couder@gmail.com/\n"},{"id":"486025","messageId":"xmqqa5pwkjp3.fsf@gitster.g","threadId":"60647","inReplyTo":"bf1ce173-50d7-405f-88c1-7edb7ec5a55a@gmail.com","subject":"Re: rebase invoking pre-commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-12-26T16:33:12Z","receivedAt":"2023-12-26T16:33:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> On 21/12/2023 20:58, Sean Allred wrote:\n>> Is there a current reason why pre-commit shouldn't be invoked during\n>> rebase, or is this just waiting for a reviewable patch?\n>\n> The reason that we don't run the pre-commit hook is that the commit\n> being rebased may have been created with \"git commit --no-verify\" and\n> so running the pre-commit hook would stop it from being rebased - see\n> e637122ef2 (rebase -m: do not trigger pre-commit verification,\n> 2008-03-16).\n\nVery true.  And back then we didn't have \"rebase -x\" mechanism but\nthese days, anybody who is interested in running a command between\neach step can use it to run any validation script, not the one with\nfixed name called \"hooks\", so I'd place this to fairly low priority.\n\nThanks.\n"},{"id":"486176","messageId":"m0sf3itvpy.fsf@epic96565.epic.com","threadId":"60647","inReplyTo":"CABPp-BFbvRDCbMp9Gs9PuV7WfgoVNwyOOn1rB7fe_8UvEEdehA@mail.gmail.com","subject":"Re: rebase invoking pre-commit","fromName":"Sean Allred","fromEmail":"allred.sean@gmail.com","sentAt":"2023-12-31T10:52:00Z","receivedAt":"2023-12-31T12:14:37Z","isPatch":false,"sender":{"key":"allred.sean@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2082195?v=4"},"body":"\nElijah Newren <newren@gmail.com> writes:\n\n> On Thu, Dec 21, 2023 at 12:59 PM Sean Allred <allred.sean@gmail.com> wrote:\n>> Is there a current reason why pre-commit shouldn't be invoked during\n>> rebase, or is this just waiting for a reviewable patch?\n>>\n>> This was brought up before at [1] in 2015, but that thread so old at\n>> this point that it seemed prudent to double-check before investing time\n>> in a developing and testing a patch.\n>>\n>> [1]: https://lore.kernel.org/git/1m55i3m.1fum4zo1fpnhncM%25lists@haller-berlin.de/\n>\n> I'm very opinionated here.  I'm just one person, so definitely take\n> this with a grain of salt, but in my view...\n>\n> Personally, I think implementing any per-commit hook in rebase by\n> default is a mistake. It enforces a must-be-in-a-worktree-and-the-\n> worktree-must-be-updated-with-every-replayed-commit mindset, which I\n> find highly problematic[2], even if that's \"what we always used to\n> do\".\n>\n> [2] https://lore.kernel.org/git/20231124111044.3426007-1-christian.couder@gmail.com/\n\nI'm not hip with what most pre-commit hooks do, but I'll point out that\na hook like pre-commit assuming there is a worktree is the fault of the\nhook implementation, not of the infrastructure that invokes the hook. I\nimagine most folks on this list are aware that a worktree is not needed\nto create a commit and update a branch to point at it.\n\nFWIW, I would also find such a mindset to be highly problematic :-) I'll\ntake a moment here to thank you, Christian, and everyone else in that\neffort for your interest in and work on git-replay; I've been trying to\nwatch its activity on-list closely in the hopes that we can adopt it\ninto our system once it's ready.\n\n> Because of that, I would prefer to see this at most be a command line\n> flag. However, we've already got a command line flag that while not\n> identical, is essentially equivalent: \"--exec $MY_SCRIPT\" (it's not\n> the same because it's a post-commit check, but you get notification of\n> any problematic commits, and an immediate stop of the rebase for you\n> to fix up the problematic commit; fixing up the commit shouldn't be\n> problematic since you are, after all, already rebasing).\n\nIndeed, and an\n\n    --exec 'git hook run pre-commit || git reset --soft HEAD~'\n\nwould probably get you farther. I can certainly see an argument for\nthis, but from the perspective of designing a system for other\ndevelopers to use, such a rebase would have to be triggered\nautomatically (perhaps on pre-push).\n\n> I see Phillip already responded and suggested not running the\n> pre-commit hook with every commit, but only upon the first commit\n> after a \"git rebase --continue\".  That seems far more reasonable to me\n> than running on every commit...though even that worries me in regards\n> to what assumptions that entails about what is present in the working\n> tree.\n\nIt's worth noting the context here is to prevent developers from\ncommitting conflict markers, so this would actually be exactly\nsufficient.\n\nInvoking pre-commit at this time would also be consistent with the\nbehaviors of prepare-commit-msg, commit-msg, and post-commit -- at least\nwhen I reword a commit during a rebase.\n\nHowever, post-commit is executed after each picked commit during a\nrebase, so pre-commit there would also be consistent.\n\n> (For example, what about folks with large repositories, so large that\n> a branch switch or full checkout is extremely costly, and which could\n> benefit from resolving conflicts in a separate sparse-checkout\n> worktree, potentially much more sparse than their main checkout?\n\nAs it happens, a single checkout of our source runs upwards of 2GB, so\nI'm exactly in the population you're describing :-) The main reason\nwe're moving to Git from SVN is that an SVN checkout can take upwards of\nan hour for us today -- even with some real shenanigans to make them go\nfaster. On the Git side, we've also looked into (though I don't recall\nif we had much success with) narrowing the sparsity patterns to just the\nconflicts for conflict resolution workflows -- particularly when moving\nfeature code between separate trunks. So I guess I'm also glad we\nweren't too far off in left field on that one! (As I recall, one of the\nmain challenges we faced there was ensuring there was enough stuff\n'still around' so that both binary and project references could resolve\nand folks could use that information to help resolve conflicts.\nHopefully git-replay can be smart enough to allow some customization on\nthat front. We found some success with feeding the list of conflicted\nfiles into some arbitrary logic that spat out the sparsity pattern to\nuse.)\n\n> And what if people like that really fast rebase resolution (namely,\n> done in a separate very sparse checkout which also has the advantage\n> of not polluting your current working tree) so much that they use it\n> on smaller repositories as well? Can I not even experiment with this\n> idea because of the historical per-commit-at-least-as-full-as-main\n> -worktree-checkout assumptions we've baked into rebase?)\n\nI'd be interested in reading more about this baked-in assumption. Are\nthese mostly laid out in replay-design-notes.txt[3]?\n\n> While at it, I should also mention that I'm not a fan of the broken\n> pre-rebase hook; its design ties us to doing a single branch at a\n> time.  Maybe that hook is not quite as bad, though, since we already\n> broke that hook and no one seemed to care (in particular, when\n> --update-refs was implemented).  But if no one seems to care about\n> broken hooks, I think the proper answer is to either get rid of the\n> hook or fix it.\n\nIf I were to guess, this likely stems either from an inexact definition\nof the hook in documentation (ultimately resulting in incomplete tests)\nor folks incorrectly assuming what each hook should do based purely on\nits name.\n\nWhich leads to an interesting point: pre-commit specifically states that\nit is invoked by git-commit -- not that it's invoked whenever a commit\nis created. So perhaps the correct thing to do here (if a hook is in\nfact needed) would be to define a new hook -- but I worry about doing\nthat in the current state where there doesn't *seem* to be very rigid\ncoordination of when client hooks are invoked in terms of plumbing\nrather than porcelain.\n\n> Anyway, as I mentioned, I'm quite opinionated here.  To the point that\n> I deemed git-rebase in its current form to possibly be unfixable\n> (after first putting a lot of work into improving it over the past\n> years) and eventually introduced a new \"git-replay\" command which I\n> hope to make into a competent replacement for it.  Given that I do\n> have another command to do my experiments, others on the list may\n> think it's fine to send rebase further down this route, but I was\n> hoping to avoid further drift so that there might be some way of\n> re-implementing rebase on top of my \"git-replay\" ideas/design.\n\nI appreciate your perspective; you've certainly thought a lot about this\nspace -- and I definitely share your goal of consolidating\nimplementations for obvious reasons.\n\nSo I suppose that leaves me with four possible paths forward:\n\n1. Pursue invoking pre-commit before each commit in `git rebase` (likely\n   generic in the sequencer) to be consistent with post-commit.\n\n   It sounds like this isn't a popular option, but I'm curious to folks'\n   thoughts on the noted behavior of post-commit here.\n\n2. Pursue invoking pre-commit on `git rebase --continue` (likely on any\n   --continue in the sequencer). This has the benefit of using existing\n   configuration on developer machines to purportedly 'do the right\n   thing' when its likely humans are touching code during conflict\n   resolution. It's worth noting this isn't the only reason you might\n   --continue, though, since the naive interpretation of this approach\n   completely ignores sequencer commands like 'break', though it could\n   probably just do what commit-msg does.\n\n3. Define and implement a new hook that is called whenever a new commit\n   is about to be (or has been?) written. Such a hook could be\n   specifically designed to discourage assuming there's a working copy,\n   though we're kidding nobody by thinking it won't be used downstream\n   with that assumption. At least we could be explicit about\n   expectations, though.\n\n   This is *probably* a lot more design work than this little paragraph\n   lets on, but I've not personally watched the introduction of a new\n   hook so I don't have context for what to expect.\n\n4. Trigger a rebase --exec in our pre-push. This is certainly the least\n   work in git.git (i.e., no work at all), but it comes with the\n   distinct disadvantage of playing whiplash with the developer's focus.\n   During conflict resolution, they're thinking about conflicts. When\n   you're ready to push, its likely that you're no longer thinking about\n   conflicts.\n\nDoes the behavior of post-commit here change any minds?\n\n[3]: https://github.com/newren/git/blob/2a621020863c0b867293e020fec0954b43818789/replay-design-notes.txt#L162\n\n--\nSean Allred\n"},{"id":"486337","messageId":"CABPp-BFpdQ-uSMgOWdRxbPmVKZd1TzaqKVzcD1gnRb4usWo3iA@mail.gmail.com","threadId":"60647","inReplyTo":"m0sf3itvpy.fsf@epic96565.epic.com","subject":"Re: rebase invoking pre-commit","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-01-05T04:59:28Z","receivedAt":"2024-01-05T04:59:43Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Sun, Dec 31, 2023 at 4:14 AM Sean Allred <allred.sean@gmail.com> wrote:\n>\n> Elijah Newren <newren@gmail.com> writes:\n>\n> > On Thu, Dec 21, 2023 at 12:59 PM Sean Allred <allred.sean@gmail.com> wrote:\n> >> Is there a current reason why pre-commit shouldn't be invoked during\n> >> rebase, or is this just waiting for a reviewable patch?\n> >>\n> >> This was brought up before at [1] in 2015, but that thread so old at\n> >> this point that it seemed prudent to double-check before investing time\n> >> in a developing and testing a patch.\n> >>\n> >> [1]: https://lore.kernel.org/git/1m55i3m.1fum4zo1fpnhncM%25lists@haller-berlin.de/\n> >\n> > I'm very opinionated here.  I'm just one person, so definitely take\n> > this with a grain of salt, but in my view...\n> >\n> > Personally, I think implementing any per-commit hook in rebase by\n> > default is a mistake. It enforces a must-be-in-a-worktree-and-the-\n> > worktree-must-be-updated-with-every-replayed-commit mindset, which I\n> > find highly problematic[2], even if that's \"what we always used to\n> > do\".\n> >\n> > [2] https://lore.kernel.org/git/20231124111044.3426007-1-christian.couder@gmail.com/\n>\n> I'm not hip with what most pre-commit hooks do, but I'll point out that\n> a hook like pre-commit assuming there is a worktree is the fault of the\n> hook implementation, not of the infrastructure that invokes the hook. I\n> imagine most folks on this list are aware that a worktree is not needed\n> to create a commit and update a branch to point at it.\n\nWell, since git-commit requires a worktree (cmd_commit in cmd_struct\nin git.c has NEED_WORK_TREE in its options), and \"pre-commit\" is\nlikely to be inferred to be a \"git-commit\" thing (in fact, my reading\nof the current text of the pre-commit hook in the githooks(1) manpage\nseems to suggest that this hook is tied exclusively to git-commit),\nassuming a working tree for \"pre-commit\" doesn't seem like a very far\nleap at all.  In fact, I can't see why that assumption is broken given\nour current documentation.\n\nPerhaps we'd like to change the documentation and try to avoid such an\nassumption...but even in cases where we've made explicit claims in the\npast about various assumptions not being reliable, users have gone\nahead and made those assumptions anyway (Hyrum's Law), and then we\nhave sometimes been unable to make changes that broke those\nassumptions.\n\nSo, I'm still concerned, especially if this is applied to every commit.\n\nGranted, hooks are already pretty messed up.  We invoke the\npost-commit hook -- IF we're using the merge backend (and don't invoke\nit if we're using the apply backend), meaning it's not clear to users\nwhether the post-commit hook will be invoked or not in a rebase.\n(Especially since while the backend can be specified directly, it's\noften just selected based on other options that are only implemented\nby one of the two backends).  I've put a lot of work into making the\nrebase backends more consistent and would like them to be even more\nso, but the history here is slightly troubling.  The interactive\nbackend was \"fixed\" once, accidentally, and then the accident was\nrealized and was treated as a bug and reverted\n(https://lore.kernel.org/git/67a711754efce038914e8ec15c5dec4a5983566d.1571135132.git.gitgitgadget@gmail.com/),\nleaving us again inconsistent.  At least we've documented the\ninconsistency and the desire to change it\n(https://lore.kernel.org/git/pull.749.v3.git.git.1586044818132.gitgitgadget@gmail.com/).\n\n> FWIW, I would also find such a mindset to be highly problematic :-) I'll\n> take a moment here to thank you, Christian, and everyone else in that\n> effort for your interest in and work on git-replay; I've been trying to\n> watch its activity on-list closely in the hopes that we can adopt it\n> into our system once it's ready.\n\nI'm glad others are interested in git-replay.  Sadly, the work on\npushing it forward stopped about a year and a half ago.  All work\nsince then was limited to pulling out the bits that were ready to be\nupstreamed and cleaning those up.\n\n> > Because of that, I would prefer to see this at most be a command line\n> > flag. However, we've already got a command line flag that while not\n> > identical, is essentially equivalent: \"--exec $MY_SCRIPT\" (it's not\n> > the same because it's a post-commit check, but you get notification of\n> > any problematic commits, and an immediate stop of the rebase for you\n> > to fix up the problematic commit; fixing up the commit shouldn't be\n> > problematic since you are, after all, already rebasing).\n>\n> Indeed, and an\n>\n>     --exec 'git hook run pre-commit || git reset --soft HEAD~'\n>\n> would probably get you farther. I can certainly see an argument for\n> this, but from the perspective of designing a system for other\n> developers to use, such a rebase would have to be triggered\n> automatically (perhaps on pre-push).\n\nWell, there is a pre-push hook...  ;-)\n\nBut yeah, it does defer the discovery of the issue for the developer,\nwhich is kind of counter-productive.\n\n> > I see Phillip already responded and suggested not running the\n> > pre-commit hook with every commit, but only upon the first commit\n> > after a \"git rebase --continue\".  That seems far more reasonable to me\n> > than running on every commit...though even that worries me in regards\n> > to what assumptions that entails about what is present in the working\n> > tree.\n>\n> It's worth noting the context here is to prevent developers from\n> committing conflict markers, so this would actually be exactly\n> sufficient.\n\nAh, that makes sense what you want to do.  Totally fair.\n\n> Invoking pre-commit at this time would also be consistent with the\n> behaviors of prepare-commit-msg, commit-msg, and post-commit -- at least\n> when I reword a commit during a rebase.\n>\n> However, post-commit is executed after each picked commit during a\n> rebase\n\nOnly if using the merge backend, and even in that case I personally\ndon't think it should be executed; see the \"Hooks\" subsection of the\n\"Behavioral Differences\" section of the git-rebase manpage.\n\n> , so pre-commit there would also be consistent.\n\nOr maybe consistently inconsistent (i.e. being consistent with the\ninconsistency that exists with the post-commit hook between backends),\nand diverging even further from the aspirational goal in the \"Hooks\"\ndocumentation in the git-rebase manpage.\n\n> > (For example, what about folks with large repositories, so large that\n> > a branch switch or full checkout is extremely costly, and which could\n> > benefit from resolving conflicts in a separate sparse-checkout\n> > worktree, potentially much more sparse than their main checkout?\n>\n> As it happens, a single checkout of our source runs upwards of 2GB, so\n> I'm exactly in the population you're describing :-)  The main reason\n> we're moving to Git from SVN is that an SVN checkout can take upwards of\n> an hour for us today -- even with some real shenanigans to make them go\n> faster. On the Git side, we've also looked into (though I don't recall\n> if we had much success with) narrowing the sparsity patterns to just the\n> conflicts for conflict resolution workflows -- particularly when moving\n> feature code between separate trunks. So I guess I'm also glad we\n> weren't too far off in left field on that one! (As I recall, one of the\n> main challenges we faced there was ensuring there was enough stuff\n> 'still around' so that both binary and project references could resolve\n> and folks could use that information to help resolve conflicts.\n> Hopefully git-replay can be smart enough to allow some customization on\n> that front. We found some success with feeding the list of conflicted\n> files into some arbitrary logic that spat out the sparsity pattern to\n> use.)\n\nAn hour?  For a mere 2 GB?  I mean, I know 2 GB isn't exactly tiny and\nit's a pain to deal with...but an hour?  Do you have some directories\nwith an extraordinary number of files directly within them (i.e. not\nmultiple subdirectories deep, but a directory immediately containing a\nhuge number of files)?  Or some kind of network filesystem?  Or some\nreally slow hooks?\n\nAnyway, I'm glad others are concerned with slow checkouts and branch\nswitches too.  And yeah, the idea was just that we make an initial\nsparse checkout limited to the conflicted files, but users can use the\nsparse-checkout command to widen as needed.  The bigger piece was\nmaking sure the git code didn't defeat this by unnecessarily expanding\nthe index (currently with a sparse index, any conflicted files causes\nthe sparse-index to be expanded to a completely full index).\n\n> > And what if people like that really fast rebase resolution (namely,\n> > done in a separate very sparse checkout which also has the advantage\n> > of not polluting your current working tree) so much that they use it\n> > on smaller repositories as well? Can I not even experiment with this\n> > idea because of the historical per-commit-at-least-as-full-as-main\n> > -worktree-checkout assumptions we've baked into rebase?)\n>\n> I'd be interested in reading more about this baked-in assumption. Are\n> these mostly laid out in replay-design-notes.txt[3]?\n\nmerge-recursive, our default merge backend for about 15 years (and\nreused in rebase, cherry-pick, etc. too) only worked in a worktree and\nmuddied it as it went.  The other merge algorithms also assumed a\nworktree was present.  When I wrote merge-ort and removed the\nrequirement for having a worktree, I naturally wanted to fix rebase\n(and cherry-pick) to stop updating the working tree with every single\ncommit being rebased; it was such a needless waste of resources.  But\nthat assumption was all over the code and hard to remove.  And, I ran\ninto several items where it was unclear if \"fixing\" the code would be\ndeemed a backward compatibility break.  Hooks were one of the issues.\n\nIt's been quite a while since I've looked at it, but yes, I suspect\nsome of the issues are documented (explicitly or implicitly) in the\nreplay-design-notes.txt file, and you may see some as well in the\n\"Behavioral differences\" section of the git-rebase manpage.\n\nBut there may well be additional parts that aren't documented, since I\nnever went through the full effort to de-worktree-ify rebase, and only\ngot git-replay doing some basic things.  So, at least part of the\nissue is what other unknown assumptions are in the code which people\nmay have knowingly or unknowingly depended upon.\n\n> > While at it, I should also mention that I'm not a fan of the broken\n> > pre-rebase hook; its design ties us to doing a single branch at a\n> > time.  Maybe that hook is not quite as bad, though, since we already\n> > broke that hook and no one seemed to care (in particular, when\n> > --update-refs was implemented).  But if no one seems to care about\n> > broken hooks, I think the proper answer is to either get rid of the\n> > hook or fix it.\n>\n> If I were to guess, this likely stems either from an inexact definition\n> of the hook in documentation (ultimately resulting in incomplete tests)\n> or folks incorrectly assuming what each hook should do based purely on\n> its name.\n\nNo, neither is true.  The definition is very exact; the problem is\nthat the definition is excessively rigid.  It says, \"The second\nparameter is *the* branch being rebased\" (emphasis added).  There is\nno facility in the hook for rebasing more than one branch or\nreference, and if we attempt to pass additional parameters, we're\npotentially breaking all the existing pre-rebase hooks (which won't be\nprepared to handle the extra parameters and thus may fail to reject\nthe rebase for those other branches that are being rebased, and thus\ndefeat the point of the hook).  Therefore, allowing rebase to operate\non multiple branches is a backward compatibility break...though one\nthat we overlooked/ignored when we added --update-refs.\n\nAnd I want something more general than rebase's --update-refs; I want\nthe ability to replay multiple branches that aren't entirely contained\nwithin each other, and perhaps only have a little common history (or\nmaybe even none) since the base point(s) we're replaying from.\n\n> Which leads to an interesting point: pre-commit specifically states that\n> it is invoked by git-commit -- not that it's invoked whenever a commit\n> is created. So perhaps the correct thing to do here (if a hook is in\n> fact needed) would be to define a new hook -- but I worry about doing\n> that in the current state where there doesn't *seem* to be very rigid\n> coordination of when client hooks are invoked in terms of plumbing\n> rather than porcelain.\n\nYeah, as you're discovering, hooks are a mess.  Made all the more\nmessy by the fact that higher level commands traditionally started as\nscripts that invoked other lower-level commands (git-rebase.sh\nliterally called git-cherry-pick and git-commit, or git-format-patch\nand git-am, or git-merge-recursive and git-commit, or ...), and now\nthat we've discovered inconsistencies between backends, and\nperformance problems, and whatnot, it is not clear what we should or\neven can do.  Also, even when we rewrote the scripts into C code,\nwe've often done so by reimplementing shell in C -- we just fork\nvarious subprocesses repeatedly (yes, really), and then maybe come\nalong later and clean it up a little.\n\n> > Anyway, as I mentioned, I'm quite opinionated here.  To the point that\n> > I deemed git-rebase in its current form to possibly be unfixable\n> > (after first putting a lot of work into improving it over the past\n> > years) and eventually introduced a new \"git-replay\" command which I\n> > hope to make into a competent replacement for it.  Given that I do\n> > have another command to do my experiments, others on the list may\n> > think it's fine to send rebase further down this route, but I was\n> > hoping to avoid further drift so that there might be some way of\n> > re-implementing rebase on top of my \"git-replay\" ideas/design.\n>\n> I appreciate your perspective; you've certainly thought a lot about this\n> space -- and I definitely share your goal of consolidating\n> implementations for obvious reasons.\n>\n> So I suppose that leaves me with four possible paths forward:\n>\n> 1. Pursue invoking pre-commit before each commit in `git rebase` (likely\n>    generic in the sequencer) to be consistent with post-commit.\n>\n>    It sounds like this isn't a popular option, but I'm curious to folks'\n>    thoughts on the noted behavior of post-commit here.\n\nI've already said quite a bit about this above, but as a quick\nreminder: (1) I'm not sure it makes sense to make something consistent\nwith something that is inconsistent, and (2) we have a documented\ndesire to stop calling post-commit from rebase even in the situations\nwhere it is currently called.\n\n> 2. Pursue invoking pre-commit on `git rebase --continue` (likely on any\n>    --continue in the sequencer). This has the benefit of using existing\n>    configuration on developer machines to purportedly 'do the right\n>    thing' when its likely humans are touching code during conflict\n>    resolution. It's worth noting this isn't the only reason you might\n>    --continue, though, since the naive interpretation of this approach\n>    completely ignores sequencer commands like 'break', though it could\n>    probably just do what commit-msg does.\n\nIn the case of `break`, though, the commit was already transplanted,\nso a `--continue` doesn't need to create one before moving on.  Thus,\nwe could say that only when rebase --continue has an unfinished commit\nthat needs to be created (i.e. we have a conflicted commit to resolve)\nthat the `pre-commit` hook gets called.\n\nThe idea of having an 'unfinished' commit state is also useful because\ninteractive rebases make it easy to forget if you're in the middle of\na break/edit or trying to resolve a conflict elsewhere.  That seems to\nhappen to me far too many times, and I really want `git commit\n--amend` to throw an error (unless overridden with e.g. --force) when\nyou have an unfinished commit that you are working on by resolving\nconflicts.  In fact, this state is probably already recorded\nsomewhere, we just need to make better use of it...but I'm going off\non a tangent now.\n\n> 3. Define and implement a new hook that is called whenever a new commit\n>    is about to be (or has been?) written. Such a hook could be\n>    specifically designed to discourage assuming there's a working copy,\n>    though we're kidding nobody by thinking it won't be used downstream\n>    with that assumption. At least we could be explicit about\n>    expectations, though.\n>\n>    This is *probably* a lot more design work than this little paragraph\n>    lets on, but I've not personally watched the introduction of a new\n>    hook so I don't have context for what to expect.\n\nYeah, and at least as worded, this suggestion would run afoul of the\nlogic for the distinction between pre-commit and pre-merge-commit.\nAnd it seems to have all the same problems as #1, unless we restrict\nit to just when running \"rebase --continue\", in which case it's not\nclear what it buys us over #2.\n\n> 4. Trigger a rebase --exec in our pre-push. This is certainly the least\n>    work in git.git (i.e., no work at all), but it comes with the\n>    distinct disadvantage of playing whiplash with the developer's focus.\n>    During conflict resolution, they're thinking about conflicts. When\n>    you're ready to push, its likely that you're no longer thinking about\n>    conflicts.\n\nYeah, simple to implement but much less helpful.\n\n> Does the behavior of post-commit here change any minds?\n\nNo...but although I'm slightly worried about solution #2 (as opposed\nto very worried with #1), your usecase for pre-commit seems quite\nreasonable.  So, I can get behind #2, though it'd be nice if we could\nupdate the description of the `pre-commit` hook (so that it's clear\nthat it's not just \"git-commit\"), and while we're at it perhaps try to\nbe more explicit about what can and can't be assumed by the hook.\n"},{"id":"486345","messageId":"xmqqttnroig4.fsf@gitster.g","threadId":"60647","inReplyTo":"m0sf3vi86g.fsf@epic96565.epic.com","subject":"Re: rebase invoking pre-commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-01-05T16:26:03Z","receivedAt":"2024-01-05T16:26:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sean Allred <allred.sean@gmail.com> writes:\n\n> Is there a current reason why pre-commit shouldn't be invoked during\n> rebase, or is this just waiting for a reviewable patch?\n>\n> This was brought up before at [1] in 2015, but that thread so old at\n> this point that it seemed prudent to double-check before investing time\n> in a developing and testing a patch.\n>\n> [1]: https://lore.kernel.org/git/1m55i3m.1fum4zo1fpnhncM%25lists@haller-berlin.de/\n\nIf you are trying to make it less likely that your developers would\ncommit conflict markers by mistake, I think an effective way would\nbe to give \"git rebase\" an option (or a configuration variable) that\nforbids it from making a new commit upon \"git rebase --continue\",\nwhich AFAIK was added merely to help \"lazy\" folks to omit the\nexplicit \"git commit\" step in the following sequence:\n\n    $ git rebase origin/master\n    ... stops with conflicts\n    $ edit\n    ... now the conflicts are resolved (and hopefully you have\n    ... tested the result)\n    $ git commit\n    $ git rebase --continue\n\n"}]}