Inline comments is only partially working in an imported SVN repo
Open, Needs TriagePublic
Actions

Assigned To

None

Authored By

	foresightyj
	Dec 25 2014, 8:41 AM

Description

I successfully set up Phabricator in a Virtualbox guest ubuntu machine and also successfully made it import a SVN repo which was hosted via VisualSVN. However, I failed to make the inline comments working reliably. The comment box at the bottom works well by the way.

In this commit, it works.

In this commit, it doesn't.

After trying various files and commits. I summarized that only a few javascript/css files support inline-comment. But all c# files were not supported. Is it because Phabricator doesn't natively support Microsoft-related files (like syntax highlighting) and that screwed up the inline comments?

I cannot figure out a way to debug this. Opening the chrome javascript console only shows an error like Failed to load resource: net::ERR_CACHE_MISS, which I do not think is the cause of the bug.

Related Objects

Mentioned In: D11047: Inline Comments not working
Mentioned Here: T6891: Daemon fails to parse SVN Folder with Whitespace
D11047: Inline Comments not working

Event Timeline

foresightyj created this task.Dec 25 2014, 8:41 AM

foresightyj assigned this task to epriestley.

foresightyj raised the priority of this task from to Normal.

foresightyj updated the task description. (Show Details)

foresightyj added a project: Audit.

foresightyj added a subscriber: foresightyj.

chad removed epriestley as the assignee of this task.Dec 25 2014, 3:40 PM

chad raised the priority of this task from Normal to Needs Triage.

I responded to your bug on https://github.com/phacility/phabricator/issues/778 and did not receive any replies back. As it stands this bug is not reproducible. If you can demonstrate the issue on this install (upload a diff and give specifics to how we can produce this issue) we can look at it.

https://secure.phabricator.com/book/phabcontrib/article/bug_reports/ covers what we look for in a good bug report.

foresightyj mentioned this in D11047: Inline Comments not working.Dec 29 2014, 8:52 AM

Sorry my bad. I didn't get notifications from Github.

I reinstalled Phabricator yesterday and the new system still had this problem. I submitted the diff file to D11047: Inline Comments not working and the observation is that the diff view (in my own server setup) works in Differential but not Audit.

My server do not have a DNS pointing to it so I cannot post a link here.

I know for a command there is a --trace switch that can be used to get more information about a command. Are there any less well known commands I can use to get more information?

Does the issue exist on this Audit installation here?
What browser / OS are you using?
What other browser / OS combinations have you tried?

IIRC there is no different code path for Audit vs. Diffusion vs. Differential. We have a single set of code for rendering a diff. So why you have issues with one is a bit of a mystery.

It works well in secure.phabricator.com.
I use Chrome and IE10. The behavior is the same in both.

I installed three times and got the same problem again and again.

After digging effortlessly into the problem, I was able to narrow down the problem a bit more. I right-clicked on line numbers and chose "inspect element" in chrome.

The observation is that in my Audit page, ids for the page numbers were not assigned.

Here is my Audit page:

Here is my Differential Page:

Here is your Audit Page

Does your server show any errors in the php/webserver logs when you bring up in Audit/Diffusion? None of my test repo's have this information missing upon inspection. Maybe @epriestley has other thoughts.

Do you mean the apache webserver's error log?

