On 08/06/20 22:05, Artem Shchygel wrote:

Hello Mike,

Thanks for your feedback.

There are several levels of intent behind our proposal.
The one that you mentioned (search and identify commits by certain tag attribute) is important, but not the only one. Moreover one of your examples where you can have both "fix" and "feat" tag in the same commit is exactly the thing we're trying to avoid.

I find that regrettable, because in some (rare!) cases, a commit would
really deserve both tags.

And I also want to emphasize the notion that short tag in subject line must be mandatory.

To me personally adding short tag in the subject line coupled with removing "Package/Module" and CVE references from it serves the following purposes:
1. Mandatory tag forces committer to explicitly state what commit is about. This should prevent mixed commits (like bug fix and improvement together)

I agree this is a good general principle; it's always good to make a
patch author think explicitly about the nature of their patch, and to
make them capture the outcome of that thought process.

But this does not need to be encoded in the subject line, plus see my
point above about the (infrequent) case when a patch is both a fix and a
feature.

2. Removing "Package/Module" reference allows commits across packages boundaries where it makes sense. It also allows reviewer to see changes in their logical context instead of package-boundary context. Now I'm not advocating for huge commits, just commit partitioning better be on logical level than package boundary level.

We've managed very well to split up patches *both* at logical boundaries
*and* at package boundaries, at the same time. What you advocate for in
this point is 100% correct for a self-contained open source project, one
that explicitly disregards forks (such as the Linux kernel); I don't
think it's a good fit for edk2 though.

3. Removing CVE reference as well as "Package/Module" reference frees up the space in the subject line for more meaningful messages. Moreover your example shows why having CVE refs in subject line is bad - what if one commit fixes several of them.

This is a good point.

I'm in favor of including CVEs in subject lines (they are extremely
important, and also exceptional). But including multiple CVE numbers in
the subject is indeed a tough nut.

4. Placing short tag in the subject line allows for commits without long description, that can be created from the command line without the need of firing up the editor, as opposite to adding "Package/Module" ref and named tag inside the commit body. For mandatory things ergonomic matters

As a reviewer, I reject patches with empty or meaningless message
bodies, as a norm (for the packages that I co-maintain / co-review), and
as a community member, I protest against such patches for other packages
too, if I notice them on the list.

A commit message that consists of a subject line and a Signed-off-by,
plus (perhaps) some mechanic boilerplate included from a template, is
entirely useless. The message on a commit is nearly as important as the
code in the commit; sometimes *much more* important. It's entirely
normal to write a full page of rationale for a one-liner code change.

I've been on a crusade for years to train community members to write
more expressive commit messages. Your point#4 is a huge red flag to me,
personally.

All in all we understand that following our proposal to the letter will create disruptions for maintainers/reviewers workflow while simplify things for committers/users. But we still believe this is change for the better.

Unfortunately, in any open source project that professes "no patch
merged without review", the bottleneck is *always* on the
reviewer/maintainer side, and not on the contributor / consumer side.

Any change that makes things harder for reviewers / maintainers will
decrease code quality and long-term coherence (regressions!), and/or
worsen review feedback (such as response time), and/or erode commitment
to the above principle ("no patch merged without review").

Thanks
Laszlo