[QGIS-Developer] QGIS repository management

[Nyall Dawson]

On Tue, 17 Oct 2023 at 03:41, Sandro Santilli <strk at kbt.io> wrote:
>
> On Mon, Oct 16, 2023 at 09:59:35AM +1300, Nyall Dawson via QGIS-Developer wrote:
>
> > If you flip the situation, you'll see that yes, you do have trust!
> >
> > - a complete stranger CANNOT approve their own changes
> > - a complete stranger CANNOT approve other stranger's changes
> > - a complete stranger CANNOT approve an approved member's changes
> >
> > vs
> >
> > - an approved member CANNOT approve their own changes
> > - an approved member CAN approve a complete stranger's changes
> > - an approved member CAN approve a another approved member's changes
>
> That's partial trust. I'm trusted to be able to judge someone else's
> work but not to judge my own work !

Ok, it's partial trust. What's wrong with that? I wouldn't trust
myself not to make accidental mistakes, or do something inefficient,
or that I'm not duplicating code which is present elsewhere in QGIS. A
second set of eyes over a pull request definitely reduces that risk.
QGIS is MASSIVE, and none of us can keep a complete picture of the
impact of code changes in our heads.

Some real world examples, picked at semi-random (because they are recent):

https://github.com/qgis/QGIS/pull/54941 : Submitted by a core
developer. I reviewed, and it looked good to me. But then a third (!)
set of eyes over the code has revealed a massive potential performance
regression caused by the change (thanks Even!).

https://github.com/qgis/QGIS/pull/54670: Submitted by a very
experienced core developer. Code looks good at first pass, no visible
bugs. But it's introducing a partial duplication of another utility
function implemented elsewhere and exhaustively tested. The review
identified that and as a result we avoided introducing an unnecessary,
non trivial duplication of code.

That's two examples. I'd very conservatively estimate that 1 in 10
approved PRs submitted by core committers have at least one set of
changes suggested and implemented. Let's (pessimistically!) write half
of those off as non-bugs, such as optimisation suggestions or changes
relating to documentation/comments/code style. That's still **AT
BEST** 1 in 20 pull requests submitted by core committers which needed
fixes.

In short: I'm more than OK with only partial trusting everyone. We ALL
make mistakes, so let's stick with the processes we have which reduce
the risk of these mistakes hitting end users..

Nyall


>
> Beside the same-company reviews things, consider I could always ask a
> friend to file PRs for the sole purpose of me being able to merge them
> in (I cannot merge my own...). The policy is flawed to me.
>
> --strk;