Bug: Seems like BREAKING CHANGE is broken since it doesn't bump a MAJOR version

[sezaru]

If I got it right, adding a footer with BREAKING CHANGE: something should bump the MAJOR version, but it is actually bumping the MINOR version.

If I would guess, this is related to the same problem highlighted in the https://github.com/zachdaniel/git_ops/issues/57 where a single commit is being split into multiple ones. Meaning that a BREAKING CHANGE is actually never inside the Commit footer field

[zachdaniel]

yep, you're correct. Perhaps what we can do for now is make an explicit allowance for BREAKING CHANGE: ... in the commit parser, i.e the subject line of a "nested" commit can never be BREAKING CHANGE. That will at least fix the explicit BREAKING CHANGE commit issue.

[sezaru]

Since it seems that the multiple commit feature is broken, doesn't make more sense to rollback it since maybe other parts of the system are broken because of that change as-well? Or is that change big/hard to rollback?

[zachdaniel]

AFAIK, BREAKING CHANGE: is the only special prefix. Either way, I rely on that behavior regularly because I squash commits on all my projects. We can push to fix that separately after this I think. Or perhaps we publish a new major version that removes that behavior, and puts it behind an opt-in configuration.

[sezaru]

@zachdaniel actually, seems like the parser bug also makes creating releases with commits that have other footers creates a bunch of warnings.

For example, this commit:

feat: bla

TAGS: test_git_ops

Will generate the following commits:

[
  %GitOps.Commit{
    type: "feat",
    scope: nil,
    message: "bla",
    body: nil,
    footer: nil,
    breaking?: false
  },
  %GitOps.Commit{
    type: "TAGS",
    scope: nil,
    message: "test_git_ops",
    body: nil,
    footer: nil,
    breaking?: false
  }
]

Which will trigger the following warning when running a release:

Commit with unknown type in: feat: bla

TAGS: test_git_ops

Because it will consider TAGS a type and will not find it in the types map.

My workaround for now is to add this to my config:

types: [
    tags: [
      hidden?: true
    ]
]

I was wondering, do you think it would be hard to fix the parser?

[zachdaniel]

I don't know if its possible to fix the parser w/o removing the feature of allowing nested commit messages. There is no way to tell the difference between a "footer" and a nested commit. For now we should special case "BREAKING CHANGE" and "TAGS", IMO. After that we can do the opt-in behavior that I mentioned above, and explain in the docs:

"you can choose between having arbitrary footers, and allowing nested conventional commits, but not both"

[sezaru]

Do you remember which commit(s) added support for nested commit messages? I was thinking about taking a look at it this weekend and maybe adding the opt-in feature

[zachdaniel]

I don't recall, would have to hunt it down. Even with the opt-in feature, it will need to make exceptions for TAGS and BREAKING CHANGE so ideally that would get merged first, but its not necessarily something you have to do since you won't be using the multiple commits feature.

[marc0s]

I think the commit introducing support for nested commit messages is this one https://github.com/zachdaniel/git_ops/commit/8ee8f9859e04737e3104e0630a2388bb398161c5

I assume if it's there is because someone felt the need to have it, but as discussed, it's kinda incompatible with processing message footers like BREAKING CHANGE as spec'd.

For now we should special case "BREAKING CHANGE" and "TAGS", IMO. After that we can do the opt-in behavior that I mentioned above

I also think this is the way forward. @sezaru Did you manage to have a look at this?