Pull request merging

[adangel]

We currently have this documentation: https://docs.pmd-code.org/latest/pmd_projectdocs_committers_merging_pull_requests.html

Questions, thoughts about this:

• Is this still up to date?

  • From my point of view: partly:
  • I don't update the .allcontributorsrc while merging, I've put this as a step to the release script

• Release notes: Updating the release notes is cumbersome. It would be easier for me as a maintainer, if the release notes would be ready already, so that I don't need to change the PR with an additional commit before merging.

  • If the PR is complete (e.g. incl. release notes), we could more easily use the "merge buttons" on GitHub
  • Merge conflicts most often appear in the release notes. Maybe there is a different way to handle this? (e.g. a separate release note file for each entry corresponding to issue/pr. All files are merged when the release notes are rendered)

• Currently, we do a merge commit. This results in a potential complex git history, which might contain multiple merges. We could decide to do a squash commit instead.

  • linear history
  • easier revert in case we need to undo something
  • looses the information about how a solution has been achieved, but maybe we don't need this?

[adangel]

Another question:

• when and who approves a PR?
• when do we close a PR because of missing responds?

[adangel]

• how many approvals do we need to merge a PR?
• should we enforce this (see GitHub's protected branch rules)?

[UncleOwen]

I don't think we have the manpower to require multiple approvals.

[adangel]

About linear history / squash merging: What I currently do for the dependabot PRs - after verifying that they really only change the version and that the build ran successfully - I do a squash commit via the GitHub Web UI. This adds then the version change on top of the main HEAD without any unnecessary merge commits.
By the way, these dependabot PRs are labeled with "dependencies" and are collected during the release for generating the release notes. Note: we still need to assign a milestone to these dependabot PRs.

How to deal with dependabot PRs is also missing in the documentation currently.

[UncleOwen]

• Release notes: I did a few of them today and am already annoyed by them. I feel you. Is there a way we can auto-generate to "closed issues" section? I believe that would help a lot.
• merge vs squash: Isn't "rebase and merge" the best of both words? Linear history, keeps the "how was this done", and semi-simple to revert (multiple commits, but at least they are easy to find and all right next to each other).

[UncleOwen]

We already require all issues to start with '[java]' etc. We could require all future issues to include the rule category as well, and group the changelog entries using these tags. Would that work?

[UncleOwen]

One changelog file per PR sounds like a great idea to reduce merge conflicts!

[UncleOwen]

But my question is more: Do we need to know "how it was done"? Isn't the problem more, that some changes are huge and not properly split up into manageable chunks? So - I would rather vote for rejecting PRs if they are "too big" (whatever this means) and if they tackle two things at the same time (not focused on one problem). That requires a lot of discipline...

The more I think about it, the more I like it.

One minor caveat: I think squash and merge uses the person who pressed the button as the "owner" of the squashed commit, right? This would cause git blame to show the wrong names. Is there a way to keep the original PR author there (at least if a PR was authored by just one person)?

[adangel]

We already require all issues to start with '[java]' etc. We could require all future issues to include the rule category as well, and group the changelog entries using these tags. Would that work?

That would be an option. We have a lot of labels already (https://github.com/pmd/pmd/labels) - but we could introduce new ones, to group issues for release notes.
Instead of adding the prefix "[java]" we could add a label, and we could introduce labels for each rule category. We have a label already for false-positive/false-negative, but I tend to add this to the issue title additionally - but we could also have this in the release notes as a sub-header or just add it automatically then from the label.

One minor caveat: I think squash and merge uses the person who pressed the button as the "owner" of the squashed commit, right?

I think, the author of the squash commit is the creator of the PR - the committer is github. It might be, that if the PR consists of multiple commits with multiple authors, that github manages to mention them as "Co-authored-by: ". I think, the original PR author is already preserved. At least, for the dependabot PRs, it is preserved.

[UncleOwen]

I just tried with #6748. You're right, it's the name of the person creating the PR.

Shall we give squash+merge a try?