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

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added a comment.

Contributions like this make LLVM more welcoming towards new-comers, thanks for drafting this @DavidSpickett ! @jhenderson left some great comments already and I added a couple more inline. Nothing major.



================
Comment at: llvm/docs/Phabricator.rst:138
+`This page <https://moz-conduit.readthedocs.io/en/latest/arcanist-user.html#series-of-commits>`_
+describes two ways to do this. One for submissions via arc and one
+for the web interface.
----------------
jhenderson wrote:
> I'm not all that convinced about referring to the external arcanist reference here, since arcanist is only one way of uploading patches for review. I would be more inclined to just summarise the contents, e.g. the following two paragraphs:
> 
> "Creating a patch series requires some manual work. There are two methods to achieve this. The first approach is to create a patch using arc or the web interface for each individual commit, and then select the "Edit Related Revisions->Edit Parent Revisions" option in the Phabricator UI for each patch after the first in the series. This will open a dialog where you can enter the patch number (e.g. `D12345`) of the parent patch(es). Click "Save Parent Revisions" after entering them.
> 
> The second approach is to create the patches and simply add "Depends on DXXXXXX" to the patch description (where "DXXXXXX" is replaced by the parent's patch number) in the child patch, entirely on its own line, with a blank line before it."
> 
> You could still add a link, if you felt the concrete examples are helpful.
@jhenderson , I really like your suggestion here!

> I'm not all that convinced about referring to the external arcanist reference here, since arcanist is only one way of uploading patches for review.

External references for Arcanist/Phab are frequently requested and, AFAIK, this is the only thing that is available. It is very well written and goes over two approaches:
* **fully manual**, i.e. via the web UI
* **semi-automated**, i.e. via `arc`
So it's not really Arcanist specific and I would definitely include it here. Perhaps you could add it as a "Useful reference with some examples". Btw, I know that both approaches require "some manual work", but perhaps it's worth emphasizing the difference between "fully manual" and "semi-automated".


================
Comment at: llvm/docs/Phabricator.rst:141-144
+A recommended workflow is to interactively rebase back to the first commit
+for review. Marking that and all your subsequent commits to be
+edited. At each commit make the necessary changes, submit with arc or the web
+interface and then continue the rebase.
----------------
jhenderson wrote:
> I'm not entirely convinced this paragraph is a) that useful, and b) all that understandable. A recommended workflow for what? Assuming "creating a commit series", in which case, what does "make the necessary changes" mean in this case?
> I'm not entirely convinced this paragraph is (...) that useful,

I think that adding a paragraph like this is very helpful. People frequently ask: "how do it do this with `arc`?" and this paragraph provides an answer.


> A recommended workflow for what? 

@DavidSpickett Perhaps just clarify that this is for rebasing/updating a series of patches on Phabricator with `arc`? Basically, the first paragraph in this patch is about the basics (i.e. how to **create** a series of patches) and this paragraph is about **updating** a series of patches, right?

Also, perhaps rephrase "A recommended" as "One frequently used"? Personally I don't mind too much, but some people might not like any Arcanist-related recommendations :) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115519