Rebase tool shouldn't perform any automated merge steps, because bad TS code might be accidentally merged
justingrant opened this issue · 3 comments
Now that upstream's ecmascript.mjs merges properly into ecmascript.ts, there's a new problem: the rebase tool can now perform fully automated rebase steps when there are no merge conflicts for a commit. (Before it was almost certain to have at least one merge conflict in every commit.)
IMO this auto-continue behavior is bad, because even when there's no merge conflicts, there's still a chance that TS-specific edits are needed. Being popped right into the commit message editor makes it too easy to accidentally close the editor and continue the rebase before making needed edits.
It's true that using an --exec might be able to catch this, but that adds a lot of extra time for each commit, given that I'm already manually running tests before continuing.
IMO a better solution would be to (if possible) require the tool to stop on every commit, whether there are merge conflicts or not.
Also, if you use a separate terminal window (or VS Code's IDE) to unstage the staged files, make edits, and re-stage them, and then you close the editor, then the rebase will be hosed with errors. IMO that's another reason to avoid auto-continuing.
If we do this, then one thing to check would be to make sure that the authorship of each commit stays the same. In the past, when I've interrupted commits during a rebase, it lost the original commit author.
FWIW, when I cleared the commit message editor and saved the file, unstaged, made edits, re-staged, and then trt continue-d, the commit author stayed the same. Yay! Evidence: 72e72a1
Somehow I missed this in my email and only saw this now. Lots of thoughts, but I do think there's plenty of improvement here.
using an --exec might be able to catch this
Maybe. The current implementation adds the "thing to run" as a git exec command, and so has the disadvantage that when it runs (and fails) the commit has already happened, and so we'd have to amend to that commit (and be careful not to mess up authorship, commit message, upstream commit tracking, etc) to fix problems it finds. This does have the advantage that, from git's perspective, it's ~impossible to accidentally skip over the testing involved (as if it fails then when you next continue the exec is retried). It has the downside that it doubles the number of steps as seen by git, and so the number of remaining actions becomes... demoralizingly high... at times.
I think I considered replacing this implementation with a slightly more complicated one where the tool itself always run the exec if it was going to continue (including user-initiated continues). This can be fairly cheap depending on what is in the exec script, and the tool itself tries to keep track of when it doesn't need to re-test (
).Running the TS compiler before every continue seems like a decent compromise here.
given that I'm already manually running tests before continuing.
Isn't this just you running all the same steps that would be run by an exec script? Or do you sometimes skip these steps if you think a change is trivial enough? If there's a good heuristic then adapting that to automation might be a good compromise.
a better solution would be to (if possible) require the tool to stop on every commit
Another idea: we could have the tool always stop regardless if source files within polyfill/lib/ (upstream) changed. This would allow the automation to breeze through the simpler things (test262, docs and spec changes) while still allowing granular control later.
a better solution would be to (if possible) require the tool to stop on every commit
Another idea: we could have the tool always stop regardless if source files within polyfill/lib/ (upstream) changed. This would allow the automation to breeze through the simpler things (test262, docs and spec changes) while still allowing granular control later.
Agree, this sounds like the best solution.
given that I'm already manually running tests before continuing.
Isn't this just you running all the same steps that would be run by an exec script? Or do you sometimes skip these steps if you think a change is trivial enough? If there's a good heuristic then adapting that to automation might be a good compromise.
Yes, same steps. But my manual steps run before the commit happens, so I won't have to worry about amending and the other worries you noted above. After ~100 porting commits, my workflow has been to run without an exec at all. I just run tests manually before each continue. It's worked pretty well, although I did forget once or twice in and had to fix things up later after the rebase was done.
has the downside that it doubles the number of steps as seen by git, and so the number of remaining actions becomes... demoralizingly high... at times.
Also, when you're porting a lot of commits, running 45+ seconds of tests TWICE for each commit can get really frustrating. This was the main reason I stopped using execs.
I think I considered replacing this implementation with a slightly more complicated one where the tool itself always run the exec if it was going to continue (including user-initiated continues).
Do you mean that instead of git running the exec, trt would run it (and then not even run git rebase --continue if the exec fails) ?
Running the TS compiler before every continue seems like a decent compromise here.
Not sure I understand the proposal yet, but might be better to leave this up to the caller? Or maybe make a default exec script and run that, and the caller could choose a different script?