mumble icon indicating copy to clipboard operation
mumble copied to clipboard

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

Open Kissaki opened this issue 10 months ago • 8 comments

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.

Kissaki avatar Apr 10 '24 16:04 Kissaki

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

Hartmnt avatar Apr 10 '24 17:04 Hartmnt

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?)

Kissaki avatar Apr 10 '24 20:04 Kissaki

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 :)

Hartmnt avatar Apr 10 '24 20:04 Hartmnt

@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).

Kissaki avatar Apr 13 '24 09:04 Kissaki

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).

davidebeatrici avatar Apr 13 '24 12:04 davidebeatrici

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.

Krzmbrzl avatar Apr 16 '24 17:04 Krzmbrzl

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.)

Kissaki avatar Apr 19 '24 15:04 Kissaki

What's your concerns on rebase or otherwise?

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

Krzmbrzl avatar Apr 19 '24 16:04 Krzmbrzl

Given there's been no solution suggestion to Krzmbrzls concern nor discussion I'll close this as unresolved/unresolvable.

I guess we'll continue with no guideline or documented intention then.

Kissaki avatar May 31 '24 12:05 Kissaki