Feedback for squash merges

Issue #13633 resolved
Abhin Chhabra
staff created an issue

This is a placeholder ticket to gather feedback for the squash merge labs feature. The reason for a separate feedback ticket is to not spam people in the original ticket who just want the feature but don't want to be involved in further discussion.

squash-merge.png

What currently works

  • Pull request merge dialog contains a dropdown that can be used to choose between having a merge commit and just squashing all your commits on top of the destination.
  • The default commit message changes based on how one wants to do the merge. In the case of squash merges, we use a combination of the commit messages of all the commits being squashed. Obviously, the user should probably edit this commit message, but the default is a good start.

Known missing functionality

  • The merge dialog in the branch details page, branch list page and compare page does not yet support squashing. These will be rolled out in the coming weeks. (done)
  • Mercurial support for squash merges is planned, but will be implemented after all the merge dialogs are fixed. (we're planning on supporting obsolescence markers for this, so it'll be split into a separate beta feature later).
  • API support is missing.
  • Public documentation has not been updated yet (obviously this should be the last task).

Comments (54)

  1. Stephen Rosenthal

    Thanks a lot for adding this!

    Some feedback about feedback: I tried the "Give feedback" button the Labs settings but it doesn't work for me on this or the other two beta features I have enabled.

  2. Bassam Khouri

    I enabled the feature in BitBucket Labs, but I don't see the option in my PR. The Merge strategy doesn't exists for PR's that have been created prior to enabling the lab feature.

  3. Abhin Chhabra staff reporter

    @Bassam Khouri: Enabling the feature in labs should enable it for all PRs, even if they were created before enabling the feature. On the other hand, it will not enable merge strategies for PRs in Mercurial repositories (yet). Can you please confirm that you're dealing with PRs in Git repositories before I investigate further?

  4. Maxim Novikov

    +1 for forcing fast forward merges. I believe for lots of people who use "rebase" strategies to keep the repositories linear and clean, it's a much more important feature. Non-FF and squashing don't really bring anything to the table in our case.

  5. Stephen Rosenthal

    Some feedback after actually using the feature:

    1. Once the PR is squash merged, we see "There are no changes" under the commits tab. It would be nice for reference to keep either the original unsquashed commits (ideal) or the final squashed commit (OK as well).
    2. My teammate expected the default commit message for the squash-merge to combine the messages from the individual commits before they were squashed, like you get by default in git rebase -i. If we could see the original commits in the PR, I wouldn't mind this as much.
  6. Stephen Rosenthal

    Hmm, I just read "What currently works". As I noted above, my teammate reported that the default squash merge message did not contain the messages from the original commits. I haven't tested this part specifically... maybe he is wrong.

  7. Michiel van Baak

    It would be very nice if the commit message makes mention of the PR that has been merged. Now we loose all this info (the commit message does indeed list all the seperate commits, the same as the default contents of the description box on a pr)

    Thanks.

  8. Garret Wilson

    Why does Bitbucket capitalize my branch name in the squash commit message? If my branch name is "foo/bar", I expect "foo/bar" to be in the commit message, not "Foo/bar". Why is Bitbucket modifying my identifiers?

  9. Garret Wilson

    I want to second that "There are no changes" isn't helpful. Of course there were changes. Maybe there is no only a single commit, but there were changes. I hope there were changes!

  10. Abhin Chhabra staff reporter

    Hi all,

    I'm just finishing up the mercurial support for this. After that's done (hopefully in a couple of days), I'll start iterating on the default commit message and look into the commits list and see what I can do about it.

  11. Kush Kella

    We have limited people in our organization who can merge a PR, so every time one of us merges a PR whose author is someone else, the name of the actual author just goes away. e.g. if A submitted the PR and B merged it, B becomes author of the final commit. When I do the same in GitHub, it keeps the author of PR on the final commit. Thoughts?

  12. Abhin Chhabra staff reporter

    I just wanted to add an update to the mercurial support work:

    We decided to support obsolescence markers for Mercurial. So the squashed commit would be declared the successor of the commits from the feature branch. Because this change has important caveats for pushes/pulls, we will be moving the mercurial support into its own beta trial. The Git support will move to general release before the Mercurial support.

    I'll be continuing work on the Mercurial support in the beginning of January.

  13. Abhin Chhabra staff reporter

    @Stephen Rosenthal you're correct about the commits tab issue. It's been reported by many people and it looks like it needs some work. I'll be looking into it today.

    The default squash commit message should include all the individual commit messages.

  14. Abhin Chhabra staff reporter

    @Michiel van Baak you're right about the default commit message not containing the PR details. That's an oversight on my part and I'm glad you pointed that out. I'll be starting work on that issue immediately after posting all these replies. The changes wouldn't get deployed till January unfortunately (today is my last day before my holiday vacation 😄).

  15. Abhin Chhabra staff reporter

    @Garret Wilson I basically reused the method that generates the default PR description. But you make a good point about the capitalization. Maybe I can kill two birds with one stone and fix that issue in the PR description as well. I'll keep you posted on this.

  16. Abhin Chhabra staff reporter

    @Kush Kella That's a good observation. But I would argue that the author fields should be based on the user who took the action to create the squash commit. After all, it is indeed that user who decided to change the repo history.

    Regardless of my thoughts, I'll bring this up with the PMs in the team and see what they think.


    edit: I did talk to a PM on the team and they agree with basing the author field on the user who takes the action. For a slightly different point of view, consider how the author field would be populated if the feature branch contains commits from multiple authors. Would we pick the author from the head commit? Or the author with the most number of commits? Seems pretty unpredictable. The only safe option seems to be the acting user.

  17. Abhin Chhabra staff reporter

    @Dominik Bartholdi The goal was to replicate the behavior of "git merge --squash". It is indeed getting rid of the unnecessary merge commit that a lot of people want squash merging for.

    Currently, we have no plans to support a feature such as the one you described. For now, you may just have to squash the feature branch commits locally and force push, if you want such a merge.

  18. Garret Wilson

    Maybe I can kill two birds with one stone and fix that issue in the PR description as well.

    Great! We really appreciate your work on this --- even if we are severely frustrated and disappointed with Atlassian for leaving this to languish for so long. Thank you.

  19. Garret Wilson

    edit: I did talk to a PM on the team and they agree with basing the author field on the user who takes the action.

    So if I have a trainee who makes 100 commits and finally gets it right, and I merge+squash his/her pull request, it shows that I am the author of the commit!?? That's not good at all!! We absolutely need to show who authored the code.

    For a slightly different point of view, consider how the author field would be populated if the feature branch contains commits from multiple authors. Would we pick the author from the head commit? Or the author with the most number of commits? Seems pretty unpredictable.

    But that's not a good argument. You're saying you don't know what to do in a situation that probably accounts for a small percentage of use cases. But that's no reason not to do the behavior that we know is correct and expected for the overwhelmingly most common use case: that where all the commits are by a single person.

    So pick the author of the HEAD commit. Or pick the most common author. Or pick a random author from the pull request. It doesn't matter --- any of those options will give you the correct, expected answer 99% of the time. By your reasoning, because you might be wrong 1% of the time, you instead choose an implementation that is guaranteed to give you the incorrect answer 99% of the time. I don't get that reasoning.

  20. Kush Kella

    @Abhin Chhabra I agree with Garret. Whether you select the person who submitted the PR or the person who took the action to merge the PR, both have the same problem. Neither of them is a safe bet. In large organizations everyone is not supposed to merge PRs. That is so error prone. I think using the submitter of the PR is way more accurate then the person who took the action. If you really want to solve the problem when there are multiple authors in the commit history, you should come up with a UI/UX that solves it. But the submitter of the PR should be the author of the final commit.

  21. Garret Wilson

    We have a trainee who went to a lot of work on his first big project. He made a lot of extra commits getting it right, so I wanted to try out the new merge+squash option. I had no idea that by using this option I was essentially erasing his attribution of all his hard work, and making it look like I made the changes. Now I'm going to have to do the whole rebase/change history/force push stuff I wanted to avoid in the first place just to get his authorship back on that commit. If that's the way it works, I won't be using it.

    Update: I just looked at the squashed commit. It lists me as the committer. Good. But it lists me as the author, and I am not the author. This completely messes up blame, as well we throws away attribution for our trainee.

    But to make things worse, we already have people branching master, so for me to fix this (using rebase/amend/whatever) it's going to really cause some problems for the whole team. The new "squash" is causing me more problems that simply having the developers do manual squashes on their branch before merging.

    This is a blocker issue. I will not use this is if discards the author information.

  22. Jean-Frédéric

    I just enabled this feature for my team, and it does not appears on my previously opened PRs. I’m using Git repos.

    I echo above concerns − keeping author information is a must.

  23. Abhin Chhabra staff reporter

    I have some updates:

    PR commits tab bug

    A PR to fix the commits tab issues with squash merges has been merged. After a squash merge, the commits tab on a PR will now show the squashed commit as its sole commit. This change will be deployed to production in the next deploy (which should happen early next week).

    Default commit message improvement

    A PR to change the default squash commit message has also been merged. This one got merged a bit earlier this week, so it should be going out to production much sooner (hopefully today). The change brings the PR title, PR number and source branch into the header of the default commit message.

    Squashed commit authorship concerns

    I thought a lot about how to address this one. I think it would be fair to set the committer field to the requesting user (the one who hit the merge button) and the author to the author of the PR itself (even though the PR's source branch could contain commits by other people).

    The problem with this approach is that currently Bitbucket doesn't do a very good job of surfacing the author/committer difference in its UI, because that's a very infrequent occurrence. So we'll also have to go through all the places in Bitbucket where commit authorship is surfaced. This greatly expands the scope of the work needed.

    So we're going to go through with a general release of squash merge first, before looking into this issue further. In the mean time, we'll be adding some instrumentation to see how pervasive this issue really is.

  24. Abhin Chhabra staff reporter

    @Jean-Frédéric This feature affects the requesting user and not the repo owner. That is, it'll show up when you try to merge a PR if it is enabled for you (not the team). Try going back to the labs page and enable it for your personal account (instead of the team).

  25. Karol Tyl

    @Abhin Chhabra thanks for the great work you've done so far. Unfortunately, we have exactly the same concerns as reported by @Kush Kella and @Garret Wilson, and similarly as for them, for us it's also a case that caused us to not use the new feature in our company until it keeps the author of the changes made on the feature branch.

    Looking forward for updates.

    Best regards! Karol

  26. Thomas Turrell-Croft

    @Abhin Chhabra I would just like to second the comments by @Karol Tyl et al.

    The problem with any issue like this is that most people simply move on without bothering to comment that it is an issue for them so it's very difficult to gauge opinion.

    Sadly my team is going to have to continue to use merge --no-ff until the author and committer question is resolved.

    Personally I do not care so much about what the BitBucket UI displays because I use git on the command line or other UI tools. The BitBucket UI could be updated later from my perspective.

  27. Scott Buchanan

    I second Thomas (won't let me tag him?). Unless you're saying that the BitBucket web UI actually displays commits with different committers/authors incorrectly, then the UI clarifications shouldn't hold up the fix to the actual squash merge functionality. By switching the functionality to correctly set the committer & author sooner, any squash merges done in the mean time will eventually show correctly. By waiting, you're forcing all intermediate squash merges to remain incorrect.

  28. Abhin Chhabra staff reporter

    I acknowledge the importance of differentiating between the author and committer for squash merges. I also acknowledge that we could actually set the fields in the squashed commit correctly and we can worry about displaying it correctly in the UI later.

    That being said, we think that since the majority of authors are also ones who do the merging, we can release squash merges in the current state and update squash merges to differentiate between the author and committer in the coming weeks. With that in mind, I'll be resolving the squash merge feature after the general release, but keep this ticket around until the author/committer differentiation issue is sorted out.

  29. Garret Wilson

    we think that since the majority of authors are also ones who do the merging

    If that is true, then what will it hurt if you pick any of the authors of the merged commits as the merge author? That way you will choose the "right" answer for 100% of the people who use it the way you say they use it, and the "right" answer 95% of the users who are merging for another team member who did all the commits.

    The way you are choosing to do it now will choose the "right" answer for 100% of the people who use it like you think they use it, but will get the "wrong" answer for 100% of the people who use it to merge requests of another team member (i.e. it will be the "right" answer 0% of the time).

    So I don't get it. Why choose an approach that you know will get the wrong answer for a large portion of users, when choosing the approach we have all requested will still get the right answer in the situation you claim is most common?

    Will we now have to wait years again just to get this "really fixed"?

  30. Karol Tyl

    we think that since the majority of authors are also ones who do the merging

    Can you share some stats supporting this statement? Because from my experience most common approach is using pull request as the opportunity for doing a code review and person who accepts the code also merges it to the main branch. Also, there are branches where only certain people (ie senior devs) can push. In such case your argumentation is invalid.

    Best regards

  31. Abhin Chhabra staff reporter

    Thank you for the feedback everybody. While I'm unable to share any data without going through some internal process, it's unnecessary to do so, since I'll be starting work on the author/committer bugfix this week.

    Just to reiterate, I'll be updating the feature so that the PR creator is marked as the author of the commit and the user who hits the merge button as the committer. As mentioned earlier, I do not have any roadmap time for surfacing that distinction in the UI very well, but fortunately, the feedback here has indicated that that's an acceptable compromise. I'm grateful for that feedback.

  32. Karol Tyl

    @Abhin Chhabra Thanks for the update, looking forward to have it implemented.

    Meanwhile I've noticed one thing that I don't like too much As explained in my previous posts, current implementation is not acceptable, so I decided to disable the feature in Bitbucket settings in order to prevent people from accidentaly using the feature that might be breaking for our workflow. Today I've noticed, that it became active again and I'm not able to deactivate it. squash.png

    Would it be possible to make this option selectable again?

    Regards, Karol

  33. Abhin Chhabra staff reporter

    @Karol Tyl: Thank you for the feedback. The reason why you were unable to deactivate it is because we've begun the rollout of the feature to all the users. The disabled toggle button indicates that the feature has been turned on for you.

    I apologize for the inconvenience. We'll be fixing the author/committer issue soon. Until then, I appreciate your patience.

  34. Andreas Jonsson

    When you use squash merge and then try to delete the branch from your local branches inside our SourceTree i get this error:

    git -c diff.mnemonicprefix=false -c core.quotepath=false -c credential.helper=sourcetree branch -d dynamic-dimension-tables 
    error: The branch 'dynamic-dimension-tables' is not fully merged.
    If you are sure you want to delete it, run 'git branch -D dynamic-dimension-tables'.
    Completed with errors, see above
    
  35. Scott Buchanan

    Andreas, that's just normal Git behavior: nothing to do with BitBucket. Since the squashed merge doesn't actually contain the original commits, you'll get that error. You have to enable "Force Delete": it should be an option in the delete UI.

  36. Abhin Chhabra staff reporter

    The change that sets the author/committer fields of the squashed commit correctly has been deployed. Here's a screenshot of a commit (viewed locally) that was squashed in a Bitbucket pull request. As indicated earlier, the UI lacks the ability to distinguish between the author/committer correctly (for now).

    2017-02-10-13-42-32.png

    On that note, I'm going to close this ticket. Any further issues with squash merging should be mentioned in new tickets dedicated to those issues.

  37. Log in to comment