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

[Andrzej Warzynski via Phabricator via llvm-commits]

awarzynski added inline comments.


================
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>"
----------------
jhenderson wrote:
> 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").
> I don't believe you need to use arc for this process.

That's true. In fact, there are at least 3 methods:
# exclusively with the web UI
# exclusively with `arc`
# 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?


================
Comment at: llvm/docs/Phabricator.rst:168
+
+    Differential revision: https://reviews.llvm.org/D12345
+
----------------
This line will be added by Arcanist _after_ you run `arc diff`, right?


================
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.
----------------
DavidSpickett wrote:
> awarzynski wrote:
> > 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 :) 
> > 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?
> 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.**


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115519