Fix issue with restoring stashed code with partially edited files by tuchfarber · Pull Request #1251 · evilmartians/lefthook

tuchfarber · 2025-12-18T20:07:05Z

Context

#1216 happens because lefthook compares the pre-stash changeset with a post-stash changeset, meaning if you have fail_on_changes enabled and a partially updated file, it will fail.

Changes

This PR adds a copy of the changesetBefore code after the stashing code so if we make it to stashing (i.e. it doesn't exit earlier) we reset the changesetBefore to be accurate.

NOTE: I'm not 100% confident on the test addition. I verified it fails in the current code and passes in the new code, and I've built the binary and manually tested as well. I attempted to match existing code/style.

This fixes evilmartians#1216 by recalculating the changeset after the stash of partially edited files is completed.

scop · 2025-12-19T17:24:20Z

Thanks for looking into this! #1216 is not a well written issue because as mentioned, it actually contains two. my bad. This appears to address only one of them, the other "revert+retry when restoring stashed fails" remains as far as I can tell. But nevermind, if/when this is merged, I can file another issue for the remaining parts.

tuchfarber · 2025-12-19T17:48:08Z

Thanks for looking into this! #1216 is not a well written issue because as mentioned, it actually contains two. my bad. This appears to address only one of them, the other "revert+retry when restoring stashed fails" remains as far as I can tell. But nevermind, if/when this is merged, I can file another issue for the remaining parts.

Ah! My bad. My team kept getting bit on this problem on partially staged files so I just trying to fix that. Looking at the issue again, do you have any opinions on comparing the pre-stash changeset or the post-stash changeset? I realize I did post-stage, but your issue sounds like you'd prefer pre-stash.

scop · 2025-12-20T08:40:46Z

Operating on post stash changesets is the right thing to do, i.e.

stash unstaged,
take changeset snapshot,
run tools,
take another changeset snapshot,
compare changesets taken in 2 and 4, determine if we should fail on changes and output diff,
restore stashed changes.

The other issue I'm talking in #1216 is that it's possible that the tools have made changes in step 3) that make the originally stashed changes in step 1) fail to apply when we attempt to restore them in 6). In that case I think the right thing to do would be to remove all changes made by tools in step 3) and attempt restoring again. I.e. what pre-commit does in https://github.com/pre-commit/pre-commit/blob/8a0630ca1aa7f6d5665effe674ebe2022af17919/pre_commit/staged_files_only.py. This is because the changes made by tools can be easily redone, but the stashed changes may contain more valuable work that cannot be automatically/easily redone.

tuchfarber · 2025-12-29T21:40:23Z

That all makes sense! I've updated the after() method to take care of that as well. Basically:

Deferring the return of the error until after the stash restore is done, and
Removing tool changes (git. HideUnstaged(changes)) if RestoreUnstaged() fails before trying RestoreUnstaged() again

p.s. I don't regularly write go code, so holler if I made it too Python-y or anything.

tuchfarber · 2026-01-09T18:43:14Z

@scop Just looking for a ballpark of when this could be reviewed and potentially released. Not trying to rush, just debating if I should move over our laptops to my fork in the meantime to prevent potential data loss.

scop · 2026-01-19T13:24:23Z

Sorry for taking so long getting back to this, and thanks heaps for working on it!

I intend to try and find time to try this out and review soon(tm) (hopefully this week), unless it is merged before that. But I'd like to note that I'm not a lefthook project member but just an individual (opinionated :)) contributor, and it'll be up to @mrexox et al to decide what goes in and how the tool is to behave, so I cannot promise anything regarding merging and releasing. I do hope that if/when I review/approve this, it'll make their job of accepting this a bit easier though ;)

scop · 2026-01-25T19:23:15Z

internal/run/controller/guard.go

+	// Capture changeset after stashing partially staged files, so we compare the same state
+	// before and after running hooks


I'm somewhat wondering why we capture the first changeset at the beginning of before, in addition to this. Maybe because it's kind of necessary also when not stashing changes for fail-on-changes support for that case.

Maybe the first one should be taken only if we are not stashing changed files?

If we fail to get the changeset here for some reason, we still end up comparing the pre-stash changeset (in case taking that one succeeded). If we want to fix that, I suppose we need to add some tracking to differentiate whether we actually succeeded in taking a changeset and it just happened to be empty, or if it's empty because an error occurred.

