Page MenuHomePhabricator

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

Authored by DavidSpickett on Dec 10 2021, 6:44 AM.

Diff Detail

Event Timeline

DavidSpickett requested review of this revision.Dec 10 2021, 6:44 AM
DavidSpickett created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2021, 6:44 AM

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 added inline comments.Dec 10 2021, 7:09 AM
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?

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 added inline comments.Dec 13 2021, 6:23 AM
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?

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 marked 4 inline comments as done.Dec 13 2021, 8:17 AM
DavidSpickett added inline comments.
llvm/docs/Phabricator.rst
187

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

DavidSpickett added inline comments.Dec 13 2021, 8:18 AM
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.

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 added inline comments.Dec 14 2021, 12:55 AM
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?

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 marked 5 inline comments as done.Dec 15 2021, 8:22 AM
DavidSpickett added inline comments.
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.

DavidSpickett marked 3 inline comments as done.Dec 15 2021, 8:22 AM

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

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 added inline comments.Dec 16 2021, 3:18 AM
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 added inline comments.Dec 16 2021, 3:41 AM
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).

  • 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 marked 13 inline comments as done.Dec 16 2021, 4:46 AM
  • Incorporate some grammar comments I missed
  • Make the rebasing workflow fit arc or web UI.
jhenderson added inline comments.Dec 16 2021, 5:31 AM
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 added inline comments.Dec 16 2021, 5:34 AM
llvm/docs/Phabricator.rst
198–200

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

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

DavidSpickett marked 4 inline comments as done.Dec 16 2021, 6:20 AM
jhenderson accepted this revision.Dec 16 2021, 6:27 AM

LGTM, with one remaining grammar fix.

llvm/docs/Phabricator.rst
173
This revision is now accepted and ready to land.Dec 16 2021, 6:27 AM

". Where" -> ", where"

DavidSpickett marked an inline comment as done.Dec 16 2021, 7:00 AM

Thanks for the detailed review @jhenderson !

awarzynski accepted this revision.Dec 16 2021, 7:09 AM

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

This revision was landed with ongoing or failed builds.Dec 16 2021, 7:32 AM
This revision was automatically updated to reflect the committed changes.

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