-
Notifications
You must be signed in to change notification settings - Fork 2.7k
chore(ci): reject uncanonicalized issue link in commits #15936
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
base: master
Are you sure you want to change the base?
Conversation
The triagebot documentation doesn't make it clear why we are using this setting and the other two links have a lot of content in them for other topics. Could you update the PR description with the motivation for this, summarizing the relevant parts of those conversations? Also, how close are we for being ready for subtrees? Should this be one of the later things we do so we don't take on costs when we aren't getting the benefits? |
66f9f44
to
0eb7ca7
Compare
This is needed if we want to turn cargo submodule into a subtree in the rust-lang/rust repository (rust-lang#15882), because commits messages will be copied over as-is, and the magic keyword like `fixes #99999` may unintentionally close issues in rust-lang/rust while it was intended only for rust-lang/cargo issues. Unlike `check-commits = false`, instead of rejecting any issue links in commit messages, the `"uncanonicalized"` option still allows issues in a canonicalized form, such as * `https://github.com/rust-lang/cargo/issues/15882` * `rust-lang#15882` but the bare issue number like `rust-lang#15882` will be warned by triagebot in a PR comment. ### Drawbacks It is bad because contributors will be forced to write commit messages in a specific style. Worse if we decide not to be a subtree and then this change puts unnecessary works on contributors. ### Is it the last step towards rust-lang#15882? Personally I consider not. Because merge commit still contains the PR number in the uncanonicalized form, as we use GitHub merge queue and it hasn't yet support [merge commit template][1]. However, this change on its own makes issue link more obvious and less ambiguous, especially when reading from outside GitHub web UI. It also stops us from adding more uncanonicalized issue links in the future, which we already have too many in the past commits [2] For discussions, see * https://forge.rust-lang.org/triagebot/issue-links.html * https://rust-lang.zulipchat.com/#narrow/channel/246057-t-cargo/topic/Turning.20cargo.20into.20a.20subtree/near/537951273 * rust-lang#15882 [1]: https://github.com/orgs/community/discussions/5955 [2]: https://rust-lang.zulipchat.com/#narrow/channel/246057-t-cargo/topic/Turning.20cargo.20into.20a.20subtree/near/537560661
0eb7ca7
to
11ea9ab
Compare
Updated. Let me know if it is still unclear. |
The part related to the motivation that remained unclear was I didn't know that subtrees cherry pick all commits. That seems noisy for rustc but oh well, thats the choice they make. |
What does this PR try to resolve?
This is needed if we want to turn cargo submodule into a subtree in
the rust-lang/rust repository (#15882), because
commits are cherry-picked with their messages left as-is, and the magic keyword
like
fixes #99999
may unintentionally close issues in rust-lang/rustwhile it was intended only for rust-lang/cargo issues.
Unlike
check-commits = false
, instead of rejecting any issue linksin commit messages, the
"uncanonicalized"
option still allows issuesin a canonicalized form, such as
https://github.com/rust-lang/cargo/issues/15882
rust-lang/cargo#15882
but the bare issue number like
#15882
will be warned by triagebotin a PR comment.
Drawbacks
It is bad because contributors will be forced to write commit messages
in a specific style. Worse if we decide not to be a subtree and then
this change puts unnecessary works on contributors.
Is it the last step towards #15882?
Personally I consider not. Because merge commit still contains
the PR number in the uncanonicalized form, as we use GitHub merge queue
and it hasn't yet support merge commit template.
However, this change on its own makes issue link more obvious and
less ambiguous, especially when reading from outside GitHub web UI.
It also stops us from adding more uncanonicalized issue links in the
future, which we already have too many in the past commits 2
There are some other concerns in #15882 (comment) and #15882 (comment). Until we resolve them and feel this is the only thing left, we might not want to merge this.
For discussions, see