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.

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

[DevPolicy] Add guidance to not submit code or reviews on someone else's #69701

Merged
merged 2 commits into from Jan 27, 2024

Conversation

kbeyls
Copy link
Collaborator

@kbeyls kbeyls commented Oct 20, 2023

... behalf.

There should be few legitimate reasons for people not submitting code or review comments themselves. Furthermore, it helps avoid situations where banned individuals circumvent their ban by asking others to submit code or review comments on their behalf.

More details, see
https://discourse.llvm.org/t/submitting-code-or-review-comments-on-someone-elses-behalf/74269

... behalf.

There should be few legitimate reasons for people not submitting code or
review comments themselves. Furthermore, it helps avoid situations where
banned individuals circumvent their ban by asking others to submit code
or review comments on their behalf.

More details, see
https://discourse.llvm.org/t/submitting-code-or-review-comments-on-someone-elses-behalf/74269
comments on their behalf, do suggest that they do it directly themselves. Or
ask the reasons why they're not doing it themselves. There should be few
legitimate reasons for them not doing so themselves.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe encourage reaching out to the coc team if you suspect that something fishy is going on after asking.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm struggling to find a way to phrase it, but I think we could do with a statement about the level of due diligence that the person doing the action on behalf of has to do. As there are plausible excuses that could be given by a banned person.

I think adding the statement about the CoC team would help here. So that this is a process that has clear steps. Ask, verify as you can, forward anything untoward to the CoC team.

I wouldn't want people (including me) to read this and draw the conclusion that it's not worth the risk to merge anything on behalf of. Especially as it could lead to a situation where these merges are more and more done by people who are not going to be as attentive.

Not intending this to be some kind of immunity statement, I am ok with merging anyone else's code having some level of risk to it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually I think I am conflating a few things here. This text does not refer to merging an existing PR, but creating your own PR with someone else's code.

So you can ignore most of that, my apologies.

I still think a clearer statement of steps would be good.

@lattner
Copy link
Collaborator

lattner commented Oct 20, 2023

This seems entirely reasonable to me.

@sam-mccall
Copy link
Collaborator

sam-mccall commented Oct 20, 2023

Per https://discourse.llvm.org/t/submitting-code-or-review-comments-on-someone-elses-behalf/74269, this change "helps with not accidentally helping a banned person to circumvent their ban", and this PR is the right venue to for questions/feedback about this.

The developer policy doesn't speak about bans, but the code of conduct says about bans:

We are not interested in evaluating severity, responding punitively, or holding people accountable. Both the relevance and our response is instead focused on how a person’s continued participation impacts the community’s safety, wellbeing, and inclusivity. We specifically prioritize remaining a welcoming community to victims as well as groups subjected to systemic marginalization or underrepresentation.

Assuming "on-behalf-of" contributions themselves meet the bar we set in terms of technical content and respectful conduct, and they are not attributed to the banned individual, it's not clear to me what the impact on inclusivity is. Avoiding circumvention of bans as a goal per se a seems more like "holding people accountable".

I'm speculating here with limited perspective. From the announcement it's unclear whether this policy change is motivated by reports of harm, or on equally-shaky speculation! More context would help.

tl;dr: https://xkcd.com/810/?

(Regardless, the actual change looks good - in my naivete I would see "being banned" as one of the few legitimate reasons to have someone else submit your code)

@efriedma-quic
Copy link
Collaborator

I don't think wording of the change makes sense; it doesn't make sense without the context of the announcement. It doesn't really explain why people should be suspicious of such requests, or why we don't want people to accept such requests. If we're specifically concerned about ban-evasion, we should say that explicitly, I think.

Maybe also more explicitly distinguish between creating a PR on behalf of someone else (which shouldn't be happening), vs. creating a PR that happens to contain code authored by someone else (which is generally not suspicious).

@asb
Copy link
Contributor

asb commented Oct 22, 2023

Very minor: please undo the truncation of the commit title when committing (I documented this gotcha here after finding my experience skimming through commits worsened due to this annoying GitHub behaviour.

I agree with the points raised by @efriedma-quic, especially that the guidance seems a bit mysterious and hard to interpret without the context of the announcement that went out. Perhaps adding something like "If you have any suspicion that someone is asking you to submit work for them in order to evade a ban, please contact the CoC team.".

I also note there are other parts of the developer policy that overlap to some degree, and I'm not sure if they should be updated as well:

  • Attribution of changes, which currently says "If someone sends you a patch privately, encourage them to submit it to the appropriate list first."
  • Some discussion under commit messages on use of the co-authored-by tag.

@AaronBallman
Copy link
Collaborator

Thank you for working on adding some clarity here! I somewhat share the concern from @efriedma-quic about this being hard to understand without some more background context.

Another thing we may want to clarify in this space is (from the RFC thread): Banning an individual means that the individual has lost access to all project infrastructure and cannot attend LLVM events (in-person or virtual).

That's a broader action than I had realized; for example, this means a banned individual is prohibited from filing issues. But "lost access to all project infrastructure" also sounds like "not allowed to clone the repo" or "not allowed to visit llvm.org" (because those involve accessing project infrastructure). I'm guessing we might want more nuance than that, but whatever we decide the rules are, we should clearly document them so banned individuals and the community understands what is and isn't acceptable.

@kbeyls
Copy link
Collaborator Author

kbeyls commented Jan 12, 2024

Thank you everyone who provided feedback here and offline.
The Code of Conduct committee evaluated all feedback and held more conversations with people.
The Code of Conduct committee based on that feedback and evaluation proposes the updated text that is part of the commit that was added to this PR just now #273db04

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

Thank you for the update, I think the new wording is in good enough shape to publish.

@shafik
Copy link
Collaborator

shafik commented Jan 12, 2024

Thank for this clarification, it looks good to me.

@kbeyls kbeyls merged commit 608d602 into llvm:main Jan 27, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants