This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by benhamilton on Jan 11 2018, 10:55 AM.

Details

Summary

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

Event Timeline

benhamilton created this revision.Jan 11 2018, 10:55 AM
asb added a comment.Jan 11 2018, 11:04 AM

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?

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

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.

And there are no subscribers on this review...

benhamilton set the repository for this revision to rL LLVM.Jan 11 2018, 1:33 PM

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

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.

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 added a comment.Jan 12 2018, 12:20 AM

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.

  • Leave first Repository field blank.

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 accepted this revision.Jan 12 2018, 7:41 AM

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–88

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

This revision is now accepted and ready to land.Jan 12 2018, 7:41 AM

Leave repository blank when submitting an updated patch.

This revision was automatically updated to reflect the committed changes.