Should adding time until auto merge per commit be removed

[IanFindlay]

I understand using the number of commits in a pull request as a metric for how complex the PR is and therefore how long reviewers should have to evaluate it but I'm not convinced it is a good idea. Here's some of my thoughts on why:

• Number of commits isn't a great indication of complexity as people develop in different ways: some like to be very granular with their commits while others like to code the entire feature before making a commit.

• It encourages, if not demands, squashes and rebases to get it down to 1 commit which masks the degree of complexity entirely.

• Squashes and rebases can be difficult to get right at first and it can be frustrating having to struggle with this so that an otherwise fine PR can be accepted in a reasonable amount of time. This might discourage future contributions.

• If the branch is already pushed online and someone else is working from it - perhaps it already implements a feature they want to build from but other aspects of the PR aren't done yet - messing around with the history via rebasing complicates things considerably.

Overall, I'm personally not sure that even if the number of commits was an accurate measure of how much time is needed to review the code that it would be worth it having considered and experienced some of the downsides; the fact that it doesn't actually indicate complexity reliably at all means that I think it should be removed.

Thoughts @TooAngel ?

[TooAngel]

I thought about this one, too.

Maybe I was originally thinking more of a perfect world, where changes of pull requests do not occur so often and more experienced git users. As I have a world driven config pull request in the pipeline, we could make use of that one (and test it). The additional time per commit can be changed already therefor set to 0.

The other thing with the number commits is, that your merge boost is calculated with the total number of commits on the repository. So the one with a lot of small commits would get a higher value. Github lets you decide how a Pull Request is merged and one option is to automatically squash all commits to one. We would need to look into that and make it configurable, too.

[IanFindlay]

The merge boost isn't something I considered and that certainly makes my opinion a little bit less clear. I'm going to have to think about this some more but the config PR being merged will certainly make experimentation easier.

[TooAngel]

Github provides three options to merge Pull Requests (https://docs.github.com/en/free-pro-team@latest/github/administering-a-repository/about-merge-methods-on-github)

• Merge pull request - A merge commit is created and the single commits stay as they are
• Squashing your merge commits - All commits are squashed to a single merge commit
• Rebasing and merging your commits - The Pull Request is rebased on top of master and the commits stay as they are

When we remove the number of commits from the time calculation, I would suggest using also Squashing your merge commits, which reduces the commits to one. This way everyone can provide as many commits as they like and they will end up as one.

The scenario where multiple commits are actually used to reflect the complexity of the Pull Request is currently rare (in form of never).

So my suggestion right now would be: Change the days per PR to 10 days, set the days per commit to 0, and change the merge option. Do you have any thoughts about the full time (10 days)? My thinking behind that was if you are interested in a project you should be able to at least look into it once a week and even if the first reviews (merge boost up to three days) come in you still can have about a week. In case more reviews come in you are already close to 50% approving, which should also be fine.

[IanFindlay]

I like the idea of squashing the commits on merge. It means that the master's history contains a high-level overview of the development and each commit has passed the CI tests - something that isn't the case if all of the commits are added on merge. I suppose the only downside is that small PR's, my current one for example, are 'worth' the same merge boost as a large, complicated one. But this was already kind of the case due to the rebasing and squashing.

I think the 10 days has been fine so far. This repository is heavily weighted towards you, for obvious reasons, and you're active enough where most merges are within two days. However...

How do you feel about the auto merge time becoming zero after a certain number of approvals are reached? For example, in this repository at the moment if you and I both like a PR there is no-one else likely to review it and suggest changes. Waiting hours for this PR's timer to tick down seems a little redundant to me. A configurable setting to set a auto merge threshold might be good. It could be based upon the person with the biggest merge boost power (you in this repository) so that no one person can merge a PR immediately.

[TooAngel]

Hm, ...

From a quality perspective, I would do that. With world driven I'm getting tempted to get things merged and deployed quickly, but that shouldn't be the goal. The idea of the time is to give people time to properly review the pull request, which should lead to better quality. So in bigger projects, the review can also indicate, 'let's give us a bit more time discuss implementation details', instead of the binary merge/don't merge decision.

I think the issue lies somewhere else. Currently, each commit has a high impact, I think > 1 hour, so all contributors matter. In more mature projects with thousands of commits, this looks different. Also, the world driven awareness of the contributors is not yet there. So I would first try to improve the awareness and get a bit more experience on this repository with more commits.

I think such a configuration can still make sense and we could implement it as a config flag. I wouldn't enable it for this repository right now.