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

Prefix commit subjects with semver level #130

Closed
gibson042 opened this issue Nov 2, 2015 · 23 comments
Closed

Prefix commit subjects with semver level #130

gibson042 opened this issue Nov 2, 2015 · 23 comments

Comments

@gibson042
Copy link
Member

TL;DR: Extend commit subjects to {semver-level} {Component(s)}: {Message}, e.g. minor Foo: Extend bar

I think we should include semver level in commit messages for ease of hotfixing/releasing/cherry-picking/etc. And since it seems to be even more important than the affected component(s), I'd like it not just in the subject but at the start of the subject. I've opened jzaefferer/commitplease#44 to extend the configurability of commitplease for making such changes easier, which serves double duty as my proposed format.

I'd like to reserve this issue for bikeshedding the details of the subject requirements, and jzaefferer/commitplease#44 for figuring out how to express them.

@Krinkle
Copy link
Member

Krinkle commented Nov 2, 2015

Node.js has been using more the verbose (SEMVER-MINOR) and (SEMVER-MAJOR) in their commit messages.

For example: https://github.com/nodejs/node/blob/v5.0.0/CHANGELOG.md#commits

Commits

  • [1a41feb559] - buffer: don't CHECK on zero-sized realloc (Ben Noordhuis) #3499
  • [5f6579d366] - (SEMVER-MAJOR) buffer: remove raw & raws encoding (Sakthipriyan Vairamani) #2859
  • [870108aaa8] - (SEMVER-MAJOR) console: sub-millisecond accuracy for console.time (Michaël Zasso) #3166
  • [680dda8023] - dns: remove nonexistant exports.ADNAME (Roman Reiss) #3051

@scottgonzalez
Copy link
Member

I don't really want to implement this in any of the projects I manage.

@mgol
Copy link
Member

mgol commented Nov 2, 2015

Node.js has been using more the verbose (SEMVER-MINOR) and (SEMVER-MAJOR) in their commit messages.

Click in the links you pasted, there is no SEMVER-* there. They seem to be retroactively added when constructing the changelog for a release, they're not included in commit messages.

@gibson042
Copy link
Member Author

Semver level is the most important detail of a commit, because it dictates the versions allowed to include it. That's why I want it in the subject. If we're worried about space, I'd advocate moving component(s) into the body first, since they take up even more (especially if a commit affects multiple) and seem most useful for changelog generation, which requires the full body anyway.

Regardless, though, git log [--grep] can support any rational scheme. The point is to start capturing this information, and I'd prefer to do so consistently (hence the issue).

@markelog
Copy link
Member

markelog commented Nov 3, 2015

This information could be useful for changelog generation btw.

I don't think you can validate all commits this way, like misc, docs, etc; if such changes are not released with the project, they pretty much have nothing to do with semver terminology and because it would complicate contributing process for the beginners even more - it seems this responsibility should be on the maintainer, like sign-off tags.

I also don't think it should be requirement for everyone, team of any project should decide for itself.

I also wouldn't refuse to see example of the project with similar guide line.

@dmethvin
Copy link
Member

dmethvin commented Nov 3, 2015

I see the point @gibson042 makes about the semver level being more important than the component. It's not hard to see what component(s) are affected by a commit but you really have to understand the code that was changed to know its semver impact. Still, the first line of the commit is already pretty crowded so I'd prefer to see this info in the commit body as @leobalter suggested.

I wonder how the Node folks determine the semver level, it doesn't seem to be described in the commit so I guess they must review and determine it ex post facto.

@jzaefferer
Copy link
Member

Semver level is the most important detail of a commit, because it dictates the versions allowed to include it.

Can you explain how to expect to make use of this information?

Without more information, I agree with Scott - the commit message seems to be the wrong place to make any decisions related to backwards compatibility. For both QUnit and jQuery UI we spend a lot of time planning gradual migrations to introduce new features and remove old, commit messages were never part of that process.

Since you mention cherry-picking, maybe this is very specific to Core, with two stable releases managed in the same repository?

@rxaviers
Copy link
Member

rxaviers commented Nov 3, 2015

FWIW, I liked the idea of including semver info in the commit message, but my vote goes in line with what Leo and Dave suggested: in the commit body.

@scottgonzalez
Copy link
Member

Since you mention cherry-picking, maybe this is very specific to Core, with two stable releases managed in the same repository?

There's honestly no difference between what Core does and what UI does in terms of managing multiple branches for parallel version releases. And api.jqueryui.com has an even larger spread of cherry-picking (historically it has been fairly common to cherry-pick commits across every version).

@leobalter
Copy link
Member

This semver idea is great. Flagging commits to tell my next version is great, and it involves a change log generation, where the message title can be prefixed, as in Node.

In the commits it can still reside at the body like I suggested earlier in a private email: "Semver: Major"

This is also very useful to determine my next npm version.

This message standard would be of a great value to QUnit and its release process.

@mgol
Copy link
Member

mgol commented Nov 3, 2015

@jzaefferer

Since you mention cherry-picking, maybe this is very specific to Core, with two stable releases managed in the same repository?

This has nothing to do with us having two parallel branches. This is a result of us wanting to not release jQuery Compat at all and instead release 1.12 & 2.2 that will have non-breaking changes backported from compat & master respectively (and stop cherry-picking to compat afterwards). We need to identify commits that introduced breaking changes so that we don't backport them.

