[docs] Tweak update to Phabricator docs about setting repository for diffs uploaded via web

[benhamilton]

Summary

In D41919, I missed that there was a *second* step when uploading
diffs via web where the repository should be specified.

[asb]

Well spotted. I've just verified that if you enter the repository on the first page it isn't automatically copied through to the "Create Revision" page. I'm assuming that it's setting the repository on the "create revision" stage that results in automatically adding llvm-commits of cfe-commits to the subscriber list?

Questions:

1. Is there actually any advantage to entering the repository on the first ("create Diff")? It seems likely we should advise people to ignore it on this page, as they'll enter the repository at a future stage.
2. When updating a diff, presumably there's no advantage to entering the repository?

[probinson]

Thinking about the Desired Visible Effect, the auto-subscription would be on the Revision and not on the Diff, assuming that it's the subscriber list on the Revision that determines where the emails go.
But that would be sensible from the end-user's point of view, which doesn't mean it's actually how the software works. :-)

[benhamilton]

Is there actually any advantage to entering the repository on the first ("create Diff")? It seems likely we should advise people to ignore it on this page, as they'll enter the repository at a future stage.

I'm assuming this actually does something in some variation of the flow (maybe pasting a diff?), or else it wouldn't be there.

It's definitely not copied when uploading a diff. This is a bug which I just reproduced at https://secure.phabricator.com/ .

I reported the upstream bug here:

https://discourse.phabricator-community.org/t/creating-diff-from-web-does-not-persist-repository-selection/957

For now, it's probably safest to tell people to enter the repository twice, and fix that when/if the product behavior changes.

[probinson]

And there are no subscribers on this review...

[benhamilton]

And there are no subscribers on this review...

Ah hah. I forgot to update the .arcconfig for the llvm repo setting the repository callsign. Fix in D41964. (I just manually updated the repository for this diff to rL so it Cc:'ed llvm-commits.)

[probinson]

So, I just posted a review without specifying the Repository for the diff, but I did put it on the review, and it auto-subscribed llvm-commits.
I suspect there's no real reason to put it on a diff, and we shouldn't tell people to do that.

[benhamilton]

I suspect there's no real reason to put it on a diff, and we shouldn't tell people to do that.

My foggy recollection of Phabricator internals tells me there is actually a repository attached to both Phabricator objects (the diff and the review), but we're not using the repository on the diff for much of anything.

[asb]

Yes, I'm sure the repository information is stored somewhere attached to the diff object - but it seems we don't use it. For this patch, I think the best way forwards is to advise users they can ignore the field when uploading the diff, and should enter it on the subsequent page. It's possible the workflow will be improved based on your upstream bug report, but we'd have to wait for the LLVM phabricator install to be updated anyway.

Thanks for all of your work on addressing the llvm-commits/cfe-commits subscription issue.

[benhamilton]

• Leave first Repository field blank.

[benhamilton]

For this patch, I think the best way forwards is to advise users they can ignore the field when uploading the diff, and should enter it on the subsequent page.

OK, done.

[asb]

I think the change to "To submit an updated patch:" should probably be left out, given we agree there's no benefit currently in specifying the repo for the diff object (as opposed to the review).

With that, looks good to me!

docs/Phabricator.rst

87 ↗

(On Diff #129626)

This change could probably be left out - there's no benefit we can see in setting the repository when posting an updated patch.

[benhamilton]

Leave repository blank when submitting an updated patch.

llvm/trunk/docs/Phabricator.rst

 * ``git format-patch -U999999 @{u}``
 * ``svn diff --diff-cmd=diff -x -U999999``
 
 To upload a new patch:
 
 * Click *Differential*.
 * Click *+ Create Diff*.
 * Paste the text diff or browse to the patch file. Click *Create Diff*.
-* In the Repository field, enter the name of the project (LLVM, Clang,
-  etc.) to which the review should be sent.
+* Leave this first Repository field blank. (We'll fill in the Repository
+  later, when sending the review.)
 * Leave the drop down on *Create a new Revision...* and click *Continue*.
 * Enter a descriptive title and summary.  The title and summary are usually
   in the form of a :ref:`commit message <commit messages>`.
 * Add reviewers (see below for advice). (If you set the Repository field
   correctly, llvm-commits or cfe-commits will be subscribed automatically;
   otherwise, you will have to manually subscribe them.)
-* Leave the Repository and Project fields blank.
+* In the Repository field, enter the name of the project (LLVM, Clang,
+  etc.) to which the review should be sent.
 * Click *Save*.
 
 To submit an updated patch:
 
 * Click *Differential*.
 * Click *+ Create Diff*.
 * Paste the updated diff or browse to the updated patch file. Click *Create Diff*.
 * Select the review you want to from the *Attach To* dropdown and click
   *Continue*.
-* Leave the Repository and Project fields blank.
+* Leave the Repository field blank. (We previously filled out the Repository
+  for the review request.)
 * Add comments about the changes in the new diff. Click *Save*.
 
 Choosing reviewers: You typically pick one or two people as initial reviewers.
 This choice is not crucial, because you are merely suggesting and not requiring
 them to participate. Many people will see the email notification on cfe-commits
 or llvm-commits, and if the subject line suggests the patch is something they
 should look at, they will.