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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Thanks for the update; it looks much clearer to me. In addition to my inline comments, is it worth adding a couple of other points?

- Should we say something about "before merging, go back and remove all "Depends on..." lines" to avoid cluttering the commit message with an irrelevant comment (irrelevant because once the patch has landed, there's an implicit dependency chain anyway, due to git.
- Maybe add a note saying that patch series don't need to be a strict linear history: a patch can have multiple parents and/or multiple children. Each additional parent dependency would require its own "Depends on" line, if I'm not mistaken.



================
Comment at: llvm/docs/Phabricator.rst:157-161
+The second method is less work for you but assumes you are using `arc`. This can be
+done when updating a review as well as uploading it for the first time.
+
+* Upload the first review with `arc`. Note its patch number.
+* For each commit after that add a line to the commit message. "Depends on D<num>"
----------------
I don't believe you need to use `arc` for this process. I've used the "Depends on" trick many times, and I don't use `arc`. You can do it in the patch description in the Web UI too.

Possible alternative phrasing in the inline edit, including a couple of other suggested tweaks ("is less" is subjective, whereas "may be less" will always be true (in that "not being less" is covered under "may").


================
Comment at: llvm/docs/Phabricator.rst:164
+  This must be entirely on its own line, with a blank line before it.
+  For example::
+
----------------
Should this really be a double colon?


================
Comment at: llvm/docs/Phabricator.rst:166-170
+    [llvm] Example commit for Phabricator docs
+
+    Differential revision: https://reviews.llvm.org/D12345
+
+    Depends on D12344
----------------
I think you want to put this in some kind of no-format/code markdown or similar.


================
Comment at: llvm/docs/Phabricator.rst:171
+    Depends on D12344
+* Upload the commit for review with `arc diff`.
+* You will see a "Stack" tab in the "Revision Contents" section of the review
----------------
This line will need modifying in a similar manner to the above comment, if you go with it.


================
Comment at: llvm/docs/Phabricator.rst:188
+
+  - `arc diff` to upload for review.
+
----------------
This suggested workflow works in the same manner for the web UI.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115519