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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/Phabricator.rst:181
+
+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
----------------
awarzynski wrote:
> jhenderson wrote:
> > Canonical spelling is "GitHub" (upper-case H).
> 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?
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).


================
Comment at: llvm/docs/Phabricator.rst:187-188
+that you have a series of commits that you have not posted for review and use
+`arc` to upload them. They can be adapted if you're using the web interface,
+or updating existing reviews.
+
----------------
awarzynski wrote:
> 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?
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115519