I just saw Refactor connectionmanagement by guusdk · Pull Request #416 · igniterealtime/Openfire · GitHub
which was merged to master unreviewed only a few minutes after the PR has been created.
**56 commits, **+18,913 **** −15,828 lines changed!
WTF! Not even squashed. The git history is a mess.
dwd just recently stated, that “All PRs now require sign-off from two other committers.” and now this.
I am confused! Can we please improve this and don’t merge messy PRs like this to master?
Sorry about that - that’s my mess. In fairness: I’ve been announcing the merge for quite a while, requested reviews frequently with very little response. I should have left the PR itself stand out longer though.
There was a substantial amount of discussion and testing, but the PR doesn’t reflect that.
As for squashing, that’s personal preference and it doesn’t worry me much.
Short answer: It’s not as bad as it looks.
dwd, you rejected LDAP-Setup Starttls connection fix by sourceindex · Pull Request #304 · igniterealtime/Openfire · GitHub
because it’s a mess, but honestly, it looks harmless compared to #416. It’s unreviewable and nobody knows what it does.
squashing: IMO, it’s fine to have multiple commits, if they contain distinct and complete work, like separation between UI, plugins, core. But this one contains a lot of “Merge leftovers”, “Removed a temporary work-around used during development.”-like commits. IMO, these shouldn’t go to master.
I was only really surprised and in doubt about the “commit policies” you were trying to establish: two committers’ sign off for each PR (which I think is a good idea).
I mean, we’ve rejected simple PRs in the past, just because they contained two many whitespace changes.
I appreciate your attempts to establish some commit policies, Flow proposed it as well hereFuture of Openfire development
It would be nice, if we can stick to them.
Guus: Even if your PR stood out longer, you cannot expect a code review for +18,913 −15,828 lines changed. Nobody will ever do this. Maybe consider smaller and squashed PRs for the future.
I find these 10 tips for better Pull Requests quite useful.
But this one contains a lot of “Merge leftovers”, “Removed a temporary work-around used during development.”-like commits. IMO, these shouldn’t go to master.
I fully agree with CSH here.