jquery / contribute.jquery.org

Developer documentation common to jQuery projects
https://contribute.jquery.org
Other
25 stars 76 forks source link

Prefix commit subjects with semver level #130

Closed gibson042 closed 8 years ago

gibson042 commented 8 years ago

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 https://github.com/jzaefferer/commitplease/issues/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 https://github.com/jzaefferer/commitplease/issues/44 for figuring out how to express them.

Krinkle commented 8 years ago

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 commented 8 years ago

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

mgol commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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. https://github.com/jquery/jquery/issues/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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

+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 commented 8 years ago

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 https://github.com/jquery/contribute.jquery.org/issues/130#issuecomment-153384425 .

gibson042 commented 8 years ago

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 commented 8 years ago

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

dmethvin commented 8 years ago

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.