This is an archive of the discontinued LLVM Phabricator instance.

[doc] Clarify steps for contributors without commit access.
ClosedPublic

Authored by fhahn on Dec 30 2016, 12:40 PM.

Details

Summary

Update the Phabricator docs to clarify how changes are merged for contributors without commit access.

Diff Detail

Event Timeline

fhahn updated this revision to Diff 82741.Dec 30 2016, 12:40 PM
fhahn retitled this revision from to [doc] Clarify steps for contributors without commit access..
fhahn updated this object.
fhahn added a reviewer: delcypher.
fhahn added a subscriber: llvm-commits.
fhahn added a subscriber: anmol.Dec 30 2016, 12:43 PM

@anmol It would be great if you could let me know if that change makes things clearer for new contributors.

aaron.ballman added inline comments.
docs/Phabricator.rst
131

somebody -> someone

132

I would drop the "usually" -- it should always be sufficient to add that comment.

fhahn updated this revision to Diff 82746.Dec 30 2016, 1:12 PM

@aaron.ballman thanks for your comments! I've updated the patch.

fhahn marked 2 inline comments as done.Dec 30 2016, 1:13 PM
aaron.ballman accepted this revision.Dec 30 2016, 1:35 PM
aaron.ballman added a reviewer: aaron.ballman.

LGTM, thank you for helping improve the documentation!

This revision is now accepted and ready to land.Dec 30 2016, 1:35 PM
fhahn closed this revision.Dec 30 2016, 1:39 PM
anmol added a comment.Dec 30 2016, 2:25 PM

@fhahn thank you, this is definitely helpful to a new contributor to understand the process.

I note the wording: "...it can then be committed to trunk" and later on: "If you decide you should not commit the patch, you should explicitly abandon the review so that reviewers don’t think it is still open." So, ultimately, the contributor needs to indicate whether they wish for their patch to get committed? I would think that putting a patch on Phabricator implies that ultimately when all looks good about it, the intent is that it gets committed to trunk. On the other hand, for a reviewer with commit access, do they or do they not assume that the patch is meant to be committed? So, that needs to be clarified.

Perhaps, when the state of the review changes to: "This revision is now accepted and ready to land.", we have two questions come up for the contributor to answer:

  • Do you wish for this patch to be committed to trunk?

(if so) [] Do you have commit access?

  • that way, the fate of the patch does not remain ambiguous.

Also, if I may suggest:

[line 133] indicating you cannot commit the patch -> indicating that you cannot commit the patch
[line 135] recommend -> recommended

PS: I tried to add myself as a reviewer to this review, but could not figure it out. (I read: http://llvm.org/docs/Phabricator.html as well as tried to play with the controls on the top right).

PS: I tried to add myself as a reviewer to this review, but could not figure it out. (I read: http://llvm.org/docs/Phabricator.html as well as tried to play with the controls on the top right).

Change the 'action' at the bottom from 'Comment' to 'Add Reviewers' and enter your username.