spacedentist / spr

Submit pull requests for individual, amendable, rebaseable commits to GitHub
https://getcord.github.io/spr/
MIT License
392 stars 41 forks source link

Add a `close` command #109

Closed joneshf closed 2 years ago

joneshf commented 2 years ago

We want to be able to close a PR from spr.

This follows a lot of what happens in the other commands: we grab the commit, find its related PR, close it, and delete any branches.

We also give it a --all option so that all PRs in a stack can be closed at once.

Closes: https://github.com/getcord/spr/issues/63

Test Plan: There are three scenarios we want to check: closing a PR based on the trunk branch, closing a PR based on a non-trunk branch, and closing a stack of PRs.

Trunk branch

  1. Create a PR with spr diff.
  2. Run spr close.
    • [ ] The PR should be closed
    • [ ] The head branch should be deleted on GitHub.
    • [ ] The commit message should no longer have the "Pull Request" or "Reviewed By" sections.

Non-trunk branch

  1. Create one PR with spr diff.
  2. Create a second PR with spr diff.
  3. Run spr close on the second PR.
    • [ ] The PR should be closed
    • [ ] The head branch should be deleted on GitHub.
    • [ ] The base branch should also be deleted on GitHub.
    • [ ] The commit message should no longer have the "Pull Request" or "Reviewed By" sections.

Stack of PRs

  1. Create one PR with spr diff.
  2. Create a second PR with spr diff.
  3. Run spr close --all on the second PR.
    • [ ] Each PR should be closed
    • [ ] Each head branch should be deleted on GitHub.
    • [ ] Each base branch should also be deleted on GitHub.
    • [ ] Each commit message should no longer have the "Pull Request" or "Reviewed By" sections.
joneshf commented 2 years ago

Welp, looks like working with a fork requires you to be a bit more careful when updating. I did:

# checkout this branch
$ git pull --rebase upstream master
# fix conflicts
$ spr diff

I think what I should've done is something like:

# checkout the `master` branch
$ git pull --rebase upstream master
# checkout this branch
$ git pull --rebase upstream master
# fix conflicts
$ spr diff

Likely will want to update the documentation in the fork PR with this information.

Lemme try to fix my mistake.

joneshf commented 2 years ago

Yeah, I'm just making it worse at this point. Might need to either: use actual git commands to rewrite the way it's supposed to be, or just open a new PR. Either way, going to deal with this later.

joneshf commented 2 years ago

The fix

Aight. I think it's sorted now. For posterity, I ended up doing:

# Push the original commit to the branch
$ git push --force-with-lease origin 2d31f24:spr/joneshf/add-a-close-command
Total 0 (delta 0), reused 0 (delta 0), pack-reused 0
To github.com:joneshf/spr.git
 + 3a1c30b...2d31f24 2d31f24 -> spr/joneshf/add-a-close-command (forced update)
# Remove the two branches created by accident.
$ git push origin :spr/joneshf/master.add-a-close-command
To github.com:joneshf/spr.git
 - [deleted]         spr/joneshf/master.add-a-close-command
$ git push origin :spr/joneshf/master.add-a-close-command-1 
To github.com:joneshf/spr.git
 - [deleted]         spr/joneshf/master.add-a-close-command-1
# Update now that everything should be sorted
$ spr diff
25138c9 Add a `close` command
  #️⃣   Pull Request #109: https://github.com/getcord/spr/pull/109
Message (leave empty to abort): Rebase atop `master` branch
  ⚾  Commit was rebased - updating Pull Request #109

Probably the proper approach

I think that what I should've done originally was:

# Get the trunk branch up to date
$ git checkout master
$ git pull upstream master --rebase
# Fix any conflicts.

# Update the ref to the remote trunk branch.
# I think this was the part I was missing.
$ git push origin

# Switch to the commit for this PR.
$ git checkout 2d31f24

# Rebase on top of the updated trunk
$ git rebase master

# Update the PR
$ spr diff

Thoughts

Might be something worth discussing the fork PR or issue, rather than here. But I expected that the upstream trunk branch would be the one that was important, not the origin trunk branch. There's a good chance I missed a change in that PR though, so I wouldn't worry about it too much. Maybe it's that this current_master_oid in src/diff.rs should be based on the upstream trunk rather than the origin trunk.

In any case, that's a lot of steps and knowledge to remember. Might make sense to have a command that does a lot of this stuff when working from a fork (or just in general).

EDIT: After thinking about it for a bit, it's not that this is a lot of steps inherently, it's that this is a lot of steps if you're using the buggy implemented of from https://github.com/getcord/spr/pull/106 and also using a branch. I think if the current_master_oid is pointing to upstream, it should be about as much work as laid out in the docs. The difference being that instead of git pull --rebase on the trunk, you'd git pull --rebase upstream $TRUNK.

That said, I do still think a command for updating things (spr update or something) that did the underlying git pull --rebase/spr diff --all/whatever else would be a nice addition.

joneshf commented 2 years ago

Okay, I think I've implemented all the suggestions. Mind giving it another look whenever you get a minute? More than happy to follow up on whatever else.

joneshf commented 2 years ago

Alright, ready for another pass. Lemme know if I missed anything!

joneshf commented 2 years ago

Yeah, I can't merge this PR. Only someone with write access (like you) can merge this PR. Thanks for working through this with me!

joneshf commented 2 years ago

Thanks for reviewing/merging this!