[PATCH] D61267: Update Phabricator.rst

[Ben Hamilton via Phabricator via llvm-commits]

benhamilton added inline comments.


================
Comment at: llvm/docs/Phabricator.rst:69-70
+runs it will use your editor, set-local-editor_, so you can supply ``Reviewers``
+and ``Subscribers`` in your commit message. It requires **one or more** Reviewers
+and allows **zero or more** Subscribers, finding-reviewer-names_.
+
----------------
benhamilton wrote:
> winksaville wrote:
> > hintonda wrote:
> > > winksaville wrote:
> > > > lebedev.ri wrote:
> > > > > hintonda wrote:
> > > > > > lebedev.ri wrote:
> > > > > > > winksaville wrote:
> > > > > > > > lebedev.ri wrote:
> > > > > > > > > winksaville wrote:
> > > > > > > > > > winksaville wrote:
> > > > > > > > > > > lebedev.ri wrote:
> > > > > > > > > > > > It really should be emphasized that either the appropriate repo field should be set,
> > > > > > > > > > > > or appropriate -commits list be subscribed, from the start.
> > > > > > > > > > > I'm don't know what you mean by 'appropriate repo field'. I've emphasied there must be **one or more** Reviewers.
> > > > > > > > > > > I could explicitly say that Differential is automatically adding "llvm-commits" to Subscribers.
> > > > > > > > > > > 
> > > > > > > > > > > Or do you mean something else?
> > > > > > > > > > @lebedev.ri do you mean add information about forking of llvm/llvm-project, clone the fork and then creating a commit?
> > > > > > > > > I'm only highlighting "The mailing list should be added as a subscriber on all reviews" from https://llvm.org/docs/Phabricator.html
> > > > > > > > > I think that a similar phrase really should be repeated here.
> > > > > > > > Ok, so instead of:
> > > > > > > > 
> > > > > > > > "It requires **one or more** Reviewers and allows **zero or more** Subscribers, finding-reviewer-names_."
> > > > > > > > 
> > > > > > > > How about:
> > > > > > > > 
> > > > > > > > "`arc` requires **one or more** Reviewers (finding-reviewer-names_) and Subscribers should be **llvm-commits** plus the appropriate **-dev** list(s) from [listinfo](https://lists.llvm.org/mailman/listinfo)."
> > > > > > > > 
> > > > > > > > I'll update llvm/docs/Phabricator-commit-message.png when I get it right :)
> > > > > > > > and Subscribers should be llvm-commits plus the appropriate -dev list(s) from listinfo."
> > > > > > > 
> > > > > > > Hm, not exactly. 
> > > > > > > 
> > > > > > > "`arc` requires one or more Reviewers (finding-reviewer-names_) and Subscribers should be the appropriate **-commits** list(s) from [[ https://lists.llvm.org/mailman/listinfo | listinfo ]]."`
> > > > > > Although I normally add subscribers out of habit, Herald takes care of this for you when you submit a patch -- believe this is all maintained by @benhamilton.
> > > > > > 
> > > > > There is a sufficient number of cases where that isn't so,
> > > > > be it either intentionally going against the docs,
> > > > > or just confusing the phabricator from auto-subscribing.
> > > > This leads to a different question.
> > > > 
> > > > I use git gui to amend the commits locally and then `arc diff master` to update. Do I need to manually update `Subscriber` with `benhamilton` so I don't accidentally remove it after @hintonda has added it?
> > > Not sure I understand your question -- don't use gui's, but figure they can't be that different.
> > > 
> > > In any case, after you create a new patch in phab, you can update it by simply typeing `arc diff`.  If you want to do it from somewhere other than where you originally ran the initial `arc diff`, you need to use the `--update` option, e.g., `arc diff --update D61267`.
> > > 
> > > You don't need to do anything else.  For example, look at the first few lines of history at the top of this page.  You'll see that Herald add the `llvm` project, and subscribed `llvm-commits`.   It's a one-time thing you don't need to worry about.
> > > 
> > > As for @lebedev.ri comment about it not always working, personally, I'd consider that a bug and raise it on the list.  I think @benhamilton fixed a few of those for me a couple of years ago.  However, it doesn't hurt to mention it in the docs.  hth... don
> > It seems adding "xxx-commit" doesn't hurt and its nice Hearld will do the right thing. I went ahead and updated the text and Phabricator-commit-message.png image.
> > 
> > If you guys would like something different, let me know.
> The repository field is set automatically based on the closest `.arcconfig` file going up from the directory in which you ran `arc diff`.
> 
(I think there are scenarios where someone can run `arc diff` from a different directory or upload a diff directly to Phabricator which can bypass its detection of the repository, which could explain @lebedev.ri 's issues. Regardless, directly reporting problems can help.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61267