GeezerVison

No objection on the flag, but I'm not sure we will be able to add it to nodejs/node.
There are valid commits with metadata:

• backports
• cherry-picks of floating patches (on V8 for example)

We don't land backports with the CQ, so I don't think that would be problematic. For cherry-picks, I don't think the commit would contain PR-URL or Reviewed-by trailers, would they? If not, then this flag won't cause a problem since it's only checking for those two.

It does. At least for V8 patches, I always keep the metadata for the first time a commit landed. For example: nodejs/node@cc36db7

I see! So if we started to use that flag in Node.js, we would need to land those PRs manually. Would that disrupt your process?

I could land them manually.

I guess it would actually not apply for those, since the GHA commit linter is only validating the first commit message, and those floating patches are never first IIRC.

I guess it would actually not apply for those, since the GHA commit linter is only validating the first commit message, and those floating patches are never first IIRC.

They are on, e.g. nodejs/node#53997.

I guess it would actually not apply for those, since the GHA commit linter is only validating the first commit message, and those floating patches are never first IIRC.

They are on, e.g. nodejs/node#53997.

I added a test with the commit on the PR you linked, it still passes with the new flag.

Useful to ensure the commit message doesn't include metadata that should be added by automation (PR-URL and reviewers). In particular, we could use that in the commit linter in nodejs/node to report as invalid commits that include a PR-URL or Reviewed-by trailers so the CQ refuse to land such commit as having those manually entered is error prone.

We also add PR-URL to the release commits.. For most releases, the release commit won't be the first commit in the PR, but it will be for the LTS transition release. While we do not use the CQ on releases, it would be good to not have the commit linter fail for them.

We also add PR-URL to the release commits.

Sure but I don't think it's an objection to adding the flag, it sounds to me that's something to handle on nodejs/node side – i.e. the PR making use of this flag should make sure it's not applied to release proposals.

I think it'll be confusing that we'll have both --no-validate-metadata and --validate-no-metadata but I don't have a suggestion for an alternative name for the new option.

Come to think of it, that means the negation of the new option will be --no-validate-no-metadata 😆.
OTOH since --validate-metadata is true, are there valid scenarios to run --no-validate-metadata where the commit has metadata (i.e. could this be the negation of the existing option?)? 🤔

Thinking more about this, other options would be to:

• filter out trailers such as Reviewed by and PR-URL in NCU – so having wrong trailers in the PR has no impact in the commit that actually lands, and having them reported by core-validate-commit would not make sense.
• only report as error if the trailer value is incorrect (i.e. it's fine to have a PR-URL trailer as long as it matches the actual PR-URL) – not sure we could do that for Reviewed-by though, we would still have to chose whether to ignore, filter, or report.

If we do the latter, we can probably rename the flag to --validate-metadata-for-unmerged-pr or something like that

No longer needed IMO after nodejs/node-core-utils#1036