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

DOCS(git): Add PR and merge commit guidance to commit guidelines #6391

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Kissaki
Copy link
Member

@Kissaki Kissaki commented Apr 10, 2024

Implements #6385

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

The GitHub repository can be configured to merge with PR title + (#<nr>) + desc by default, which would follow these suggested rules.

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
@Hartmnt
Copy link
Member

Hartmnt commented Apr 10, 2024

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

It looks like it does, but it does not. The problem was that the regex match groups are enumerated based on the original regex string and not the number of groups actually matched. Therefore the script fails. I have fixed this before when creating the last RC, but I sadly somehow dropped the Mumble repo folder this change was in...

I will fix this again, when I draft the next RC

Copy link
Member

@Hartmnt Hartmnt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said I do not mind what kind of message merge commits get either way, so this gets a "neutral" approval from me 😄

@Kissaki
Copy link
Member Author

Kissaki commented Apr 10, 2024

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

It looks like it does, but it does not. The problem was that the regex match groups are enumerated based on the original regex string and not the number of groups actually matched. Therefore the script fails. I have fixed this before when creating the last RC, but I sadly somehow dropped the Mumble repo folder this change was in...

I will fix this again, when I draft the next RC

I don't quite follow. Does this PR change influence whether or in what way the script breaks or fails? Or is the script broken after the same way it was broken before? Within the (broken) script context, does the regex not match PRs of the specified form? (Is this PR sound within its context?)

@Hartmnt
Copy link
Member

Hartmnt commented Apr 10, 2024

I don't quite follow. Does this PR change influence whether or in what way the script breaks or fails? Or is the script broken after the same way it was broken before? Within the (broken) script context, does the regex not match PRs of the specified form? (Is this PR sound within its context?)

The script is and was broken independent of this PR. It fails in subtle ways, that will become apparent once I post a PR to fix it. So don't worry about it for now. Just note that Our scripts/generate_changelog.py script pr_number_pattern already supports PR numbers in the form (#<nr>) is only true in theory :)

@Kissaki
Copy link
Member Author

Kissaki commented Apr 13, 2024

@Krzmbrzl @davidebeatrici Are you fine with this change?

Merge commits would no longer be Merge PR #123: TYPE(scope): Title but TYPE(scope): Title (#123).

Once approved and merged, I would/will change the repo setting so that is the default (no need for manual title and desc changes on merge).

@davidebeatrici
Copy link
Member

Sounds good to me, consistency in PR merge titles was broken a while ago anyway.

On a separate note: the Rebase and merge method should be considered, but in our case it may be useful to bring the PR's title and message into the commit history (i.e. keep the merge commit behavior).

@Krzmbrzl
Copy link
Member

I would prefer having an explicit Merge appear in merge commits as they behave very differently during e.g. rebasing and I would like to know if I'll get issues while rebasing by a quick glance at git log.

@Kissaki
Copy link
Member Author

Kissaki commented Apr 19, 2024

I would prefer having an explicit Merge appear in merge commits as they behave very differently during e.g. rebasing and I would like to know if I'll get issues while rebasing by a quick glance at git log.

I don't see a good solution for that. If we want the merge commits to follow commit format we either

  • use PR titles that follows commit format, use the corresponding default, but have to manually add "merge" (error-prone)
  • use PR titles that follows commit format, require adding "merge" to the PR title, use the corresponding default
  • use the traditional default, require adjusting merge commit title (error-prone)

What's your concerns on rebase or otherwise? Rebase at least can preserve merge commits. (Which is not the default (it's omission) and not necessarily obvious or convenient.)

@Krzmbrzl
Copy link
Member

What's your concerns on rebase or otherwise?

Counting commits doesn't work across merge commits (for e.g. git rebase HEAD^n)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants