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

[DavidSpickett]

The linked page is: https://moz-conduit.readthedocs.io/en/latest/arcanist-user.html#series-of-commits
Which did link to another now gone blog post but I found a copy on Internet Archive: https://web.archive.org/web/20180907120112/https://www.smacleod.ca/posts/commit-series-with-phabricator/

I could rewrite the linked Mozilla page so that we don't have to worry about it moving but I thought it would be a shame since it's quite well laid out.

I added the "recommended" (at least by me, please disagree) workflow paragraph to summarise what the now missing second blog post talked about.

Since this is in response to a question on llvm-dev I will post a link to this back there and hopefully the author can truly say whether this helps or not.

[jhenderson]

llvm/docs/Phabricator.rst

138

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.

141–144

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?

[awarzynski]

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.

llvm/docs/Phabricator.rst

138

@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".

141–144

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 :)

[DavidSpickett]

llvm/docs/Phabricator.rst

141–144

and this paragraph is about updating a series of patches, right?

You could use it for both. That's what I do personally but perhaps just because I could never figure out how to stop arc combining commits. Maybe once you've got a series setup, it knows not to because of the differential links?

[DavidSpickett]

Link to Mozilla docs as well as describing both methods in our own docs.

Expand the git rebase workflow suggestion that is now "frequently used"
not "reccomended".

[DavidSpickett]

llvm/docs/Phabricator.rst

187

If you're wondering, Rst wants these blank lines so it can recognise the new indent level.

[DavidSpickett]

llvm/docs/Phabricator.rst

156

Probably going to add sub-titles for each method "with web interface", "with arc" just don't have the time to get to it today.

[jhenderson]

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.

llvm/docs/Phabricator.rst

157–161

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").

164

Should this really be a double colon?

166–170

I think you want to put this in some kind of no-format/code markdown or similar.

171

This line will need modifying in a similar manner to the above comment, if you go with it.

188

This suggested workflow works in the same manner for the web UI.

[awarzynski]

llvm/docs/Phabricator.rst

141–144

That's what I do personally but perhaps just because I could never figure out how to stop arc combining commits.

arc diff --head <commit> where <commit> is the end of the commit range. I think.

157–161

I don't believe you need to use arc for this process.

That's true. In fact, there are at least 3 methods:

1. exclusively with the web UI
2. exclusively with arc
3. web UI mixed with arc

Originally David only documented 1. and 2. For me that's sufficient and it makes a lot of sense - there are 2 paragraphs and 2 "completely" different methods. IMO, adding 3. to the mix blurs the message a bit ("essentially, there are 2 ways to achieve this"). Perhaps "less is more" in this case?

168

This line will be added by Arcanist _after_ you run arc diff, right?

[DavidSpickett]

Rephrase so that method 1 is using the add revisions in the web interface and
method 2 is using "Depends on" with either arc or the web interface.

Add note that you can have multiple parent reviews. I tested that "and"
works to add more.

Implement phrasing comments.

Double backtick where showing a command so that dash dash isn't merged
visually into one character. Put the rebase command in its own box
since it didn't make sense on the end of a line.

Removed the "Differential review" line from the example commit since
that's only there once you upload it with arc.

[DavidSpickett]

llvm/docs/Phabricator.rst

156

On second thought there's not too much text here that it needs it.

157–161

On reflection I think the 2 routes are "Depends" with arc or web, and "Edit parent" with only the web. So I've rephrased it that way.

164

This makes the following text into a box.

166–170

Switched to plain text boxes or double backtick for all the commands.

168

Removed that line.

[jhenderson]

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.

llvm/docs/Phabricator.rst

160

162

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)

172

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.

173–174

181–183

Canonical spelling is "GitHub" (upper-case H).

[awarzynski]

It looks like after a few rounds of updates, two methods emerged:

• "Using the web UI"
• "Through commit messages/summaries"

Perhaps it's worth revisiting the idea of adding section titles? I think that it would help with the flow.

llvm/docs/Phabricator.rst

181

I think that "pushing to GitHub" is too vague. Also, this might suggest that you normally push multiple patches at once. But often you push one patch at a time, right?

185

It's not immediately clear what "this" refers to here. I would expand.

187–188

I would remove this last sentence. In this case, IMHO, less is more.

Basically, this is a bit complex and I think that focusing on arc diff + git rebase is sufficient. Yes, you can use the web UI too, but perhaps that's something that people can deduct here themselves?

[jhenderson]

llvm/docs/Phabricator.rst

181

Looking at this again, I think you should just put "When you push the patches, please ...". The fact that we push to GitHub is an implementation detail that might change, plus it could be misunderstood as something to do with GitHub PRs, I guess (i.e. pushing your private branch to the LLVM repo as a PR).

187–188

I wouldn't agree here (as someone who doesn't use arc at all, if I read this paragraph without the sentence, I'd just skip the rest of the example, as it "doesn't apply to me"), but on reread, this example really isn't all that applicable to the Web UI, since commit messages aren't directly linked to patch descriptions (i.e. modifying the local commit message has no impact on anything in the web UI).

[awarzynski]

llvm/docs/Phabricator.rst

187–188

Fair point. Maybe the first sentence should be rephrased to be a bit more general:

