Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Introduce new one ACK carve-out rule #2627

Merged
merged 3 commits into from Apr 1, 2024
Merged

Introduce new one ACK carve-out rule #2627

merged 3 commits into from Apr 1, 2024

Conversation

tcharding
Copy link
Member

@tcharding tcharding commented Mar 22, 2024 •
edited

Update merge carve-out policy and introduce new rule.

  • Patch 1: Fix stale test script mention
  • Patch 2: Merge current carve-outs into a single carve-out with multiple rules
  • Patch 3: Introduce new carve-out rule

From patch 3:

    Introduce new one-ack carve out rule
    
    Our merge process is being artificially slowed down because of a
    combination of:
    
    - Using merge-commit merging means PRs often have to be rebased with no
      changes but a different merge base (and force pushed).
    - Trivial changes, like fixing nits, are often force pushed also.
    - Force pushes invalidate ACKs
    - Our devs are spread around the world working at different times
    
    What this means is trivial force pushes often cause multi day delays in
    merging. To try and alleviate this problem introduce an additional rule
    to the One ACK carve-out so that Andrew can merge PRs that have
    previously been ack'ed by another dev and have only minimal changes.
    The definition of "trivial" is subjective which introduces a burden on
    Andrew to not merge stuff willy-nilly but also allows simple changes to
    the original PR (eg fixed nits that the original reviewer suggested).

@tcharding
Update test script mention
ebf5b67 
In the One ACK carve out just say "test scripts" instead of `test.sh`
because we re-named the test scripts recently.
@tcharding
Merge Refactor and One ACK carve outs
42d02fb 
The "One ACK carve-out" has 3 rules and then there is a separate
"Refactor carve-out" that covers things that are not only refactoring -
this makes it hard to reference the carve-outs in github because its a
bit confusing.

Merge the carve-outs into a single "one ACK carve-out" with multiple
rules. Use rule 0 for the original refactor carve-out stuff because it
makes the diff smaller and all good lists start with 0.

Also remove mention of the refactor carve-out from rule 3.
@github-actions github-actions bot added the doc label Mar 22, 2024
@coveralls
Copy link

coveralls commented Mar 22, 2024 •
edited

Pull Request Test Coverage Report for Build 8494297684

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 83.225%
Totals Coverage Status
Change from base Build 8474502277: 0.0%
Covered Lines: 19066
Relevant Lines: 22909
💛 - Coveralls

@tcharding
Copy link
Member Author

At the risk of seeming self important, policy changes like this require a few eyes, @stevenroose, @RCasatta, @sanket1729, @Kixunil, @TheBlueMatt, @elichai, @apoelstra - can a few of you guys please review to make sure I am are not pushing through unwanted policy.

@tcharding tcharding mentioned this pull request Mar 22, 2024
Merged
sanket1729
sanket1729 previously approved these changes Mar 22, 2024
View reviewed changes
Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 234f444. I believe this will boost velocity.

apoelstra
apoelstra previously approved these changes Mar 24, 2024
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 234f444 SGTM. Unsure whether it should be just me, but I guess I am the one with the local CI box...

RCasatta
RCasatta previously approved these changes Mar 24, 2024
View reviewed changes
Copy link
Collaborator

@RCasatta RCasatta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 234f444

Kixunil
Kixunil reviewed Mar 30, 2024
View reviewed changes
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK policy change but I believe there's a typo. IDK if you want to force push and apply the policy right away. ;)

CONTRIBUTING.md Outdated
@@ -165,8 +165,17 @@ any of the following conditions:
submodule and re-exporting them from the original module. Must not include
any code changes except to import paths. Requires absolutely no change to the
public API.
4. PR as previously had two ACKs, had minimal changes, and gets a single ACK
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

has previously had?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, lol. as sorta works in a flowery way puts on monocle "I do say sir, a PR as [one which has] had previously two ACKs, shall get a single ACK".

But I think you're right that it was supposed to be has, which can be read in a normal tone.

I think it'd be fine to fix the typo and then one-ACK merge this, since the typo doesn't really affect the meaning, and for process changes it's really just concept ACKs that we're looking for.

@tcharding
Introduce new one-ack carve out rule
9b70c65 
Our merge process is being artificially slowed down because of a
combination of:

- Using merge-commit merging means PRs often have to be rebased with no
  changes but a different merge base (and force pushed).
- Trivial changes, like fixing nits, are often force pushed also.
- Force pushes invalidate ACKs
- Our devs are spread around the world working at different times

What this means is trivial force pushes often cause multi day delays in
merging. To try and alleviate this problem introduce an additional rule
to the One ACK carve-out so that Andrew can merge PRs that have
previously been ack'ed by another dev and have only minimal changes.
The definition of "trivial" is subjective which introduces a burden on
Andrew to not merge stuff willy-nilly but also allows simple changes to
the original PR (eg fixed nits that the original reviewer suggested).
@tcharding tcharding dismissed stale reviews from RCasatta, apoelstra, and sanket1729 via 9b70c65 March 31, 2024 00:12
@tcharding
Copy link
Member Author

Woops, fixed s/as/has/ - no other changes.

apoelstra
apoelstra approved these changes Mar 31, 2024
View reviewed changes
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 9b70c65 confirmed via range-diff that the commit everyone ACKed and this one differ only in as vs has

@apoelstra apoelstra merged commit 6c4f5df into rust-bitcoin:master Apr 1, 2024
168 checks passed
@tcharding tcharding deleted the 03-23-policy branch April 1, 2024 22:58
@apoelstra apoelstra mentioned this pull request Apr 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kixunil Kixunil Kixunil left review comments

@apoelstra apoelstra apoelstra approved these changes

@RCasatta RCasatta RCasatta left review comments

@sanket1729 sanket1729 sanket1729 left review comments

Assignees
No one assigned
Labels
doc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tcharding @coveralls @Kixunil @apoelstra @RCasatta @sanket1729