foresightyj@phabricatorvb:/usr/share/php$ tail -f  /var/log/apache2/error.log
[Mon Dec 29 19:23:08.326813 2014] [:error] [pid 4998] [client 10.0.2.2:56158]   #5 queryfx_all(AphrontMySQLiDatabaseConnection, string, string, string, string) called at [<phabricator>/src/infrastructure/daemon/workers/query/PhabricatorWorkerArchiveTaskQuery.php:42]
[Mon Dec 29 19:23:08.326825 2014] [:error] [pid 4998] [client 10.0.2.2:56158]   #6 PhabricatorWorkerArchiveTaskQuery::execute() called at [<phabricator>/src/applications/daemon/controller/PhabricatorWorkerTaskDetailController.php:20]
[Mon Dec 29 19:23:08.326836 2014] [:error] [pid 4998] [client 10.0.2.2:56158]   #7 PhabricatorWorkerTaskDetailController::processRequest() called at [<phabricator>/src/aphront/AphrontController.php:33]
[Mon Dec 29 19:23:08.326848 2014] [:error] [pid 4998] [client 10.0.2.2:56158]   #8 AphrontController::handleRequest(AphrontRequest) called at [<phabricator>/webroot/index.php:103]
[Tue Dec 30 08:49:14.992486 2014] [mpm_prefork:notice] [pid 2178] AH00169: caught SIGTERM, shutting down
[Tue Dec 30 08:49:18.375612 2014] [mpm_prefork:notice] [pid 30732] AH00163: Apache/2.4.10 (Ubuntu) PHP/5.5.12-2ubuntu4.1 configured -- resuming normal operations
[Tue Dec 30 08:49:18.375968 2014] [core:notice] [pid 30732] AH00094: Command line: '/usr/sbin/apache2'
[Tue Dec 30 09:11:17.075158 2014] [mpm_prefork:notice] [pid 30732] AH00169: caught SIGTERM, shutting down
[Tue Dec 30 09:11:21.000497 2014] [mpm_prefork:notice] [pid 32077] AH00163: Apache/2.4.10 (Ubuntu) PHP/5.5.12-2ubuntu4.1 configured -- resuming normal operations
[Tue Dec 30 09:11:21.001027 2014] [core:notice] [pid 32077] AH00094: Command line: '/usr/sbin/apache2'

There are no errors when I go to an audit page.

Or do you mean there is a php/webserver directory in my server? I tried this command and only this many showed up. And there is no webserver directory underneath.

foresightyj@phabricatorvb:/usr/share/php$ sudo find / -type d -name 'php'
/usr/share/php
/usr/share/libgda-5.0/php
/home/foresightyj/arcanist/src/lint/linter/__tests__/php
/home/foresightyj/libphutil/src/lexer/__tests__/php

foresightyj updated the task description. (Show Details)Dec 30 2014, 5:17 AM

I dug a bit more into the problem this afternoon and found the cause.

I added the following logging statements in the DiffusionDiffController.php file.

