Disable merge commit option and use "squash and merge"

[tsalo]

I apologize if this has already been discussed, but I couldn't find anything in the issues. My request is to disable the "Create a merge commit" option for merging PRs in the repository, and to make "Squash and merge" the default. The reason I'm proposing this is that the commit history when using merge commits is unreadable, while for squashed commits the commit message is the title of the pull request.

For example, the commit message for merging #490 with merge commit is

Merge pull request #490 from franklin-feingold/master

while the message for that PR with squash and merge would have been

[ENH] Add 'Commenting on a PR' to CONTRIBUTING.md (#490)

[sappelhoff]

Would be fine by me 👍

We'd have to adjust the conditional clause triggering the changelog bot:

bids-specification/.circleci/config.yml

Line 153 in af7414b

if (git log -1 --pretty=%s | grep Merge*) && (! git log -1 --pretty=%b | grep REL:) ; then

[tsalo]

Something like the following should work, right?

if (! git log -1 --pretty=%s | grep '\[DOC] Auto-generate changelog entry') && (! git log -1 --pretty=%b | grep REL:) ; then

[sappelhoff]

collecting some opinions on this rather technical request: @yarikoptic @effigies

[effigies]

I'm okay with this, and that seems roughly right. There's not much penalty to getting it wrong, though.

[yarikoptic]

I am afraid (didn't check) that with a squash we might negatively effect accountability of contributions, in particular if a PR was prepared by multiple contributors. Also it might confuse contributors who are not that verse in git who would not see their branch merged when looking at the history e.g. in gitk. Overall I think there is no silver bullet, but I do not feel strongly.
If there is a tech issue with change log generator -- it should be solved "technically" ;)

[tsalo]

We use squash and merge in tedana and have been pretty happy with it. Multi-contributor PRs show up as by all contributors in the commit history, so that's alright, but I don't use command-line git all that much and I don't know what drawbacks there are for git users.

[effigies]

It really depends how big the PR is. If you might want to see the specific rationale for a particular line, instead of "which PR was this", you're going to lose that. In GitHub you can see the "blame" for a file (example), and click on commit message to see that batch (example).

If the PRs are granular enough that having them collapsed into a single commit would be sufficiently informative, then that's fine.

It might make sense for the maintainers to decide which it is, and the submitter could request either a squash or a merge.

[tsalo]

@effigies, thank you for the clarification. I am used to working with packages that try to treat PRs, rather than commits, and the primary unit of change, but I see how that doesn't really work with large-scale PRs like BEPs. You definitely want to be able to access individual commits in those cases. I don't exactly love the merge commit messages, and I generally rely on squash-and-merge to hide my awful commit messages as I work through something (rather than revising my history), but I can definitely see why that's how it's set up for this repository.

Given how the changelog updater is set up, I don't know if it would be possible to support both merge commits and squash-and-merges, so I'm happy to close this.

Thanks everyone for discussing this.

[sappelhoff]

@tsalo

Given how the changelog updater is set up, I don't know if it would be possible to support both merge commits and squash-and-merges, so I'm happy to close this.

Yes, the changelog generator is triggered only on "Merge Commits" that do not contain "REL" in their description.

But if we don't trigger it on a given PR due to e.g. using "Squash" instead of "Merge Commit", then that's not a problem, because in the next PR using a "Merge Commit", the previous "Squash Commit" PR will be parsed and added as well.

So we CAN and should make use of "Rebase and Merge" and "Squash and Merge" whenever it makes sense (at the discretion of the person merging the PR)

The only caveat is that the last PR merged before a release MUST be merged with a "Merge Commit", to make sure the GitHub Changelog Generator runs and picks up all PRs that might have not triggered a run due to the applied merge strategy.