[PATCH] D56482: DO NOT SUBMIT. Draft for guidelines on using Phabricator.

Kristof Beyls via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jan 10 01:18:47 PST 2019


kristof.beyls added inline comments.


================
Comment at: docs/Phabricator.rst:35-40
+  such and that once a revision is committed it is closed. The easiest way as
+  mentioned below involves simply putting the following on a separate line 
+  in your commit message where ``xxxxx`` is the revision number in the URL:
+  ``Differential Revision: https://reviews.llvm.org/Dxxxxx``
+  This will automatically update your revision with the committed one, link
+  it to the commit and close it.
----------------
This feels like a bit of duplication in the documentation. I wonder if something like
"The easiest way is to insert a line in your commit message, as documented below_[link to below]'
?
It will keep the documentation a bit shorter, which should help for people to actually read through all of it...


================
Comment at: docs/Phabricator.rst:55-56
+* Pre-commit reviews are much better than post-commit reviews, in general, as
+  per developer policy, most commits should be reviewed or at the very least
+  be given time for others to voice potential concerns or point out issues.
+* Post-commit concerns are best addressed by replying directly to the relevant
----------------
This seems to be inconsistent with a maybe unwritten policy which seems to exist that "if you asked for review, you must wait until someone reviewed it."
This sentence seems to suggest that "it's ok to put something up for review and then later commit it without sign-off in at least some circumstances".
I think it would be useful if there was a way to somehow share with the community "I intend to commit this - it seems obviously good to me - but I want to give time for concerns to be raised."
I think currently, this is mostly done by just going for post-commit review.
I'm not sure what a good solution for the above is.


================
Comment at: docs/Phabricator.rst:60-62
+* Unless discussed outside of Phabricator, in general, don't commit revisions
+  with unaddressed issues raised by reviewers. Doing so defeats the point of
+  pre-commit review.
----------------
Maybe also state: "If you do commit with unaddressed issues on the Phabricator review, please add a comment to the Phabricator review why those unaddressed issues can be ignored before committing."
I think it's nice for people to be able to find after the fact why those unaddressed issues were felt to not be important enough to block a commit.
Adding a comment on the Phabricator review may well be the most discoverable place for such rationales.


================
Comment at: docs/Phabricator.rst:68-73
+  difficult to work out at first, so searching for past reviews for similar
+  area is a good way to start. Some revisions are often added with a single
+  reviewer relevant to only one area, with followup revisions having the
+  same pattern despite being related to different areas. For bigger changes, it
+  is worth considering adding multiple reviewers depending on the size and
+  nature of the patches. This in turn ensures better review quality. 
----------------
Maybe better to move the suggestion to the section "Finding potential reviewers" and just point to that section from here?
There are more suggestions already in that section.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56482/new/

https://reviews.llvm.org/D56482





More information about the llvm-commits mailing list