foresightyj@phabricatorvb:~/phabricator$ git log --oneline -1
102e431 Migrate Maniphest task blockers to modern EdgeType classes
foresightyj@phabricatorvb:~/phabricator$ git diff
diff --git a/src/applications/diffusion/controller/DiffusionDiffController.php b/src/applications/diffusion/controller/DiffusionDiffController.php
index 4f6e3a8..79149d6 100644
--- a/src/applications/diffusion/controller/DiffusionDiffController.php
+++ b/src/applications/diffusion/controller/DiffusionDiffController.php
@@ -83,6 +83,13 @@ final class DiffusionDiffController extends DiffusionController {
     $ids = $pquery->loadPathIDs();
     $path_id = $ids[$changeset->getFilename()];

+    foreach($ids as $key => $value)
+    {
+       error_log("key: " . $key. " => " . $value);
+    }
+    error_log("path_id = ".$path_id);
+    error_log("filename = ".$changeset->getFilename());
+
     $parser->setLeftSideCommentMapping($path_id, false);
     $parser->setRightSideCommentMapping($path_id, true);

Then I restarted apache2, went to one of problem commit's page and refreshed. Then checked the error log:

foresightyj@phabricatorvb:~/phabricator$ tail -4 /var/log/apache2/error.log
[Wed Jan 14 16:51:21.701145 2015] [:error] [pid 16956] [client 10.0.2.2:18622] filename = Enterprise SNS Project/Enterprise360/02 /HuoHuo.Cloud.Enterprise360/Areas/WeiBo/Controllers/LetterController.cs
[Wed Jan 14 16:51:24.921871 2015] [:error] [pid 16856] [client 10.0.2.2:18623] key: Enterprise SNS Project/Enterprise360/05 /HuoHuo.Cloud.Enterprise.MessagePush/App.config =>
[Wed Jan 14 16:51:24.922053 2015] [:error] [pid 16856] [client 10.0.2.2:18623] path_id =
[Wed Jan 14 16:51:24.922082 2015] [:error] [pid 16856] [client 10.0.2.2:18623] filename = Enterprise SNS Project/Enterprise360/05 /HuoHuo.Cloud.Enterprise.MessagePush/App.config

But actually the file name is E:\FHT360\Enterprise SNS Project\Enterprise360\05 服务模块\HuoHuo.Cloud.Enterprise.MessagePush\App.config.

So it turns out that Phabricator discards all the Chinese characters in my file paths, and $ids keeps no value for the wrong path. As I went down further, I got confused quickly on the function getChangesets. I am new to PHP so I am asking if the Phabricator team can give me further help.

Some proof that I could access the file content on the same linux machine Phabricator runs on:

foresightyj@phabricatorvb:~/phabricator$ svn cat 'https://192.168.0.110/svn/FHT360/Enterprise SNS Project/Enterprise360/05 服务模块/HuoHuo.Cloud.Enterprise.MessagePush/App.config' | head -3
<?xml version="1.0" encoding="utf-8"?>
<configuration>
  <startup>

Probably if I change all folders to English characters, the problem will disappear instantly. But that is not an option now. My team will be utterly pissed off if I do that.

What version of Subversion and MySQL are you running?

I am in GMT+8. I cannot check my server at work right now as it is mid night. The subversion is VisualSVN installed in Windows. I will check he version tomorrow if needed. But accessing son from Ubuntu worked well as shown in my previous reply. The MySQL is installed by the phabricator's install script(the one that installs everything including GIT, Apache, MySQL). I Installed this version of Phabricator about one month ago so it should be pretty new.

Oh. In case you meant which SVN client I use. I installed that via apt-get a month ago as well.

I need the version number of Subversion you have installed on the server as well as the version number of MySQL that you are running.

No problem. I will give you those in 10 hours.

Here I have them:

MySQL Version:

foresightyj@phabricatorvb:~$ mysql -V
mysql  Ver 14.14 Distrib 5.5.40, for debian-linux-gnu (i686) using readline 6.3

SVN Client Version:

foresightyj@phabricatorvb:~$ svn --version
svn, version 1.8.10 (r1615264)
   compiled Aug 21 2014, 13:46:31 on i686-pc-linux-gnu

Copyright (C) 2014 The Apache Software Foundation.
This software consists of contributions made by many people;
see the NOTICE file for more information.
Subversion is open source software, see http://subversion.apache.org/

The following repository access (RA) modules are available:

* ra_svn : Module for accessing a repository using the svn network protocol.
  - with Cyrus SASL authentication
  - handles 'svn' scheme
* ra_local : Module for accessing a repository on local disk.
  - handles 'file' scheme
* ra_serf : Module for accessing a repository via WebDAV protocol using serf.
  - using serf 1.3.7
  - handles 'http' scheme
  - handles 'https' scheme

SVN Server Version:

C:\Program Files (x86)\VisualSVN Server\bin>svn --version
svn, version 1.6.17 (r1128011)
   compiled Jun  1 2011, 13:24:15

Copyright (C) 2000-2009 CollabNet.
Subversion is open source software, see http://subversion.apache.org/
This product includes software developed by CollabNet (http://www.Collab.Net/).

The following repository access (RA) modules are available:

* ra_neon : Module for accessing a repository via WebDAV protocol using Neon.
  - handles 'http' scheme
  - handles 'https' scheme
* ra_svn : Module for accessing a repository using the svn network protocol.
  - handles 'svn' scheme
* ra_local : Module for accessing a repository on local disk.
  - handles 'file' scheme
* ra_serf : Module for accessing a repository via WebDAV protocol using serf.
  - handles 'http' scheme
  - handles 'https' scheme

Please update SVN to 1.8 or newer on the server and let us know if that resolves your issue. This feels similar to T6891

I updated my VisualSVN. Here is the SVN version:

C:\Program Files (x86)\VisualSVN Server\bin>svn --version
svn, version 1.8.11 (r1643975)
   compiled Dec 12 2014, 20:22:25 on x86_64-microsoft-windows6.1.7601

Copyright (C) 2014 The Apache Software Foundation.
This software consists of contributions made by many people;
see the NOTICE file for more information.
Subversion is open source software, see http://subversion.apache.org/

The following repository access (RA) modules are available:

* ra_svn : Module for accessing a repository using the svn network protocol.
  - handles 'svn' scheme
* ra_local : Module for accessing a repository on local disk.
  - handles 'file' scheme
* ra_serf : Module for accessing a repository via WebDAV protocol using serf.
  - using serf 1.3.8
  - handles 'http' scheme
  - handles 'https' scheme

I restarted VisualSVN but I still get this:

foresightyj@phabricatorvb:~/phabricator$ tail -4 /var/log/apache2/error.log
[Thu Jan 15 09:42:35.797693 2015] [:error] [pid 1405] [client 10.0.2.2:24206] filename = Enterprise SNS Project/Enterprise360/02 /HuoHuo.Cloud.Enterprise360.PlatformAPI/RestLiteApi/DepartmentAndUser.cs
[Thu Jan 15 09:42:38.360821 2015] [:error] [pid 2147] [client 10.0.2.2:24209] key: Enterprise SNS Project/Enterprise360/05 /HuoHuo.Cloud.Enterprise.MessagePush/App.config =>
[Thu Jan 15 09:42:38.360992 2015] [:error] [pid 2147] [client 10.0.2.2:24209] path_id =
[Thu Jan 15 09:42:38.361052 2015] [:error] [pid 2147] [client 10.0.2.2:24209] filename = Enterprise SNS Project/Enterprise360/05 /HuoHuo.Cloud.Enterprise.MessagePush/App.config

T6891: Daemon fails to parse SVN Folder with Whitespace seems a bit different. He has errors in phd log. I don't have any. Here is my latest phd log, where the most recent error occurred 8 hours ago and was completely unrelated:

foresightyj@phabricatorvb:~/phabricator$ bin/phd log | tail -18
Daemon 199 STDE [Thu, 15 Jan 2015 01:34:19 +0000] STDERR
Daemon 199 STDE [Thu, 15 Jan 2015 01:34:19 +0000] svn: E000110: Unable to connect to a repository at URL 'https://192.168.0.110/svn/FHT360'
Daemon 199 STDE [Thu, 15 Jan 2015 01:34:19 +0000] svn: E000110: Error running context: Connection timed out
Daemon 199 STDE [Thu, 15 Jan 2015 01:34:19 +0000]  at [<phutil>/src/future/exec/ExecFuture.php:397]
Daemon 199 STDE [Thu, 15 Jan 2015 01:34:19 +0000]   #0 ExecFuture::resolvex() called at [<phabricator>/src/applications/repository/storage/PhabricatorRepository.php:292]
Daemon 199 STDE [Thu, 15 Jan 2015 01:34:19 +0000]   #1 PhabricatorRepository::execxRemoteCommand(string, string) called at [<phabricator>/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php:220]
Daemon 199 STDE [Thu, 15 Jan 2015 01:34:19 +0000]   #2 PhabricatorRepositoryDiscoveryEngine::verifySubversionRoot(PhabricatorRepository) called at [<phabricator>/src/applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php:148]
Daemon 199 STDE [Thu, 15 Jan 2015 01:34:19 +0000]   #3 PhabricatorRepositor... (939 more bytes) ... at [<phutil>/src/future/exec/ExecFuture.php:397]
Daemon 199 STDE [Thu, 15 Jan 2015 01:34:19 +0000]   #0 phlog(PhutilProxyException) called at [<phabricator>/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php:335]
Daemon 199 STDE [Thu, 15 Jan 2015 01:34:19 +0000]   #1 PhabricatorRepositoryPullLocalDaemon::resolveUpdateFuture(PhabricatorRepository, ExecFuture, integer) called at [<phabricator>/src/applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php:198]
Daemon 199 STDE [Thu, 15 Jan 2015 01:34:19 +0000]   #2 PhabricatorRepositoryPullLocalDaemon::run() called at [<phutil>/src/daemon/PhutilDaemon.php:91]
Daemon 199 STDE [Thu, 15 Jan 2015 01:34:19 +0000]   #3 PhutilDaemon::execute() called at [<phutil>/scripts/daemon/exec/exec_daemon.php:111]
Daemon 205 INIT [Thu, 15 Jan 2015 01:39:30 +0000] Starting process.
Daemon 206 INIT [Thu, 15 Jan 2015 01:39:31 +0000] Starting process.
Daemon 207 INIT [Thu, 15 Jan 2015 01:39:32 +0000] Starting process.
Daemon 208 INIT [Thu, 15 Jan 2015 01:39:33 +0000] Starting process.
Daemon 209 INIT [Thu, 15 Jan 2015 01:39:34 +0000] Starting process.
Daemon 210 INIT [Thu, 15 Jan 2015 01:39:36 +0000] Starting process.

His problem is related to empty spaces in path. But I don't have that problem. Remeber I said it is partially working. This page on my Phabricator works but its path has spaces in it.

It seems to be more related to character encoding.

I am really longing for this functionality to work in my setup. Will this be officially considered a bug?

Unfortunately, we don't have a reproducible case, and SVN in general is fairly popular with our CJK language installs, and we haven't see any other issues filed. Unfortunately there are too many nuanced variables outside of our control to continue guessing the issue remotely.

https://secure.phabricator.com/book/phabcontrib/article/bug_reports/#unreproducible-problems

We're happy to keep this filed as a bug. As more information becomes available, feel free to update it.

Thanks for the reply. I understand the situation.

I will create another SVN repo with Chinese characters in the path and see if it works. If it still has problems. I will dig a bit deeper into the PHP code when I have time.

I will update this task/bug as soon as I get anything meaningful.

I spent an afternoon on this bug today. Here are my findings. Hope it might be useful.

I updated to the last version first. Proof:

foresightyj@phabricatorvb:~/phabricator$ git log -1
commit 6d5aec86181c28bcd29cfa7e70a11a5b77cb69ac
Author: epriestley <git@epriestley.com>
Date:   Fri Feb 13 11:00:41 2015 -0800

    Allow logged-out users to accept invites on nonpublic installs

I made the following changes to the source code only:

foresightyj@phabricatorvb:~/phabricator$ git diff
diff --git a/src/applications/diffusion/query/DiffusionQuery.php b/src/applications/diffusion/query/DiffusionQuery.php
index 5a17550..73f76eb 100644
--- a/src/applications/diffusion/query/DiffusionQuery.php
+++ b/src/applications/diffusion/query/DiffusionQuery.php
@@ -76,9 +76,16 @@ abstract class DiffusionQuery extends PhabricatorQuery {
       $user,
       $drequest->getIsClusterRequest());
     if (!$client) {
-      return id(new ConduitCall($method, $params))
+
+       error_log("XXX here: " . $params['path']);
+
+        $stuff = id(new ConduitCall($method, $params))
         ->setUser($user)
         ->execute();
+
+       error_log("YYY there: " . $stuff['changes'][0]['currentPath']);
+
+       return $stuff;
     } else {
       return $client->callMethodSynchronous($method, $params);
     }
foresightyj@phabricatorvb:~/phabricator$

Here are the last six lines of the error log:

foresightyj@phabricatorvb:~/phabricator$ </var/log/apache2/error.log grep -P '(XXX|YYY)' | tail -6
[Sat Feb 14 18:39:28.480967 2015] [:error] [pid 25571] [client 10.0.2.2:47303] XXX here: Enterprise SNS Project/Enterprise360/05 \xe6\x9c\x8d\xe5\x8a\xa1\xe6\xa8\xa1\xe5\x9d\x97/HuoHuo.Cloud.Enterprise.MessagePush/App.config
[Sat Feb 14 18:39:28.499304 2015] [:error] [pid 25571] [client 10.0.2.2:47303] XXX here:
[Sat Feb 14 18:39:28.543731 2015] [:error] [pid 25571] [client 10.0.2.2:47303] XXX here: Enterprise SNS Project/Enterprise360/05 \xe6\x9c\x8d\xe5\x8a\xa1\xe6\xa8\xa1\xe5\x9d\x97/HuoHuo.Cloud.Enterprise.MessagePush/App.config
[Sat Feb 14 18:39:28.594253 2015] [:error] [pid 25571] [client 10.0.2.2:47303] YYY there:
[Sat Feb 14 18:39:28.607729 2015] [:error] [pid 25571] [client 10.0.2.2:47303] YYY there:
[Sat Feb 14 18:39:29.183507 2015] [:error] [pid 25571] [client 10.0.2.2:47303] YYY there: Enterprise SNS Project/Enterprise360/05 /HuoHuo.Cloud.Enterprise.MessagePush/App.config

Basically, the path is screwed up in this call id(new ConduitCall($method, $params)). I then traced ConduitCall down to ConduitAPIRequest and the getter getAllParameters. I was lost right there. Also I can find nowhere id was defined.

I am pretty new to PHP and I cannot easily mock the environment to test this line id(new ConduitCall($method, $params)) in isolation.

epriestley moved this task from Backlog to Inlines on the Audit board.Jan 10 2017, 4:53 PM

	F268139: spaceok.jpg
	Jan 15 2015, 1:49 AM

	F258268: my2.PNG
	Dec 30 2014, 2:53 AM

	F258264: my1.PNG
	Dec 30 2014, 2:53 AM

	F258266: their.PNG
	Dec 30 2014, 2:53 AM

Inline comments is only partially working in an imported SVN repoOpen, Needs TriagePublicActions

Description

Related Objects

Event Timeline

Inline comments is only partially working in an imported SVN repo
Open, Needs TriagePublic
Actions