[Issue #149] Fix Commit Message to "Merges" instead of "Closes"

[pboling]

:+1:

[codenamev]

So without squashing commits before delivery, this will leave the PR in an open state (as GH only marks the PR as merged when using squash-merge via their merge button).

[simonzhu24]

@codenamev I believe that deliver does squash the commits before delivery?

In git_reflow.rb line 131, merge_feature_branch is called. This method (in git_helpers.rb) calls run_command_with_label "git merge --squash #{feature_branch_name}" after checking out to the destination branch.

The only question is that we might need to force push (-f flag). Thoughts?

[codenamev]

@simonzhu24 sorry for the lack of detail. At some point in the delivery process, the code does squash-merge. The issue is that we squash merge manually. Unfortunately GH is not able to keep track of the commits to ensure they make it to the branch we're merging to since they get squashed into a new single commit on the base branch. Their "merge button" can handle squash-merges now, but since GH is performing it they can tell what commits make it into the base branch and mark it appropriately.

To see an example of the issue with the new Merges keyword, see our fishgrub repo for a test commit I made to see if this would work. The related PR is still open.

Force-pushing master is never a good idea 😜

[simonzhu24]

@codenamev Thanks for your detailed explanation. So what is your advice here? Is there anything we can do to guarantee that the commits make it to the branch we are merging to? Or is this something that depends Github.

[codenamev]

Unfortunately this is a limitation of GH as they don't allow you to arbitrarily tag the PR with whatever state you want. By using reflow we can guarantee our commits get there :wink:

Issue #149 is a proposal to change the timing of the squash. We are having an internal discussion on how that might look. Two ideas so far have been to do a non-interactive rebase against base_branch with autosquash but there are some challenges there as the autosquash only applies commits marked with fixup; and the other idea would be doing a hard reset of the base branch on the local feature branch and then squash merging the remote feature branch so we can direct merge to the base branch. We're open to ideas here as well.

[codenamev]

Ok, so we discovered that GH allows us to handle this via API with the squash option. Bitbucket doesn't.

If you're feeling up to it, we'll need a refactor of the codebase here to handle the discrepancies. I think we should have a merge method on PullRequest. It will need to do everything that exists in the merge_feature_branch helper and just replace the commands with calls to GH. The existing merge_feature_branch should continue to live on in a new merge method in the BitBucket::PullRequest so we can maintain backwards compat with Bitbucket.

Either way this is outside the scope of this PR. I'm going to close this for now. If you decide to tackle it, just re-open and re-title the PR to reflect more accurately the change.

[pboling]

@codenamev Github does not squash merge anything. Github's green merge button is:

git checkout <destination_branch>
git merge --no-ff <branch_name>

[codenamev]

@pboling they recently introduced a feature that allows you the option to squash merge with the green merge button. The link I provided in my previous comment shows how to accomplish this with their API. This is the only way to squash merge and have GH label the PR as merged instead of closed as they only allow you to update the state to open or closed via API.

[pboling]

Awesome, so we can do that via the API then. :+1:

[simonzhu24]

@pboling Should we reopen this PR and I can work on this?

[pboling]

@simonzhu24 You can reuse the branch, or create a new one, and create a new PR when ready.