When we have a commit that is directly based on master, we don't create a base branch for the Pull Request.
When updating such a PR, spr currently creates a base branch when the commit was rebased on a newer master. That's not necessary though. We can just merge the newer master into the Pull Request branch, no need to create a base branch for that.
This is a regression, probably introduced by #48.
This commit changes the logic actually only very slightly to fix this. Changing
let pr_base_parent = if pr_base_tree != new_base_tree
|| (pr_has_base_branch && needs_merging_master)
to
let pr_base_parent = if pr_base_tree != new_base_tree
&& (pr_has_base_branch || !directly_based_on_master)
would have done the trick, but I have added a long comment to explain the situation and reordered the branches in the implementation for better readability.
That does indeed fix the unnecessary creation of base branch commits, but it does not yet fix the creation of the base branch. When I rebased a single commit onto newer master and updated a test PR, a base branch was still created, just with no commits on it, i.e. just pointing at the master commit I rebased on.
In the rest of the code, pr_base_parent.is_some() is used to check if a base branch is in use. And that's of course wrong, as pr_base_parent may also be a master commit we want to merge in. I tidied the code up a bit and it should be a lot easier to follow now.
Specifically, base_branch is first set as an Option<GitHubBranch> which contains the existing base branch or None if there isn't one. Next, it is changed to be a GitHubBranch that contains the existing base branch, or a generated name for a new one, if there isn't one already, even if we end up not using a base branch anyway.
This commit changes that: base_branch is still initialised as Option<GitHubBranch> as before, but it remains that type. When we construct a new base branch commit, we generate the name for a new base branch if there isn't one already, but base_branch remains an Option and remains None if there is no base branch and we are not creating one.
As a result base_branch.is_some() is the correct way to test whether a base branch is being used. (That makes so much sense, right?)
Fixes #72
Test Plan:
cargo build and use the new build for submitting/landing this PR.
Also, in a test repository:
create a new commit whose parent is an older master commit, spr diff
amend the commit, spr diff
rebase the commit on newer master, spr diff
amend the commit, spr diff
Up to here, the PR should have 4 commits in it, but no base branch (i.e. target branch is still master).
make another commit and reorder the commits on the local branch, so the PR commit is on top of the new commit. spr diff
This now should create a master branch.
start another new branch in the test repo, base should be an older master commit. Make two commits on the branch, spr diff both of them. The top commit's PR should have a base branch.
amend the top commit, spr diff -> should update PR but not add to the PR's base branch
amend the bottom commit, update the top commit's PR with spr diff -> should update PR and add a new commit to base branch that gets merged into PR branch
rebase the branch on newest master, spr diff the top commit -> should update PR, add a new commit to base branch that merges in new master, and the PR branch merges in the new base branch commit
When we have a commit that is directly based on master, we don't create a base branch for the Pull Request. When updating such a PR, spr currently creates a base branch when the commit was rebased on a newer master. That's not necessary though. We can just merge the newer master into the Pull Request branch, no need to create a base branch for that. This is a regression, probably introduced by #48.
This commit changes the logic actually only very slightly to fix this. Changing
to
would have done the trick, but I have added a long comment to explain the situation and reordered the branches in the implementation for better readability.
That does indeed fix the unnecessary creation of base branch commits, but it does not yet fix the creation of the base branch. When I rebased a single commit onto newer master and updated a test PR, a base branch was still created, just with no commits on it, i.e. just pointing at the master commit I rebased on.
In the rest of the code,
pr_base_parent.is_some()is used to check if a base branch is in use. And that's of course wrong, aspr_base_parentmay also be a master commit we want to merge in. I tidied the code up a bit and it should be a lot easier to follow now. Specifically,base_branchis first set as anOption<GitHubBranch>which contains the existing base branch orNoneif there isn't one. Next, it is changed to be aGitHubBranchthat contains the existing base branch, or a generated name for a new one, if there isn't one already, even if we end up not using a base branch anyway. This commit changes that:base_branchis still initialised asOption<GitHubBranch>as before, but it remains that type. When we construct a new base branch commit, we generate the name for a new base branch if there isn't one already, butbase_branchremains anOptionand remainsNoneif there is no base branch and we are not creating one. As a resultbase_branch.is_some()is the correct way to test whether a base branch is being used. (That makes so much sense, right?)Fixes #72
Test Plan:
cargo buildand use the new build for submitting/landing this PR. Also, in a test repository:spr diffspr diffspr diffspr diffUp to here, the PR should have 4 commits in it, but no base branch (i.e. target branch is still master).
spr diffThis now should create a master branch.
spr diffboth of them. The top commit's PR should have a base branch.spr diff-> should update PR but not add to the PR's base branchspr diff-> should update PR and add a new commit to base branch that gets merged into PR branchspr diffthe top commit -> should update PR, add a new commit to base branch that merges in new master, and the PR branch merges in the new base branch commit