I. Expected: patch found in the code browser. (Potential link target for the current "gitblit" links in gerrit.)
II. Observed: nothing found.
| Restricted Differential Revision |
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | Dzahn | T123525 reduce amount of remaining Ubuntu 12.04 (precise) systems in production | |||
| Resolved | • Cmjohnson | T138978 decom antimony (datacenter) | |||
| Resolved | Dzahn | T123718 Phase out antimony.wikimedia.org (git.wikimedia.org / gitblit) | |||
| Resolved | Dzahn | T323073 Make https://git.wikimedia.org not redirect to Phabricator Diffusion but remove it | |||
| Resolved | • demon | T111465 [keyresult] Deprecate gitblit in favor of Diffusion | |||
| Resolved | • demon | T752 Use Diffusion as canonical location for browsing code repos (not gitblit) | |||
| Resolved | • mmodell | T89940 Replicate open patchsets to diffusion | |||
| Resolved | • mmodell | T128009 Next Phabricator Upgrade | 2016-04-28 |
https://phabricator.wikimedia.org/daemon/ shows the Queued tasks for PhabricatorSearchWorker.
Indexing might take a bit longer when load is heavy. If there's no load at all though, this is something to investigate indeed.
Diffusion probably doesn't fetch refs/changes/* by default whereas with Gerrit we explicitly push refs/* to Gitblit. Have to look into what we can do here.
Would that be because links are shown for gitblit instead of phabricator in gerrit.
I propose we show two links one for gitblit and one for phabricator.
No, I was just searching for the raw sha1s in Phab, which doesn't catch them.
I propose we show two links one for gitblit and one for phabricator.
Not possible.
Gitblit doesn't, Gerrit replicates all of refs/* to Gitblit. Phabricator was also setup to do the same (rOPUP9b5d4), but it's not working as intended clearly.
I think this is because in gerrit the branches look like refs/heads/master where as in phabricator they are just master, Just the branch name. So I think it wont replicate since it would replicate to the wrong path.
I think phabricator may be causing this problem.
Since it looks like
Why dosen't this 'text/x-php': 'text/x-php; charset=utf-8' work for desktops but works for mobiles. This is to do with viewing raw files on phabricator.
but refs/ is used in phabricator but the bare branch is.
I am not sure how to fix this.
But this
[http "https://gerrit.wikimedia.org"]
proxy =
looks blank for proxy should that have a link or something could that be the reason it doesn't work.
We will want to update diffusion upstream and use a config for that to add refs/changes.
Change 283208 had a related patch set uploaded (by Paladox):
Allow viewing all ref type commits
What have you done?!? I got dozens of notifications about "commits" for Gerrit changes I never merged.
We added support for importing all refs. You can now view open gerrit changes in phabricator.
I've long recommended that people turn off "a commit is created" in their settings anyway. It's a mostly useless notification.
The more annoying thing is 'added a commit to a task' notification which doesn't have it's own separate setting, so you have to disable 'other maniphest task actions' in email settings.
Yeah, I agree and I would have preferred if we kept those patches out of Phabricator, however, it wasn't really up to me.
What do we need to do to find out if there is consensus to turn this back off? I really can't imagine why we need to have patch sets like rOPUPb734a6a50c09 in diffusion and I especially don't see the utility in them spamming any tasks they mention with "bd808 mentioned this in rOPUP524c1e18bbe8: logstash: Update default mappings for Elasticsearch 2.x." on each revision.
@bd808: I agree about spamming tasks.
The primary motivation for importing those changes into phabricator is simply that gerrit had links to those changes in gitblit. Since we killed off gitblit we needed something to replace those links in gerrit.
So this is for feature parity via the little "(diffusion)" links in gerrit that are listed to the right of the patch set labels in gerrit (e.g. "► Patch Set 1 rOPUP524c1e18bbe8 (diffusion)".
I'm probably a bad person to argue about the utility of keeping these links as I honestly had never noticed them in 3 years of using gerrit on a very regular basis. On IRC @Paladox made a claim that the external viewer links are valuable for reviewing code from phones and other unspecified user-agents that have difficulty with rendering large patches via gerrit. This seems like a pretty niche edge case to support at the expense of possible email spam and definite task timeline clutter. Imagine how "awesome" this feature would have been if it had been enabled for https://gerrit.wikimedia.org/r/#/c/195297/ where 175 patch sets were submitted over the course of a year related to three different phab tasks.
(Reopening while we discuss this.)
"Anything" is possible in code (with enough time), so comments like this aren't exactly helpful.
That's actually useful information. I would put @bd808 in the category of "major code review user".
On IRC @Paladox made a claim that the external viewer links are valuable for reviewing code from phones and other unspecified user-agents that have difficulty with rendering large patches via gerrit. This seems like a pretty niche edge case to support at the expense of possible email spam and definite task timeline clutter.
If that is truly the only use case, then yes, this is not a sufficient trade off. Are there any others that are missed here? I read the entirety of this task and didn't see anything else mentioned. @Paladox?
Imagine how "awesome" this feature would have been if it had been enabled for https://gerrit.wikimedia.org/r/#/c/195297/ where 175 patch sets were submitted over the course of a year related to three different phab tasks.
This change, indeed, may negatively influence developers from making small changes to their proposed changes (or even doing [WIP]-type changes in fear they will get too much attention). This is conjecture on my part, but it is not far fetched in my opinion.
The use case I've often seen mentioned isn't necessarily about mobile user agents or large patches, but just that people find the Gerrit diffs inadequate. This is a valid concern, but a sign that we should push on having Gerrit diffs improved imho rather than relying on an external viewer (and I'm curious if there's enough improvements in 2.12.x)
This isn't to say an external viewer for the *actual code* is bad, Gerrit is not a repository browser and for looking at files and branches and tags and commits and such, a dedicated viewer is great. But showing every prospective change in that viewer I've never really seen the point of. I'm mostly sure that because the links were there by default, people got used to the workflow, and now we're here (cf: xkcd)
I'll note the "large patches" issue isn't really solved with an external viewer. Gitblit would just crash (because it was bad) and Phabricator has limits (see D219).
The one use case I have for refs/changes in phabricator is for converting gerrit unmerged changes into differential diffs.
There's a difference between having the refs on-disk and importing and displaying them though :)
Having them in diffusion makes importing them a lot easier thanks to DifferentialDiffExtractionEngine.php