[PATCH] D115519: [llvm][docs] Describe how to work with patch series on Phabricator

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Mostly looks good. In case I don't get back to review this again (my last day before Christmas is tomorrow), looks good, once my comments have been addressed.



================
Comment at: llvm/docs/Phabricator.rst:160
+
+* Upload the first review and note its patch number. Either with the web interface
+  or `arc`.
----------------



================
Comment at: llvm/docs/Phabricator.rst:162
+  or `arc`.
+* For each commit after that add a line to the commit message. "Depends on D<num>"
+  where "<num>" is the patch number of the previous commit.
----------------
I think it's important to state "patch description" because the Phabricator description doesn't have to match the commit message the patch corresponds to.

(Also some minor grammar improvements)


================
Comment at: llvm/docs/Phabricator.rst:172
+* If you want a single commit to have multiple parent commits then
+  add more with "and". "Depends on D12344 and D12345".
+* Upload the commit with the web interface or `arc`.
----------------
I think the current form looks a little off to me (though I can't explain exactly why). I suggest adding "for example:" as per the inline edit.


================
Comment at: llvm/docs/Phabricator.rst:173-174
+  add more with "and". "Depends on D12344 and D12345".
+* Upload the commit with the web interface or `arc`.
+  (``arc diff --verbatim`` to update an existing review)
+* You will see a "Stack" tab in the "Revision Contents" section of the review
----------------



================
Comment at: llvm/docs/Phabricator.rst:181-183
+When you finally push the series to Github, please remove the "Depends on"
+lines from the commit messages. Since they add noise and duplicate git's
+implicit ordering.
----------------
Canonical spelling is "GitHub" (upper-case H).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115519