Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include Merge Commits in Commit Guidelines #6385

Open
Kissaki opened this issue Apr 7, 2024 · 6 comments
Open

Include Merge Commits in Commit Guidelines #6385

Kissaki opened this issue Apr 7, 2024 · 6 comments

Comments

@Kissaki
Copy link
Member

Kissaki commented Apr 7, 2024

Our Commit Guidelines in COMMIT_GUIDELINES.md https://github.com/mumble-voip/mumble/blob/master/COMMIT_GUIDELINES.md guide for individual change commits, but not merge commits.

Current Practice

Currently, it seems like we have a mixture of merge commit titling

  • Merge PR #<id>: <MR/change title>
  • Merge pull request #<id>: <MR/change title>
  • Merge pull request #<id> from <thirdparty/branch>

Suggestion 1: Simple, unquestionable definition

Consolidating current practice into a defined rule of preference is simple, and doable without much discussion.

  1. Define that Merge PR #<id>: <change title>
  2. <change title> must conform to change commit guidelines

Suggestion 2: More elaborate uniformation; changes current practice

Define that

  1. Merge Request describes a changeset [of one or more commits]
  2. MR titles must be titled according to commit guidelines
  3. A merge commit shall be the MR title + description, with the title appended with a (PR #<id>) suffix
<MR title> (PR #<id>)

<MR desc>

Reasoning

I would prefer

  1. Consistent merge commit naming
  2. Concise PR over pull request
  3. Concise (PR #<id>) suffix over Merge PR #<id>: prefix [1]
  4. Merge commit follows commit conventions/guidelines - Merge commit has same form as other commits
  5. MR title becomes merge commit title + MR id ref suffix (PR #<id>), and MR description becomes merge commit description

[1]: Not only is it more concise, IMO it is also more consistent in what commits represent. An individual change commit title describes the change. Why does a merge commit describe a git operation?

My work-preference, which I have good experiences with, is a merge commit representing the entire changeset of the one or multiple commits, or equivalent to what the GitHub merge request is and does.

In cases where the MR consists of only one commit, the merge request and merge commit essentially duplicate information. Which I think is acceptable. The explicit merge commit clearly, formally indicates that it was/is a MR. A reasonable alternative is allowing merging merge requests as single commits on the target branch, adding a MR reference to the commit title. (I like the conciseness and cleanness of that approach. It evades the information duplication and redundancy at the cost of not seeing tree-blocks in git history for every merge request changeset.)

Generating Changelogs

IIRC we have a script to generate a changelog, also to prepare releases. I don't know if that is indeed and still the case. But I have to assume the workflow is currently lacking because of "dirty" merge request names.

Having conforming merge request names allows generating a clean and fully useful changelog from the first-parent commits (normal commits + merge commits - excluding merged-changeset-commits). Having consistent TYPE allows categorizing them.


RFC @Krzmbrzl @davidebeatrici @Hartmnt

@Krzmbrzl
Copy link
Member

Krzmbrzl commented Apr 7, 2024

We indeed have a changelog script and it should still work fine.
And effectively the implicit rule always has been (at least to my understanding) to do

Merge PR xyz: PR title

where a PR's title follows the commit guideline.

Explicitly documenting that seems like a good idea though

@Hartmnt
Copy link
Member

Hartmnt commented Apr 7, 2024

We indeed have a changelog script and it should still work fine.

It does not. I had to hotfix the regex for the last release. I just forgot to create a merge request...

@Hartmnt
Copy link
Member

Hartmnt commented Apr 7, 2024

By the way, I was always using whatever GitHub pre-populated the text area with. I have no strong feelings on how to format merge commits one way or the other. Is there a way in GitHub to set a default message for this as there is in GitLab?

@Kissaki
Copy link
Member Author

Kissaki commented Apr 7, 2024

By the way, I was always using whatever GitHub pre-populated the text area with. I have no strong feelings on how to format merge commits one way or the other. Is there a way in GitHub to set a default message for this as there is in GitLab?

In the repo settings the default merge commit fill can be changed.

Not to the same degree as in GitLab (default choice or custom template). But we can set it to use MR title or MR title + MR description.

https://github.blog/changelog/2022-08-23-new-options-for-controlling-the-default-commit-message-when-merging-a-pull-request/#options

@Hartmnt
Copy link
Member

Hartmnt commented Apr 8, 2024

In a repo where every commit is a concise "atomic" logical change, I do not care one bit about merge commits. So please someone else have the final word on this.

(I could live with a fast forward only repo ducks and runs away)

@Krzmbrzl
Copy link
Member

Krzmbrzl commented Apr 9, 2024

In a repo where every commit is a concise "atomic" logical change, I do not care one bit about merge commits

That's also true. Still, I feel like for consistency's sake it'd be nice if merge commits would more or less follow the same formatting rules as other commits 🤷

Kissaki added a commit to Kissaki/mumble that referenced this issue Apr 10, 2024
Implements mumble-voip#6385

Our `scripts/generate_changelog.py` script `pr_number_pattern` already supports PR numbers in the form `(#<nr>)`.

The GitHub repository can be configured to merge with PR title + (#<nr>) + desc, which follows this form.

[1]: https://github.com/mumble-voip/mumble/blob/56f03e8d7e5f9cf9d1a318d3a1858db4e09c06ab/scripts/generate_changelog.py#L22-L24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants