This is an archive of the discontinued LLVM Phabricator instance.

Update Phabricator.rst
Needs ReviewPublic

Authored by winksaville on Apr 29 2019, 9:32 AM.

Details

Summary

Add additional information to help beginners submit reviews.

Event Timeline

winksaville created this revision.Apr 29 2019, 9:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 29 2019, 9:32 AM
lebedev.ri added inline comments.
llvm/docs/Phabricator.rst
70–71

It really should be emphasized that either the appropriate repo field should be set,
or appropriate -commits list be subscribed, from the start.

Update with additional information

winksaville added inline comments.Apr 29 2019, 10:36 AM
llvm/docs/Phabricator.rst
70–71

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?

winksaville marked an inline comment as done.Apr 29 2019, 10:51 AM
winksaville added inline comments.
llvm/docs/Phabricator.rst
70–71

@lebedev.ri do you mean add information about forking of llvm/llvm-project, clone the fork and then creating a commit?

lebedev.ri added inline comments.Apr 29 2019, 11:05 AM
llvm/docs/Phabricator.rst
70–71

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.

winksaville added inline comments.Apr 29 2019, 11:56 AM
llvm/docs/Phabricator.rst
70–71

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

I'll update llvm/docs/Phabricator-commit-message.png when I get it right :)

lebedev.ri added inline comments.Apr 29 2019, 12:03 PM
llvm/docs/Phabricator.rst
70–71

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 listinfo."`

hintonda added inline comments.
llvm/docs/Phabricator.rst
70–71

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.

lebedev.ri added inline comments.Apr 29 2019, 12:16 PM
llvm/docs/Phabricator.rst
70–71

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.

winksaville marked an inline comment as done.Apr 29 2019, 12:28 PM
winksaville added inline comments.
llvm/docs/Phabricator.rst
70–71

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?

hintonda added inline comments.Apr 29 2019, 12:40 PM
llvm/docs/Phabricator.rst
70–71

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

winksaville marked an inline comment as not done.

Apply changes from review comments

Btw, thanks again for doing this. I'll try take a closer look tonight.

winksaville marked an inline comment as done.Apr 29 2019, 1:07 PM
winksaville added inline comments.
llvm/docs/Phabricator.rst
70–71

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.

benhamilton added inline comments.Apr 29 2019, 2:31 PM
llvm/docs/Phabricator.rst
70–71

The repository field is set automatically based on the closest .arcconfig file going up from the directory in which you ran arc diff.

benhamilton added inline comments.Apr 29 2019, 2:33 PM
llvm/docs/Phabricator.rst
70–71

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

winksaville marked an inline comment as done.Apr 29 2019, 6:37 PM
winksaville added inline comments.
llvm/docs/Phabricator.rst
70–71

So is there any more changes anyone would like me to make?

I've added a few reviewers more familiar with this documentation.

My main concern is the addition of images. None of the other documentation has any, and I'm not sure about the policy, so I'll leave that question to others.

However, I have made a couple comments below.

llvm/docs/Phabricator.rst
71

I think it would be helpful to include the subscribers info @benhamilton provided.

170–171

You could also mention arc cover here.

200

Cut-n-paste error?

winksaville marked 2 inline comments as done.

Changes suggested by Don Hinton

winksaville added inline comments.Apr 30 2019, 10:10 AM
llvm/docs/Phabricator.rst
170–171

I had it previously, but then felt it needed more explanation, so was lazy and removed it. But it does reasonable information, I'll add it back with an example section.

200

Yes :)

Are there any other edits people would like?

jyknight added inline comments.May 9 2019, 3:17 PM
llvm/docs/Phabricator.rst
71

I'd add one more sentence here, to tell people that in the case that they are doing the currently-recommend thing, they do _NOT_ need to worry about this. Something like:
"If you are working off a clone of the official https://github.com/llvm/llvm-project/ repository, Phabricator will subscribe the correct lists automatically. Otherwise, please add the relevant ones yourself."

LGTM otherwise!

Update as suggested by jyknight

Add an additional sentence to the "Before uploading ..." paragraph
so clarify that Phabicator can update Subscribers automatically.

I believe this is ready for merging, is there something more I need to do?

I think this is ready to be commit, if additional changes are needed let me know.