Add pre-commit.stash_unstaged_changes to control stashing#1412
Add pre-commit.stash_unstaged_changes to control stashing#1412AlexanderLarionovQC wants to merge 3 commits into
pre-commit.stash_unstaged_changes to control stashing#1412Conversation
|
I appreciate AI slop can be a real thing, however it has been a while since I have worked in Thank you for your review! |
pre-commit option to disable partial stashingpre-commit.stash_unstaged_changes to control stashing
|
Hey @AlexanderLarionovQC, Thank you for preparing a PR! Adding a new option make sense, but I am also trying to fix the issue here: #1416. It should fix the both cases you've mentioned and I hope this option won't be necessary. I believe that it's preferred that stash restoration just works out of the box. WDYT? |
|
I'd like to advocate for the non-stashing workflow, too. (doesn't have to be the default, but having this option would be nice). Partial staging + conflicting linter changes is something I deal with daily. I see two ways to address it: A. Let lefthook operate on staged changes only (current)
B. Let lefthook operate on all changes regardless of git index (proposed by this PR)
If one doesn't use partial staging at all (always committing all changes), there are no cons for either of A or B. Otherwise, these are the cons I see in each approach: Cons of A:
Cons of B:
So the preference for A vs B might depend on how often the failed-to-restore scenario happens. In my case, the cons of B are so negligible that I'd almost always prefer B ( |
|
Thanks @jjurm for a clear breakdown and highlighting the pros/cons. The motivator for this PR is less about the existing open issues (I somewhat selfishly used them as justification) but rather just about not having a tool juggling state between stashes and the working directory automatically for the user. I hope the config option offered in this PR also lets users who work with partial commits/hunk based workflows the option to not benefit from some of the convenience of A for more manual control and less confusion with B. As this would also have no impact on existing On top of that, I think there is a compelling use-case for this option for agentic coding:
|
|
FWIW I agree with the sentiment that stashed change should work out of the box, and I appreciate the PR you have done in that direction @mrexox 🙇♂️ |
|
@AlexanderLarionovQC , just released a new version 2.1.8 with fixes for restoring the changes. I hope that they cover most of the cases. The main case covered: when hook jobs change files with unstaged changes, so they conflict, the hook changes get reverted, so you could stage the files and try again. |
|
Thank you @AlexanderLarionovQC and @jjurm, this makes sense. I will consider adding this option. This PR needs some effort to rebase, but I will probably prepare a new one with an option to avoid hiding changes. Agree that this is more of a global switch of the behavior, so maybe it requires to introduce some global (not a hook-level) option. Will think about the API. BTW, there is |
|
Thank you! If you would like me to rebase I can also do that. I think |
|
Hey @mrexox - is there any update on this? I am still having to tell my AIs to just stop using lefthook to avoid infinite recursions |
mrexox
left a comment
There was a problem hiding this comment.
My concern is that having both stash_unstaged_changes: false and stage_fixed: true brings confusion. The prior disables staging the changes and it's not obvious from a configuration file.
I agree that it's an implicit behavior that pre-commit hides unstaged changes. This behavior was added to support stage_fixed: true and avoid auto-fixing uncommitted changes with linter.
Could you please tell more about infinite recursion? How does it happen?
| @@ -40,7 +42,7 @@ var AvailableHooks = map[string]struct{}{ | |||
| } | |||
|
|
|||
| func HookUsesStagedFiles(hook string) bool { | |||
| return hook == "pre-commit" | |||
| return hook == PreCommitHookName | |||
There was a problem hiding this comment.
Could you please revert this change? I agree that this is a duplication, but it's more readable when using raw string values rather than constants
| - [`parallel`](./parallel.md) | ||
| - [`piped`](./piped.md) | ||
| - [`follow`](./follow.md) | ||
| - [`stash_unstaged_changes`](./stash_unstaged_changes.md) |
There was a problem hiding this comment.
Regarding the naming: I would prefer some boolean that is false by default and you need to explicitly set it true to enable. That's the common approach for boolean values in lefthook configuration. Something like keep_unstaged_files or include_unstaged. But there's still a possible problem with stage_fixed conflict.
I agree this, and the naming issue you highlighted, both point that there needs to be a little more thought on the UX of this. I think your name suggestions seem good and will go with those. With regards the
Sorry, I should have been more clear. This is not a programmatic infinite recursion but rather one of an agent working on a codebase. It is simply that if I have a partially staged file that raises a typing error, say, and the agent fixes this error, |
Context
pre-commit, runs hooks against the staged snapshot, and then restores the hidden changes.stage_fixed: true.Essentially, this PR enables an alternative Lefthook workflow where it us up to the user to more manually manage staging of formatting changes implemented by linters and formatters but this avoids any 'magic' from interfering for both agents and potential loss of data.
Changes
pre-commit.stash_unstaged_changesoption.pre-commitbehavior unchanged when the option is omitted.false, skips the automatic save, stash, revert, and restore flow for partially staged files.{staged_files}behavior andfiles:-based overrides.stage_fixedauto-staging in this mode so Lefthook does not re-stage files while partial hunks remain visible in the worktree.AI Usage Transparency
This change was drafted and largely implemented with GitHub Copilot using GPT-5.4 as an AI coding agent.
The resulting changes were then manually reviewed and validated with the repository's test, integration-test, and lint commands.