Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rebase tool feedback #236

Open
justingrant opened this issue Apr 12, 2023 · 1 comment
Open

Rebase tool feedback #236

justingrant opened this issue Apr 12, 2023 · 1 comment
Assignees

Comments

@justingrant
Copy link
Contributor

justingrant commented Apr 12, 2023

After running the rebase tool (see https://github.com/js-temporal/temporal-polyfill/blob/main/tools/rebasing.md), I ran into some problems and have some feedback:

  • Should document which commands to run from the tools folder and which to run from the root
  • If test.sh fails, should be able to continue afterwards, and ideally a more graceful error message.
  • Might be nice for the docs to remind me to chmod +x test.sh to avoid failing the rebase later.
  • Should document how to handle updates to ecmascript.mjs, e.g. git rm ... and git add .... Might want to document creating an alias for the git rm step because it's so common.
  • The alias command in the docs doesn't work on MacOS zsh shell; when I run trt it tries to run the commit hash, not the tools script. Is there another way to write that alias that would be more compatible?
  • There should be an abort command for the tool, and it should remove all the files created by the tool. In a perfect world there'd be a "stop" (keep already-done commits, but don't do more of them) and an "abort" (roll back all changes) command. But just having abort would be a good first step.
  • When a commit only updates the spec (no polyfill files) the tool still stopped. I worked around by git rm the spec file and then trt continue. I'd have expected it to skip that commit.
  • The step to find LATEST_UPSTREAMED_COMMIT seems like it could easily be automated away, maybe with an override option for the tool, but would be nice if I didn't have to look it up.
  • It'd also be nice to document (or have a tool command) to show the pending commits in reverse order, because when there's 100+ of them it's a pain to have to hold down the ENTER key while the single-line git log output scrolls to the end.
  • If something goes wrong and it can't continue, should document manual recovery procedure per Rebase tool feedback #236 (comment).
  • When the tool is all done going through commits, it changes the current branch to a detached head. Would be nice if instead it advanced the branch that it started on. At least I think that behavior would be nice. If it's not, then we should document how to turn that detached head into a branch that can be used for a PR.
  • The .temporal_rebase_tool directory has files in it after the tool is done. Is this expected? If not, then should fix. Note that the last commit in my group of commits was a no-op "Update Test262" commit (tc39/proposal-temporal@31de2c7), so not sure if this causes the problem.
  • Should test the tool in the case where Test262 updates are no-ops. I wonder if many of the problems I found had to to with the fact that the current Test262 is far ahead of the Test262 in each ported commit. Ideally the tool would have a friendly message like "Test262 is already later than this commit, so doing nothing" instead of a bunch of confusing console output.
  • Would be nice to have docs around non-code updates, e.g. to package.json. For example, es-abstract isn't used by the polyfill, so should probably mention that updates to es-abstract are either not needed or, if they fail tests, then we'd need to... what exactly?
  • The rebase tool should refuse to proceed if any polyfill/lib/*.mjs files are present in the commit (not removed yet before trt continue)
  • The rebase tool should refuse to continue if there are zero staged files. This would prevent me accidentally forgetting to stage the files to commit. Should require git rebase --skip or perhaps a trt skip command instead.
@justingrant
Copy link
Contributor Author

More: should document manual recovery procedure in case things can't continue. Here's some content from James as a starting point

If that fails, you could continue the rebase manually (git rebase --continue). If you want to "finish" the rebase without actually going through all the remaining commits, that can be done by:

  • git rebase --edit-todo, which will pop open an editor with a bunch of lines in it - these are all the actions left to do in the rebase. Delete them all, save the file, and close the editor. :)
  • then, once git is happy with the current state of the working tree, run git rebase --continue and the rebase will finish.

Note that it will dump you into a detached head state (if i recall properly), so you'll probably want to make a branch at that commit to be able to refer to it (git checkout -b <new branch name>)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants