Page MenuHomePhabricator

Replicate open patchsets to diffusion
Closed, ResolvedPublic

Description

  1. Pick any open patch in gerrit sent more than a few minutes ago
  2. Search it in diffusion by commit hash (or in any other way, like construct the URL manually with callsign, or search by message, or browse a repo etc.)

I. Expected: patch found in the code browser. (Potential link target for the current "gitblit" links in gerrit.)
II. Observed: nothing found.

Event Timeline

Nemo_bis created this task.Feb 19 2015, 7:49 AM
Nemo_bis raised the priority of this task from to Normal.
Nemo_bis updated the task description. (Show Details)
Nemo_bis added a project: Gitblit-Deprecate.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 19 2015, 7:49 AM
Aklapper lowered the priority of this task from Normal to Low.Feb 19 2015, 11:22 AM

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.

demon added a subscriber: demon.Apr 17 2015, 4:44 PM

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.

demon added a comment.Nov 4 2015, 8:25 PM

We're fetching them but they don't show up in Phab, hrm.

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.

demon added a comment.Nov 4 2015, 8:45 PM

Would that be because links are shown for gitblit instead of phabricator in gerrit.

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.

It may be possible if we create a plugin like we have for gitblit.

demon added a comment.Nov 4 2015, 8:51 PM

We don't have a plugin for Gitblit.

Oh well then how does gitblit replicate patches that haven't been merged yet.

demon added a comment.Nov 5 2015, 7:05 AM

Oh well then how does gitblit replicate patches that haven't been merged yet.

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.

Oh ok. Could that be a bug.

greg raised the priority of this task from Low to Normal.Dec 2 2015, 7:06 PM
greg lowered the priority of this task from Normal to Low.
greg moved this task from To Triage to Backlog on the Gitblit-Deprecate board.
greg added a subscriber: greg.

(bad drag/drop on the workboard)

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.

Would refs/remotes/changes or something similar work.

demon added a comment.Dec 11 2015, 7:45 PM

These issues are all unrelated.

We will want to update diffusion upstream and use a config for that to add refs/changes.

Xqt added a subscriber: Xqt.Mar 16 2016, 8:33 PM

Change 283208 had a related patch set uploaded (by Paladox):
Allow viewing all ref type commits

https://gerrit.wikimedia.org/r/283208

mmodell closed this task as Resolved.Apr 28 2016, 1:05 AM
mmodell claimed this task.

What have you done?!? I got dozens of notifications about "commits" for Gerrit changes I never merged.

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.

@mmodell is there a way we can disable notifications for refs/changes/ please.

demon added a comment.Apr 28 2016, 1:33 PM

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.

Also when I list my commits all sorts of intermediate patch sets appear.

Also when I list my commits all sorts of intermediate patch sets appear.

This was intended.

Also when I list my commits all sorts of intermediate patch sets appear.

This was intended.

And confusing still.

Also when I list my commits all sorts of intermediate patch sets appear.

This was intended.

And confusing still.

Yeah, I agree and I would have preferred if we kept those patches out of Phabricator, however, it wasn't really up to me.

greg moved this task from In Progress to Done on the Gitblit-Deprecate board.Jun 9 2016, 6:46 PM
Paladox removed a subscriber: gerritbot.

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

@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 524c1e18bbe82e3a185a3a7d11b6b66afcdbd3ef (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.

@bd808 we could stop those commits from being linked in tasks through an update to the code but not sure how we do that.

@mmodell hi, do you have an idea how we can block refs/changes/ from being added to tasks please.

greg reopened this task as Open.Jul 14 2016, 5:26 PM

(Reopening while we discuss this.)

@bd808 we could stop those commits from being linked in tasks through an update to the code but not sure how we do that.

"Anything" is possible in code (with enough time), so comments like this aren't exactly helpful.

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.

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.

demon added a comment.EditedJul 14 2016, 5:34 PM

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?

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.

demon added a comment.Jul 14 2016, 5:40 PM

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

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

greg moved this task from Done to Backlog on the Gitblit-Deprecate board.Jul 15 2016, 6:00 PM
mmodell closed this task as Resolved.Jul 20 2016, 5:22 PM