Page MenuHomePhabricator

[docs] Phabricator, not the lists is the main entry point for new patches
Needs RevisionPublic

Authored by lebedev.ri on Jun 18 2019, 5:31 AM.

Details

Summary

I've seen people trip over the current friendly-but-not-true wording more than once.
I'm reasonably sure that all the new patches generally go to phabricator, not lists.
How about we reword it to be more specific, and prevent misdirections?

Diff Detail

Repository
rL LLVM

Event Timeline

lebedev.ri created this revision.Jun 18 2019, 5:31 AM
lebedev.ri marked an inline comment as done.Jun 18 2019, 5:40 AM
lebedev.ri added inline comments.
docs/Contributing.rst
58–59

I understand that it says that the review can happen on lists too, but the general public will
not read the second line here about first submitting the patch to Phabricator, and *then* reviewing on lists,
so it is confusing to mention lists here.

Requiring Phabricator raises the barrier to one-off patches from casual contributors, because using Phabricator requires a registration step.
I don't think we should require it until casual users with drive-by patches can contribute easily.

Requiring Phabricator raises the barrier to one-off patches from casual contributors, because using Phabricator requires a registration step.
I don't think we should require it until casual users with drive-by patches can contribute easily.

But my impressions is that, in practice, patches sent to the mailing list are not tracked, more difficult to review, and often, don't get looked at. Having a patch sent to the mailing list only to be ignored is a larger barrier to entry than the registration step. We should ask people to follow a procedure that is like to work for them, not one likely to end in frustration.

Requiring Phabricator raises the barrier to one-off patches from casual contributors, because using Phabricator requires a registration step.
I don't think we should require it until casual users with drive-by patches can contribute easily.

But my impressions is that, in practice, patches sent to the mailing list are not tracked, more difficult to review, and often, don't get looked at. Having a patch sent to the mailing list only to be ignored is a larger barrier to entry than the registration step. We should ask people to follow a procedure that is like to work for them, not one likely to end in frustration.

That's quite precisely my point.
It's friendlier to write the *actual* steps, even if they are more complicated than the
friendly "just submit the patch somewhere, it's their job to track every single entry point"
which is ultimately not true and results in silently ignored patches.

I see the point; certainly when someone emails a patch, the first response is almost always "can you put this on Phab."

There was talk at some point about connecting Phab with github authentication, somehow? If people can use an existing account then the you-need-to-register-first objection goes away.

In any event, this is a project policy change and should have an RFC on llvm-dev, not be proposed in a patch.

davide added a subscriber: davide.Jun 19 2019, 11:52 AM

I see the point; certainly when someone emails a patch, the first response is almost always "can you put this on Phab."

There was talk at some point about connecting Phab with github authentication, somehow? If people can use an existing account then the you-need-to-register-first objection goes away.

I log on Phabricator using GitHub every day, but my account was activated a while ago, so I'm not sure if that was deactivated in the meanwhile.

So, i did submit the RFC in https://lists.llvm.org/pipermail/llvm-dev/2019-June/133162.html
There were 6 replies from 5 people, summary:

So no one opposed this, everyone liked it.
The point about lists being subscribed, and being ready to reply to
mail-only comments i did not change, i believe that is all already stated.

Now, anyone care to stamp this? :)

reames requested changes to this revision.Wed, Jul 17, 12:14 PM

I will be willing to LGTM this, but first, a couple of suggested changes.

docs/DeveloperPolicy.rst
101

I'm fine with the change of tone, but deleting the specific mailing list advise about how to do so is probably a poor idea. It'd be good to shift those two paragraphs past the one about confidential information, and maybe lead in with something along the lines of:

It is also acceptable to submit patches directly to the llvm-commit mailing list. We discourage this for new users though as it less likely your patch will be seen by motivated reviewers as many who watch for contributions from new contributors rely on phabricator for filtering.

docs/Phabricator.rst
16

Addition: Such comments are almost always mirrored into phabricator automatically, but sometimes the scripting isn't quite perfect. Please check manually before committing.

This revision now requires changes to proceed.Wed, Jul 17, 12:14 PM