Add tidy check to deny merge commits #105058

Noratrieb · 2022-11-29T16:00:14Z

This will prevent users with the pre-push hook from pushing a merge commit.

Exceptions are added for subtree updates. These exceptions are a little hacky and may be non-exhaustive but can be extended in the future.

I added a link to @jyn514's blog post for the error case because that's the best resource to solve merge commits. But it would probably be better if it was integrated into https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy, then we could link that instead.

r? @jyn514

src/tools/tidy/src/no_merge.rs

jyn514 · 2022-11-30T15:54:26Z

I added a link to @jyn514's blog post for the error case because that's the best resource to solve merge commits. But it would probably be better if it was integrated into https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy, then we could link that instead.

@Nilstrieb do you have time to add this to the dev guide?

src/tools/tidy/src/no_merge.rs

klensy · 2022-12-06T12:34:08Z

And encountered again at #104195, so it will be good to have this lint.

jyn514

The git handling looks good to me (or at least, good enough it shouldn't delay this longer), but I think I agree with Mark we should run this in CI. Can you either:

change this to fail silently locally, but give a hard error if git fails in CI (see

rust/src/bootstrap/util.rs

Line 260 in dc30b92

pub fn current() -> CiEnv {

for how to detect it); or,
change this to give a hard error locally if git fails

I have a slight preference for the latter but I'm ok with either.

jyn514 · 2022-12-30T19:58:31Z

@bors r+

Thanks!

bors · 2022-12-30T19:58:32Z

📌 Commit 878af66 has been approved by jyn514

It is now in the queue for this repository.

bors · 2022-12-30T22:55:54Z

⌛ Testing commit 878af66 with merge 4839886...

…-only-bors-is-allowed-to-do-that, r=jyn514 Add tidy check to deny merge commits This will prevent users with the pre-push hook from pushing a merge commit. Exceptions are added for subtree updates. These exceptions are a little hacky and may be non-exhaustive but can be extended in the future. I added a link to `@jyn514's` blog post for the error case because that's the best resource to solve merge commits. But it would probably be better if it was integrated into https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy, then we could link that instead. r? `@jyn514`

bors · 2022-12-31T01:43:35Z

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing 4839886 to master...

…or-you-only-bors-is-allowed-to-do-that, r=jyn514" This reverts commit 4839886, reversing changes made to ce85c98.

Revert "Auto merge of rust-lang#105058 - Nilstrieb:no-merge-commits, r=jyn514" This reverts commit 4839886, reversing changes made to ce85c98. Fixes rust-lang#106232 (comment). r? `@jyn514`

rust-timer · 2022-12-31T03:16:40Z

Finished benchmarking commit (4839886): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.1%	[-2.1%, -2.1%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.1%	[-2.1%, -2.1%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

klensy · 2023-06-13T08:49:48Z

But this don't work? #99339 (comment)
Ahh, reverted.

Enable triagebot no-merges check Follow-up on rust-lang/triagebot#1704 ### Motivation Occasionally, a merge commit like rust-lang@cb5c011 makes it past manual review and gets merged into master. At one point, we tried adding a check to CI to prevent this from happening (rust-lang#105058), but that ended up [problematic](rust-lang#106319 (comment)) and was [reverted](rust-lang#106320). This kind of check is simply too fragile for CI, and there must be a way for a human to override the bot's decision. The capability to detect and warn about merge commits has been present in triagebot for quite some time, but was never enabled at rust-lang/rust, possibly due to concerns about false positives on rollup and subtree sync PRs. This PR intends to alleviate those concerns. ### Configuration This configuration will exclude rollup PRs and subtree sync PRs from merge commit detection, and it will post the default warning message and add the `has-merge-commits` and `S-waiting-on-author` labels when merge commits are detected on other PRs. The eventual vision is to have bors refuse to merge if the `has-merge-commits` label is present. A reviewer can still force the merge by removing that label if they so wish. ### Note for contributors The rollup tool should add that label automatically, but anyone performing subtree updates should begin including "subtree update" in the titles of those PRs, to avoid false positives. r? infra ## Open Questions 1. This configuration uses the default message that's built into triagebot: > There are merge commits (commits with multiple parents) in your changes. We have a [no merge policy](https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy) so these commits will need to be removed for this pull request to be merged. > > You can start a rebase with the following commands: > ```shell-session > $ # rebase > $ git rebase -i master > $ # delete any merge commits in the editor that appears > $ git push --force-with-lease > ``` Any changes to this are easy, I'll just have to add a `message` option. Should we mention the excluded titles in the message?

Rollup merge of rust-lang#114157 - pitaj:triagebot_no-merges, r=ehuss Enable triagebot no-merges check Follow-up on rust-lang/triagebot#1704 ### Motivation Occasionally, a merge commit like rust-lang@cb5c011 makes it past manual review and gets merged into master. At one point, we tried adding a check to CI to prevent this from happening (rust-lang#105058), but that ended up [problematic](rust-lang#106319 (comment)) and was [reverted](rust-lang#106320). This kind of check is simply too fragile for CI, and there must be a way for a human to override the bot's decision. The capability to detect and warn about merge commits has been present in triagebot for quite some time, but was never enabled at rust-lang/rust, possibly due to concerns about false positives on rollup and subtree sync PRs. This PR intends to alleviate those concerns. ### Configuration This configuration will exclude rollup PRs and subtree sync PRs from merge commit detection, and it will post the default warning message and add the `has-merge-commits` and `S-waiting-on-author` labels when merge commits are detected on other PRs. The eventual vision is to have bors refuse to merge if the `has-merge-commits` label is present. A reviewer can still force the merge by removing that label if they so wish. ### Note for contributors The rollup tool should add that label automatically, but anyone performing subtree updates should begin including "subtree update" in the titles of those PRs, to avoid false positives. r? infra ## Open Questions 1. This configuration uses the default message that's built into triagebot: > There are merge commits (commits with multiple parents) in your changes. We have a [no merge policy](https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy) so these commits will need to be removed for this pull request to be merged. > > You can start a rebase with the following commands: > ```shell-session > $ # rebase > $ git rebase -i master > $ # delete any merge commits in the editor that appears > $ git push --force-with-lease > ``` Any changes to this are easy, I'll just have to add a `message` option. Should we mention the excluded titles in the message?

rustbot assigned jyn514 Nov 29, 2022

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 29, 2022

This comment has been minimized.

Sign in to view

fbstj reviewed Nov 29, 2022

View reviewed changes

src/tools/tidy/src/no_merge.rs Outdated Show resolved Hide resolved

Noratrieb force-pushed the no-merge-commits-for-you-only-bors-is-allowed-to-do-that branch 2 times, most recently from 61699c2 to 117fd23 Compare November 29, 2022 19:29

inquisitivecrystal added the A-meta Area: Issues & PRs about the rust-lang/rust repository itself label Nov 30, 2022

jyn514 reviewed Nov 30, 2022

View reviewed changes

src/tools/tidy/src/no_merge.rs Outdated Show resolved Hide resolved

src/tools/tidy/src/no_merge.rs Outdated Show resolved Hide resolved

src/tools/tidy/src/no_merge.rs Outdated Show resolved Hide resolved

src/tools/tidy/src/no_merge.rs Outdated Show resolved Hide resolved

jyn514 reviewed Nov 30, 2022

View reviewed changes

src/tools/tidy/src/no_merge.rs Outdated Show resolved Hide resolved

Noratrieb force-pushed the no-merge-commits-for-you-only-bors-is-allowed-to-do-that branch from 117fd23 to 02ff160 Compare November 30, 2022 21:17

Mark-Simulacrum reviewed Nov 30, 2022

View reviewed changes

src/tools/tidy/src/no_merge.rs Outdated Show resolved Hide resolved

jyn514 mentioned this pull request Dec 14, 2022

x fmt should only format modified files by default #105688

Closed

jyn514 suggested changes Dec 14, 2022

View reviewed changes

jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 14, 2022

Noratrieb force-pushed the no-merge-commits-for-you-only-bors-is-allowed-to-do-that branch from 02ff160 to f87e7af Compare December 14, 2022 18:25

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 14, 2022

Noratrieb force-pushed the no-merge-commits-for-you-only-bors-is-allowed-to-do-that branch from d19587b to 83bab41 Compare December 14, 2022 18:42

rustbot added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Dec 14, 2022

Noratrieb force-pushed the no-merge-commits-for-you-only-bors-is-allowed-to-do-that branch from 83bab41 to 3276879 Compare December 14, 2022 18:57

Noratrieb removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Dec 14, 2022

Noratrieb force-pushed the no-merge-commits-for-you-only-bors-is-allowed-to-do-that branch from f341a3e to 0815bc9 Compare December 14, 2022 19:23

Noratrieb mentioned this pull request Dec 14, 2022

Run x test tidy sooner in mingw-check #105714

Merged

This comment has been minimized.

Sign in to view

Noratrieb force-pushed the no-merge-commits-for-you-only-bors-is-allowed-to-do-that branch from 0815bc9 to c6d6073 Compare December 23, 2022 11:09

This comment has been minimized.

Sign in to view

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 30, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 30, 2022

albertlarsan68 mentioned this pull request Dec 30, 2022

Create "suggested tests" tool in rustbuild #106249

Merged

jyn514 mentioned this pull request Dec 31, 2022

Rollup of 8 pull requests #106318

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 31, 2022

bors merged commit 4839886 into rust-lang:master Dec 31, 2022

rustbot added this to the 1.68.0 milestone Dec 31, 2022

This was referenced Dec 31, 2022

Add rust-lang CI environment check #102428

Closed

warn newer available version of the x tool #104552

Merged

Ezrashaw mentioned this pull request Dec 31, 2022

Dont use --merge-base during bootstrap formatting subcommand #106310

Merged

Noratrieb mentioned this pull request Dec 31, 2022

Deny merge in CI take 2 #106333

Closed

jyn514 mentioned this pull request Mar 17, 2023

Mock environment testing for bootstrap #102563

Open

pitaj mentioned this pull request Jul 4, 2023

add more config options for no_merges rust-lang/triagebot#1704

Merged

pitaj mentioned this pull request Jul 28, 2023

Enable triagebot no-merges check #114157

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tidy check to deny merge commits #105058

Add tidy check to deny merge commits #105058

Noratrieb commented Nov 29, 2022

This comment has been minimized.

jyn514 commented Nov 30, 2022

klensy commented Dec 6, 2022

jyn514 left a comment

This comment has been minimized.

This comment has been minimized.

jyn514 commented Dec 30, 2022

bors commented Dec 30, 2022

bors commented Dec 30, 2022

bors commented Dec 31, 2022

rust-timer commented Dec 31, 2022

klensy commented Jun 13, 2023 •

edited

Loading

Add tidy check to deny merge commits #105058

Add tidy check to deny merge commits #105058

Conversation

Noratrieb commented Nov 29, 2022

This comment has been minimized.

jyn514 commented Nov 30, 2022

klensy commented Dec 6, 2022

jyn514 left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

jyn514 commented Dec 30, 2022

bors commented Dec 30, 2022

bors commented Dec 30, 2022

bors commented Dec 31, 2022

rust-timer commented Dec 31, 2022

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

klensy commented Jun 13, 2023 • edited Loading

klensy commented Jun 13, 2023 •

edited

Loading