Page MenuHomePhabricator

Audit Changes TOC fail redirecting to right subdirectory when clicking on fa-folder-open icon and produces an unhandled exception
Closed, DuplicatePublic

Description

Versions

Os

Debian 8

Phabricator

phabricator 20e6a4200dbc72760ea5de4eb15053bc1b2c8dac (Sun, Dec 6)
arcanist 4a680c762b2e6b191d339dd3306100582b702722 (Fri, Dec 4)
phutil 0b37f385b8cd0d36c1d143949bb67d89465fbf94 (Sat, Dec 5)

Reproductions steps

  • create a hosted repository A
  • create a hosted repository B
  • in A/ do `git submodules add ssh://example.com/path/to/B
  • makes some changes in A/B/, commit (com1longhash) them and push them to server
  • makes some changes in A/., commit (com2longhash) them and push them to server
  • browse the secon audit example.com/com2longhash
  • in the changes table of content try to browse A/B by clicking on the fa-folder-open
  • it redirect to example.com/diffusion/A/browse/master/B;com1longhash instead of example.com/diffusion/B/browse/master/ and thus produces something like

subdirectory.png (370×1 px, 31 KB)

Note that when doing the same thing in diffusion browse view (example.com/diffusion/A/browse/master) ie clicking on the fa-folder-open it does correctly redirect to example.com/diffusion/B/browse/master/

Event Timeline

tycho.tatitscheff raised the priority of this task from to Needs Triage.
tycho.tatitscheff updated the task description. (Show Details)
tycho.tatitscheff changed the edit policy from "All Users" to "Custom Policy".
tycho.tatitscheff added a project: Bug Report.
tycho.tatitscheff updated the task description. (Show Details)

@epriestley, do you see it can be easy enough so I can to submit some patch ?

My digging is that :

  • DiffusionBrowseDirectoryController use DiffusionBrowseTableView
  • DiffusionBrowseTableView does the think right (see here)
  • DiffusionCommitController does not use DiffusionBrowseTableView but DiffusionHistoryTableView
  • the build of table of content is done in DiffusionCommitController::buildTableOfContents() and in peculiar here, it does not take into account the case if the $file_type == DifferentialChangeType::FILE_SUBMODULE

Or for evrything else than DifferentialChangeType::FILE_SUBMODULE the browse link must be :

$browse_link = phutil_tag('strong', array(), $this->linkBrowse(
  $full_path.$dir_slash,
  array(
    'type' => $file_type,
    'name' => $browse_text,
  )));

but for DifferentialChangeType::FILE_SUBMODULE, it should be somethink like :

$browse_link = phutil_tag('strong', array(), $this->linkBrowse(
  null,
  array(
    'type' => $file_type,
    'name' => $browse_text,
    'hash' => $path->getHash(),
    'external' => $path->getExternalURI(),
  )));

Let me know if you agree to let me submit a simple patch with a switch case.
If you want some bigger refont like moving building table of content to a new view or more, I surely can help, but will need more help.

Of course I would double check it this time, and really try the last diff send in the production environnement where i get the problem, to be sure it don't break the world like last time.

eadler added a project: Restricted Project.Aug 5 2016, 5:05 PM