Failing on changes can be an "important" thing, so from that PoV maybe we should error out if taking any of the changesets fails (both in before() and after() (in after() after trying to restore stashed changes)), instead of just logging warnings about them. That would simplify some corner cases, too, including likely the above one.

scop · 2026-01-25T19:26:21Z

internal/run/controller/guard_test.go

 				{Command: "git stash drop --quiet 1", Output: ""},
 			},
 		},
+		"stashUnstagedChanges=true failOnChanges=true with partially staged no hook changes": {


macOS tests are failing for some reason.

scop · 2026-01-25T19:27:25Z

internal/run/controller/guard_test.go

 				{Command: "git stash drop --quiet 1", Output: ""},
 			},
 		},
+		"stashUnstagedChanges=true failOnChanges=true with partially staged no hook changes": {


If the codecov numbers are sane, this being an important data loss related change, maybe we should look into increasing the test coverage for the changes?

scop · 2026-01-25T19:28:07Z

Added some observations on a quick read through, did not test (will try to do that sometime later).

mrexox · 2026-02-27T09:58:49Z

Hey guys! I'm really sorry for keeping this PR for so long. I am going to merge it and adjust the code a little bit, I'll try to address the original issue, but I need some time. After that I'll release a new version and ask you to take a look and make sure that everything is in the right places.

Thank you for your contribution!

scop · 2026-03-02T13:53:48Z

Thanks!

Simplistic testing with fail_on_changes: always looks good to me functionalitywise, nice!

As a user, as a minor improvement, I think I'd prefer to see something like this diagnostic in the output when lefthook forcibly removes changes made by tools to get the stashed ones to apply: "Could not restore stashed changes. reverting ones made by hooks and retrying". IIRC pre-commit and prek do something like this.

mrexox · 2026-03-03T09:42:09Z

I think it makes sense to defined a task for detecting changes and restoring previous state. I still lean to always printing the diff if changes get detected.

If you could prepare a feature-request, and fill it with what's expected, that would be great!

tuchfarber changed the title ~~Fix issue with restoring stashed code with partially edited files~~ [WIP] Fix issue with restoring stashed code with partially edited files Dec 18, 2025

tuchfarber changed the title ~~[WIP] Fix issue with restoring stashed code with partially edited files~~ Fix issue with restoring stashed code with partially edited files Dec 18, 2025

tuchfarber force-pushed the tuchfarber/fix_stash_issue branch 2 times, most recently from 743a078 to 1850053 Compare December 19, 2025 16:44

Fix failed_on_changes with partially staged files

4bef010

This fixes evilmartians#1216 by recalculating the changeset after the stash of partially edited files is completed.

tuchfarber force-pushed the tuchfarber/fix_stash_issue branch from 1850053 to 4bef010 Compare December 19, 2025 16:48

tuchfarber marked this pull request as ready for review December 19, 2025 16:48

tuchfarber requested a review from mrexox as a code owner December 19, 2025 16:48

Rollback hook changes if stashed changes can't be restored

62e33e2

scop reviewed Jan 25, 2026

View reviewed changes

Add coverage

35e1a97

mrexox merged commit 246f1c9 into evilmartians:master Feb 27, 2026
10 of 13 checks passed

		// Capture changeset after stashing partially staged files, so we compare the same state
		// before and after running hooks

Conversation

tuchfarber commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Changes

Uh oh!

scop commented Dec 19, 2025

Uh oh!

tuchfarber commented Dec 19, 2025

Uh oh!

scop commented Dec 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tuchfarber commented Dec 29, 2025

Uh oh!

tuchfarber commented Jan 9, 2026

Uh oh!

scop commented Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scop Jan 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scop Jan 25, 2026

Choose a reason for hiding this comment

Uh oh!

scop Jan 25, 2026

Choose a reason for hiding this comment

Uh oh!

scop commented Jan 25, 2026

Uh oh!

mrexox commented Feb 27, 2026

Uh oh!

Uh oh!

scop commented Mar 2, 2026

Uh oh!

mrexox commented Mar 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tuchfarber commented Dec 18, 2025 •

edited

Loading

scop commented Dec 20, 2025 •

edited

Loading

scop commented Jan 19, 2026 •

edited

Loading

scop Jan 25, 2026 •

edited

Loading