One frequently used workflow for creating a series of patches involves git's rebasing. These steps assume that you have a series of commits that you have not posted for review.

Then, replace:

- ``arc diff`` to upload for review.`

with:

- upload the current commit for a review (either with ``arc diff`` or via the we UI)

This way it should work for everyone (other steps are already general enough).

[DavidSpickett]

• Add section titles and used that to better signal what the rebasing suggestion refers to.
• Incorporated grammar changes.
• Removed full stops from end of lines that ended in a command. Since a monospace "." looks the same as the default style "." and could be seen as being part of the command.
• Changed a few instances of using "commit message" where I mean the patch summary. (which means it makes more sense if you're in the web UI)

[DavidSpickett]

• Incorporate some grammar comments I missed
• Make the rebasing workflow fit arc or web UI.

[jhenderson]

llvm/docs/Phabricator.rst

182

I think this should be a comma, as per my previous suggestion, rather than making "For example" a new sentence. It doesn't read right otherwise.

195

198–200

Looks like a duplicate paragraph?

[awarzynski]

llvm/docs/Phabricator.rst

198–200

I'm guessing that the previous one was meant to be deleted (it still refers to arc)

[DavidSpickett]

Remove rebase description that refered to arc.
Comma before "for example: "D..."".
Minor change you're -> you are.

[jhenderson]

LGTM, with one remaining grammar fix.

llvm/docs/Phabricator.rst

173

[DavidSpickett]

". Where" -> ", where"

[DavidSpickett]

Thanks for the detailed review @jhenderson !

[awarzynski]

Thank you for doing this David, it will be super helpful! LGTM

[nickdesaulniers]

thanks for writing this! I didn't know about Depends on D...

llvm/docs/Phabricator.rst

 * Add comments about the changes in the new diff. Click *Save*.
 
 Choosing reviewers: You typically pick one or two people as initial reviewers.
 This choice is not crucial, because you are merely suggesting and not requiring
 them to participate. Many people will see the email notification on cfe-commits
 or llvm-commits, and if the subject line suggests the patch is something they
 should look at, they will.
 
+.. _creating-a-patch-series:
+
+Creating a patch series
+-----------------------
+
+Chaining reviews together requires some manual work. There are two ways to do it
+(these are also described `here <https://moz-conduit.readthedocs.io/en/latest/arcanist-user.html#series-of-commits>`_
+along with some screenshots of what to expect).
+
+.. _using-the-web-interface:
+
+Using the web interface
+^^^^^^^^^^^^^^^^^^^^^^^
+
+This assumes that you've already created a Phabricator review for each commit,
+using `arc` or the web interface.
+
+* Go to what will be the last review in the series (the most recent).
+* Click "Edit Related Revisions" then "Edit Parent Revisions".
+* This will open a dialog where you will enter the patch number of the parent patch
+  (or patches). The patch number is of the form D<number> and you can find it by
+  looking at the URL for the review e.g. reviews.llvm/org/D12345.
+* Click "Save Parent Revisions" after entering them.
+* You should now see a "Stack" tab in the "Revision Contents" section of the web
+  interface, showing the parent patch that you added.
+
+Repeat this with each previous review until you reach the first in the series. This
+one won't have a parent since it's the start of the series.
+
+If you prefer to start with the first in the series and go forward, you can use the
+"Edit Child Revisions" option instead.
+
+.. _using-patch-summaries:
+
+Using patch summaries
+^^^^^^^^^^^^^^^^^^^^^
+
+This applies to new and existing reviews, uploaded with `arc` or the web interface.
+
+* Upload the first review and note its patch number, either with the web interface
+  or `arc`.
+* For each commit after that, add the following line to the commit message or patch
+  summary: "Depends on D<num>", where "<num>" is the patch number of the previous review.
+  This must be entirely on its own line, with a blank line before it.
+  For example::
+
+    [llvm] Example commit
+
+    Depends on D12345
+
+* If you want a single review to have multiple parent reviews then
+  add more with "and", for example: "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
+  in the web interface, showing the parent review.
+* Repeat these steps until you've uploaded or updated all the patches in
+  your series.
+
+When you push the patches, please remove the "Depends on" lines from the
+commit messages, since they add noise and duplicate git's implicit ordering.
+
+One frequently used workflow for creating a series of patches using patch summaries
+is based on git's rebasing. These steps assume that you have a series of commits that
+you have not posted for review, but can be adapted to update existing reviews.
+
+* git interactive rebase back to the first commit you want to upload for review::
+
+    git rebase -i HEAD~<number of commits you have written>
+
+* Mark all commits for editing by changing "pick" to "edit" in the instructions
+  git shows.
+* Start the rebase (usually by writing and closing the instructions).
+* For the first commit:
+
+  - Upload the current commit for a review (with ``arc diff`` or the web
+    interface).
+
+  - Continue to the next commit with ``git rebase --continue``
+
+* For the rest:
+
+  - Add the "Depends on..." line using ``git commit --amend``
+
+  - Upload for review.
+
+  - Continue the rebase.
+
+* Once the rebase is complete, you've created your patch series.
 
 .. _finding-potential-reviewers:
 
 Finding potential reviewers
 ---------------------------
 
 Here are a couple of ways to pick the initial reviewer(s):