Reviewing the process for giving people commit rights
Hello folks, When someone is proposed to get commit access, currently a sponsor proposes it, the intended recipient contacts sysadmin, sysadmin reviews, and then asks the sponsor if it's okay. This process essentially only allows for sysadmin review, since the sponsor has already implicitly approved by virtue of being the sponsor. This caused a problem recently in KWin. A new contributor was given commit rights very soon after he appeared, and then immediately after that, he inappropriately merged a not-fully-reviewed an un-accepted merge request (https://invent.kde.org/plasma/kwin/-/merge_requests/1980). It seems that he did not have a sense of the cultural norms around committing to KDE repos, and giving him commit access was probably premature. I'd like to propose that we need to make the commit access review process open to review by more people so that we can flag issues like this sooner. Maybe kde-core-devel? Nate
Re: Reviewing the process for giving people commit rights
On 4/1/22 17:28, Nate Graham wrote: Hello folks, When someone is proposed to get commit access, currently a sponsor proposes it, the intended recipient contacts sysadmin, sysadmin reviews, and then asks the sponsor if it's okay. This process essentially only allows for sysadmin review, since the sponsor has already implicitly approved by virtue of being the sponsor. This caused a problem recently in KWin. A new contributor was given commit rights very soon after he appeared, and then immediately after that, he inappropriately merged a not-fully-reviewed an un-accepted merge request (https://invent.kde.org/plasma/kwin/-/merge_requests/1980). It seems that he did not have a sense of the cultural norms around committing to KDE repos, and giving him commit access was probably premature. I'd like to propose that we need to make the commit access review process open to review by more people so that we can flag issues like this sooner. Maybe kde-core-devel? Nate I think this case shows more a lack of communication towards the person in question what rights and responsibilities come with commit access rather than a problem with the current review process. In other words, other reviewers would likely not have prevented what has happened. It's hard for any kind of reviewer to know whether the person to be reviewed knows the social etiquette that comes with commit access. To summarize: I don't see a need to change how applications are reviewed, but perhaps there are steps we can integrate into the application process to communicate better the social etiquette that comes with commit access. Cheers Nico
Re: Reviewing the process for giving people commit rights
On 4/1/22 09:36, Nicolas Fella wrote: I think this case shows more a lack of communication towards the person in question what rights and responsibilities come with commit access rather than a problem with the current review process. In other words, other reviewers would likely not have prevented what has happened. If I had seen a review request to give this person commit access, I would have objected on the basis that he only had one merged patch and had just appeared very recently. It takes time in a community to learn that community's social etiquette. Nate
Re: Reviewing the process for giving people commit rights
On Friday, 1 April 2022 17:36:50 CEST Nicolas Fella wrote: > To summarize: I don't see a need to change how applications are +1 One of the things I saw as a mark of a welcoming and trusting community when I joined KDE was that everyone had direct push access to trunk (good old SVN). While this can lead to problems, I'd rather have a different (tooling-based - maintainer notifications when something is merged or pushed directly) solution for this, than changing one of the things that, for me, define the KDE community. Cheers, Ivan -- dr Ivan Čukić i...@cukic.co, https://cukic.co/ gpg key fingerprint: 8FE4 D32F 7061 EA9C 8232 07AE 01C6 CE2B FF04 1C12
Re: Reviewing the process for giving people commit rights
On 4/1/22 09:52, Ivan Čukić wrote: On Friday, 1 April 2022 17:36:50 CEST Nicolas Fella wrote: To summarize: I don't see a need to change how applications are +1 One of the things I saw as a mark of a welcoming and trusting community when I joined KDE was that everyone had direct push access to trunk (good old SVN). I agree, and I'm not proposing that we stop giving trustworthy people global commit access. I'm saying that we should do more review regarding who we give this access to and how long they've been in the community for, because it is quite a big privilege. Nate
Re: Reviewing the process for giving people commit rights
On Freitag, 1. April 2022 17:52:50 CEST Ivan Čukić wrote: > On Friday, 1 April 2022 17:36:50 CEST Nicolas Fella wrote: > > To summarize: I don't see a need to change how applications are > > +1 -+1 > One of the things I saw as a mark of a welcoming and trusting community > when I joined KDE was that everyone had direct push access to trunk > (good old SVN). Exactly. I very much agree with this. I very much prefer a low entry barrier for giving new people commit rights and I see no reason to change our current process just because a single push didn't follow our agreed standard. It's not as if a large proportion of new people would cause trouble, is it? How are new people supposed to learn our ways if they are not allowed to make errors? It's not as if people could do serious damage. In case of severe problems with a committer it's easy to revoke their commit rights and shouldn't be too difficult to undo the damage they have done. Regards, Ingo signature.asc Description: This is a digitally signed message part.
Re: Reviewing the process for giving people commit rights
El vie, 1 abr 2022 a la(s) 12:29, Nate Graham (n...@kde.org) escribió: > > Hello folks, > > When someone is proposed to get commit access, currently a sponsor > proposes it, the intended recipient contacts sysadmin, sysadmin reviews, > and then asks the sponsor if it's okay. This process essentially only > allows for sysadmin review, since the sponsor has already implicitly > approved by virtue of being the sponsor. > > This caused a problem recently in KWin. A new contributor was given > commit rights very soon after he appeared, and then immediately after > that, he inappropriately merged a not-fully-reviewed an un-accepted > merge request > (https://invent.kde.org/plasma/kwin/-/merge_requests/1980). It seems > that he did not have a sense of the cultural norms around committing to > KDE repos, and giving him commit access was probably premature. > > I'd like to propose that we need to make the commit access review > process open to review by more people so that we can flag issues like > this sooner. Maybe kde-core-devel? I would like to add that GitLab has made it *significantly* easier to contribute without commit access. Think about what it was like in Phabricator era (or even Reviewboard). Apart from the well known UX issues with submitting reviews and the arc tool, once something was approved, the author had to explicitly say they didn't have access, the approver had to manually grab the diff and commit it with the right author info, etc. Sometimes the reviewer would approve and wait for the author to commit it (without knowing they didn't have commit access) while the author waited for something to happen not knowing what was the next step. Now the author can just push to a fork and the reviewer can just click Merge. For a casual contributor, they could well continue submitting GitLab review requests forever, rather than this being necessarily a path towards being granted commit access in the future. Given that, if we have other reasons to change the process of granting commit access, I don't think it would be a significant barrier to contributions. I'm not necessarily *advocating* that we add more restrictions to granting commit access, I just want to point out that if we have to, we can now *afford* to do so. I don't think we could before GitLab. -- Nicolás KDE Sysadmin Team
Re: Reviewing the process for giving people commit rights
> I would like to add that GitLab has made it *significantly* easier to > contribute without commit access. Think about what it was like in > Phabricator era (or even Reviewboard). [...] As a contributor who used Invent with and without commit access, I agree with this statement. Commit access was more useful (in the sense of necessary) with Phabricator.
Re: Reviewing the process for giving people commit rights
On Freitag, 1. April 2022 17:36:50 CEST Nicolas Fella wrote: > To summarize: I don't see a need to change how applications are > reviewed, but perhaps there are steps we can integrate into the > application process to communicate better the social etiquette that > comes with commit access. I agree. The current process has worked with very very few issues for decades, and is a defining part of our community. Substantially changing that for a single prematurely merged MR seems hard to justify (and it's not like we wouldn't occasionally have cases of longer term contributors prematurely merging MRs either). Yes, Gitlab makes it easier to contribute without commit access. However, commit access isn't only needed for the merge button, it also enables collaborating with others on work branches in the main repository. For an occasional drive-by contributor that is usually not needed, but looking at the Gitlab activity of the contributor we are talking about here, it's not what I see there. That's a two month continued involvement in kwin, including working non-trivial changes in longer-lived branches. It of course doesn't hurt to occasionally reiterate the expectations and responsibilities, to (new) contributors and sponsors alike, but I don't see a failure or the process here. Regards, Volker signature.asc Description: This is a digitally signed message part.
Re: Reviewing the process for giving people commit rights
On Samstag, 2. April 2022 11:21:11 CEST Kevin Kofler wrote: > Nate Graham wrote: > > This caused a problem recently in KWin. A new contributor was given > > commit rights very soon after he appeared, and then immediately after > > that, he inappropriately merged a not-fully-reviewed an un-accepted > > merge request > > (https://invent.kde.org/plasma/kwin/-/merge_requests/1980). It seems > > that he did not have a sense of the cultural norms around committing to > > KDE repos, and giving him commit access was probably premature. > > Well, the question this calls for is why the merge request was still not > fully reviewed almost six weeks after submission. I would guess that that is > what the misunderstanding came from: the submitter most likely assumed that > the changes were fine given that there were no outstanding comments. (The > submitter did try to address those comments that you had in those six > weeks.) > > I should also point out that the complaints in Xaver Hugl's post-merge > review were all only formatting/whitespace, choice of comment sign, and > brace issues (with no effect on the end user at all), Several PIM libraries have clang-format pre-commit hooks that prevent formatting issues in the first place (and, occasionally, annoy me because the hooks also complain about formatting issues in unstaged/uncommitted code, e.g. temporarily commented out code where the commented out code is not correctly indented). Formatting is something no reviewer should have to waste brain energy on nowadays. Regards, Ingo signature.asc Description: This is a digitally signed message part.
Re: Reviewing the process for giving people commit rights
+1 Só far, simple solution and would reduce a lot work of reviewers. On Wed, Apr 13, 2022, 04:27 Ingo Klöcker wrote: > On Samstag, 2. April 2022 11:21:11 CEST Kevin Kofler wrote: > > Nate Graham wrote: > > > This caused a problem recently in KWin. A new contributor was given > > > commit rights very soon after he appeared, and then immediately after > > > that, he inappropriately merged a not-fully-reviewed an un-accepted > > > merge request > > > (https://invent.kde.org/plasma/kwin/-/merge_requests/1980). It seems > > > that he did not have a sense of the cultural norms around committing to > > > KDE repos, and giving him commit access was probably premature. > > > > Well, the question this calls for is why the merge request was still not > > fully reviewed almost six weeks after submission. I would guess that > that is > > what the misunderstanding came from: the submitter most likely assumed > that > > the changes were fine given that there were no outstanding comments. (The > > submitter did try to address those comments that you had in those six > > weeks.) > > > > I should also point out that the complaints in Xaver Hugl's post-merge > > review were all only formatting/whitespace, choice of comment sign, and > > brace issues (with no effect on the end user at all), > > Several PIM libraries have clang-format pre-commit hooks that prevent > formatting issues in the first place (and, occasionally, annoy me because > the > hooks also complain about formatting issues in unstaged/uncommitted code, > e.g. > temporarily commented out code where the commented out code is not > correctly > indented). > > Formatting is something no reviewer should have to waste brain energy on > nowadays. > > Regards, > Ingo >
Re: Reviewing the process for giving people commit rights
In all honesty formatting is something that should be dealt with automatically. I use clang-format as part of my IDE just so I don't have to think about formatting in addition to the actual functionality of the code I'm writing. Auto formatting as you type helps a lot in keeping code consistently formatted even when its largely one person working on the project. On Wed, Apr 13, 2022 at 8:38 AM Helio Chissini de Castro wrote: > +1 > > Só far, simple solution and would reduce a lot work of reviewers. > > On Wed, Apr 13, 2022, 04:27 Ingo Klöcker wrote: > >> On Samstag, 2. April 2022 11:21:11 CEST Kevin Kofler wrote: >> > Nate Graham wrote: >> > > This caused a problem recently in KWin. A new contributor was given >> > > commit rights very soon after he appeared, and then immediately after >> > > that, he inappropriately merged a not-fully-reviewed an un-accepted >> > > merge request >> > > (https://invent.kde.org/plasma/kwin/-/merge_requests/1980). It seems >> > > that he did not have a sense of the cultural norms around committing >> to >> > > KDE repos, and giving him commit access was probably premature. >> > >> > Well, the question this calls for is why the merge request was still not >> > fully reviewed almost six weeks after submission. I would guess that >> that is >> > what the misunderstanding came from: the submitter most likely assumed >> that >> > the changes were fine given that there were no outstanding comments. >> (The >> > submitter did try to address those comments that you had in those six >> > weeks.) >> > >> > I should also point out that the complaints in Xaver Hugl's post-merge >> > review were all only formatting/whitespace, choice of comment sign, and >> > brace issues (with no effect on the end user at all), >> >> Several PIM libraries have clang-format pre-commit hooks that prevent >> formatting issues in the first place (and, occasionally, annoy me because >> the >> hooks also complain about formatting issues in unstaged/uncommitted code, >> e.g. >> temporarily commented out code where the commented out code is not >> correctly >> indented). >> >> Formatting is something no reviewer should have to waste brain energy on >> nowadays. >> >> Regards, >> Ingo >> >
