replace-fork should not clear uncommitted changes #22348
Conversation
The replace-fork script depends on ESLint to fix the reconciler imports — `.old` -> `.new` or vice versa. If ESLint crashes, it can leave the imports in an incorrect state. As a convenience, @bvaughn updated the script to automatically run `git checkout -- .` if the ESLint command fails. An unintended consequence of the strategy is that if the working directory is not clean, then any uncommitted changes will be lost. We need a better strategy for this that prevents the accidental loss of work. One option is to exit early if the working directory is not clean before you run the script, though that affects the usability of the script. An ideal solution would reset the working directory back to whatever state it was in before the script ran, perhaps by stashing all the changes and restoring them if the script aborts. Until we think of something better, I've commmented out the branch.
facebook-github-bot
added
CLA Signed
React Core Team
|
Comparing: f50ff35...854881d Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
|
yesssssss. |
|
Why not just have the script |
|
But then it messes with my git index :( which is part of my workflow |
|
If it restores the index back to what it was that would be nice, though |
|
Well, it could also do the following:
|
|
Do you mind if we leave that for a separate improvement and merge this in the meantime, though? I keep almost losing my work because of the existing behavior. |
|
Sure |
|
I think personally the behavior I would like best is if it only ever touched reconciler files (the ones ending in |
|
So long as it restored the repo to its pre-existing state after a failure, would you have any complaints? |
|
If it doesn't stage everything then no :D Another solution could be to add another command |
|
TBH, I think running |
|
Yeah that would also work for me, as long as I can override it somehow 😈 |
|
This might seem like a weird workflow but it's fairly common for me: I'll have just completed a large change and I want to break it into steps, so I stage the first part, run |
|
And ESLint crashes (as opposed to errors) aren't that common so I usually don't mind if the old reconciler files are left in a broken state because all I have to do is fix the crash then run the command again |
|
Yeah, it's not common, but it has caused contributor confusion before. (That's how I became aware of it and made the previous change.) I've posted a follow up #22364 that I think addresses both of our concerns. |
The replace-fork script depends on ESLint to fix the reconciler imports — `.old` -> `.new` or vice versa. If ESLint crashes, it can leave the imports in an incorrect state. As a convenience, @bvaughn updated the script to automatically run `git checkout -- .` if the ESLint command fails. An unintended consequence of the strategy is that if the working directory is not clean, then any uncommitted changes will be lost. We need a better strategy for this that prevents the accidental loss of work. One option is to exit early if the working directory is not clean before you run the script, though that affects the usability of the script. An ideal solution would reset the working directory back to whatever state it was in before the script ran, perhaps by stashing all the changes and restoring them if the script aborts. Until we think of something better, I've commmented out the branch.
The replace-fork script depends on ESLint to fix the reconciler imports —
.old->.newor vice versa. If ESLint crashes, it can leave the imports in an incorrect state.As a convenience, @bvaughn updated the script to automatically run
git checkout -- .if the ESLint command fails. An unintended consequence of the strategy is that if the working directory is not clean, then any uncommitted changes will be lost.We need a better strategy for this that prevents the accidental loss of work. One option is to exit early if the working directory is not clean before you run the script, though that affects the usability of the script.
An ideal solution would reset the working directory back to whatever state it was in before the script ran, perhaps by stashing all the changes and restoring them if the script aborts.
Until we think of something better, I've commented out the branch.