Review process: force push should not require re-approval

[Laczen]

Introduction

Due to changes the review process requires a re-approval after a branch has been force pushed. This should not be required. Even after a approval the reviewer is kept up to date with any changes that are applied before it is merged, so if a change requires new approval a reviewer can request changes.

New rules have been established for maintainers and contributors, there seems to be a unbalance in the rights and obligations for maintainers.

Problem description

These changes to the review process are slowing down the process of PR merging. The number of PR's that are waiting for approval seem to be increasing rapidly and this can't all be assigned to the growing of the community and zephyr.

A unbalance in the rights and obligations leads to frustration and unallowable power abusone by maintainers.

Proposed change

a. Remove the requirement for a PR to be re-approved after a force push. I can't imagine that there have been many problems in the past where a PR was merged before a reviewer has seen an acted upon the changes. Like it is now nearly 100% of the PR's are influenced while there might have been a problem in 1% (or even less) of the PR's.

b. Add obligations for the maintainers:
- react to a PR in a timely fashion (within a week),
- provide clear remarks whether a PR fit's in their global view of how the system should evolve,
- communicate their global view of the system (where it should go in the long run),
- do not allow maintainers to block PR's for futile reasons,
- if maintainers are unable to respond for whatever reason they should assign their task to another maintainer,

[gmarull]

+1

[carlocaione]

+1

[erwango]

+1

For authors, current situation is just like building card castles.
And for reviewers, this begins to be counterproductive as now I found myself trying to limit the level of my remarks on PRs that are in review for a long time and are starting to get approvals.

I'm noting that #41626 ("Device const qualifier check and fix"), was finally merged with 2 approvals (after 3 months of review and ~ 20 reviewers). I'm not sure this situation is better than the previous one.

[carlescufi]

+1

Without hard data to back my statement up, my impression after 2 weeks of the option being enabled is that the new measure is not achieving its desired goal. Reviewers just click on +1 automatically without actually re-reviewing, which means that we've added overhead without any realistic gains.

[gmarull]

I'll personally stop wasting my time re-approving PRs that just got rebased because there was a conflict, a CI failure or some sort of typo. I'll do that because there's no value on doing it apart from getting things in, so to blindly add +1 we have scripts/bots. I feel there's a disconnection between the reality and people that voted for this option. I'm convinced that a vote weighted by most active reviewers would not have ended in this result.

[marc-hb]

@Laczen did you close this issue following some conversation somewhere (minutes?) or because you just gave up?

Like it is now nearly 100% of the PR's are influenced while there might have been a problem in 1% (or even less) of the PR's.

This is the best summary. ALL submitters have an extremely strong incentive NOT to act on "optional" suggestions and to stick to just "good enough" code and commit messages - otherwise they lose all approvals and a LOT of time.

Maybe this generally lower quality is the price to pay to avoid JiaT75-like attacks?

Reviewers just click on +1 automatically without actually re-reviewing, which means that we've added overhead without any realistic gains.

... or not even.

[marc-hb]

Reviewers just click on +1 automatically without actually re-reviewing, which means that we've added overhead without any realistic gains.

Github has a very convenient "New changes since you last viewed" button:

• Show changes since last review microsoft/vscode-pull-request-github#363

However, that button is not compatible with the force-push based workflow:

• Process: investigate GitHub code review replacements #39194

That button merely shows newer commits: it assumes 1 "main" commit per PR and that all other commits are fixups. That's the typical Github Workflow but not the "traditional git" workflow.

The "force-pushed from... to ... " sentence is clickable and very useful to review recent changes but... only when the submitter did NOT rebase - because Github does not implement git range-diff (#39194).

Rebasing is now officially discouraged: https://docs.zephyrproject.org/latest/contribute/contributor_expectations.html#workflow-suggestions-that-help-reviewers but most submitters have no idea what "range-diff" even means. Most people just rebase without thinking.

We're screwed.

[Laczen]

@marc-hb Just gave up...