@jzaefferer
Copy link
Member

We need to identify commits that introduced breaking changes so that we don't backport them.

Shouldn't it be obvious enough when reviewing a patch to land in master if it can also land in another branch? The odds of landing breaking changes in master without a lot of scrutiny should be extremely low anyway.

I currently don't see the advantage of annotating commits with this information, when its is not relevant anymore after the commit landed in one or more branches.

@mgol
Copy link
Member

mgol commented Nov 3, 2015

Shouldn't it be obvious enough when reviewing a patch to land in master if it can also land in another branch?

This is not the situation we're in. We already have ~300 commits on master that are newer than what's in the latest 2.1 release. They're there and some of them are breaking changes. We didn't pay that much attention as to which ones were breaking as we assumed they'll make it into jQuery 3.0.0 which will be a major release that's allowed to introduce breaking changes. Now we need to backport some (or most, to be decided) of those ~300 commits to jQuery 2.2.0 (and another ~300 commits from compat to 1.12.0) but without a way to quickly filter out the breaking ones we need to carefully examine every single one of them which is more time consuming.

@scottgonzalez
Copy link
Member

Honestly, you should be examining them anyway. I think it's fine to define a standard for projects that want to use it. Since commit guidelines aren't requirements for all projects anyway, I'm trying to mostly stay out of this discussion since I don't want to implement this in the projects that I manage. But I don't think auto-porting 300 commits without manual review is a sane thing to do even with this additional information.

@jzaefferer
Copy link
Member

This seems more of a workflow or process issue then.

We didn't pay that much attention as to which ones were breaking as we assumed they'll make it into jQuery 3.0.0 which will be a major release that's allowed to introduce breaking changes.

That sounds scary. I wouldn't want to deal with that at all, no matter what the commit message claims. Even with a 3.0.0 release, how are you dealing with migration issues? Are you working on an upgrade guide already?

@mgol
Copy link
Member

mgol commented Nov 3, 2015

Even with a 3.0.0 release, how are you dealing with migration issues?

We did pay attention when landing them, we have tickets for Migrate for specific changes (some already merged), the major changes were also mentioned in the 3.0.0-alpha1 blog post. We also have the "Behavior change" label although I see it's not applied to every issue that should have it, e.g. jquery/jquery#1722. (@jquery/core do you think we may have more of those?).

Anyway, this info is not attached to commits so just by looking at a commit you may not always immediately say how to qualify it, you'll have to read the description, sometimes maybe open a linked issue to see more details. The problem is that requires way more time than just cherry-picking every commit that doesn't have semver-level: major (or sth to that effect) in the commit description. Multiply that by 300 and take into account that we want to release those versions by the end of November...

FWIW, the fact that there are so many commits to take into account is why I've been skeptical to this idea of backporting almost every non-breaking commit to 1.12/2.2; it's just too much work, easy to overlook something and since there are even no plans of 1.12-rc.1/2.2-rc.1 releases, the probability of a fatal mistake is IMO too high. I'd prefer to just backport the most important bugfixes (but to be very conservative with that) plus those that were meant for compat only so that we don't have commits that will appear in no released versions (there are very few of those changes - see the list - and not everything on that list should be backported so it's not a problem).

@scottgonzalez
Copy link
Member

Having dealt with the UI 1.8/1.9 release phase, which contained an order of magnitude more commits than the Core 1.12/2.2/3.0 release phase, I'd be really skeptical of trusting this info for a proper decision on which commits to merge. Also, your concern about only backporting the most important bug fixes makes me think this info won't help you at all anyway.

@mgol
Copy link
Member

mgol commented Nov 3, 2015

Also, your concern about only backporting the most important bug fixes makes me think this info won't help you at all anyway.

Yes, in that case it's alright to analyze every single commit. @gibson042 wants to backport as much as possible, though and it's very hard to do it then.

@timmywil
Copy link
Member

timmywil commented Nov 3, 2015

+1 for the commit body. I'd be okay with it in the subject, but as @gibson042 pointed out, we can use --grep or similar to filter the logs.

@gnarf
Copy link
Member

gnarf commented Nov 3, 2015

The last project I was on we had some auto tagging release bots and we used
#patch #minor #major (and #pre...) To tell that bot how big of a bump
something would be.

The info was super useful.

I don't think we need to do this if we don't have automation for release,
but it could be useful to start doing and have jQuery release verify that
there aren't any commits that call for say a major or minor bump while we
are doing a patch release.
On Nov 3, 2015 9:13 AM, "Timmy Willison" notifications@github.com wrote:

+1 for the commit body. I'd be okay with it in the subject, but as
@gibson042 https://github.com/gibson042 pointed out, we can use --grep
or similar to filter the logs.


Reply to this email directly or view it on GitHub
#130 (comment)
.

@gibson042
Copy link
Member Author

It looks like there's a rough consensus in favor of having this information, but having it in the body. I'm tentatively prepared to move forward on SemVer: {Major|Minor|Patch}.

@leobalter
Copy link
Member

on a second thought, for consistency with npm version, we should use the values in all lowercases: {major|minor|patch}.

@dmethvin
Copy link
Member

We didn't act on this, but if we do it should probably be at a project level. I still like the idea of marking these somehow, outside the cramped first line.

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

No branches or pull requests