Page MenuHomePhabricator
Create Task
Maniphest T6537

The "Skip Past This Commit" results in exception (diffusion svn blame view)
Open, NormalPublic
Actions

Description

How to reproduce:

  1. open file for viewing in Diffusion
  2. enable blame view by clicking on Enable Blame on top right
  3. click on "<<" icon in left column next to any change
  4. url would be like http://phabricator.domain.com/diffusion/REPOCALLSIGN/browse/path/to/file.tpl;revision_number$line_number?before=skip_revision_number&view=blame

See this exception:

>>> UNRECOVERABLE FATAL ERROR <<<

Call to a member function getID() on a non-object

/home/sites/phabricator/web/phabricator/src/applications/diffusion/query/DiffusionRenameHistoryQuery.php:86


┻━┻ ︵ ¯\_(ツ)_/¯ ︵ ┻━┻

This is the line: https://secure.phabricator.com/diffusion/P/browse/master/src/applications/diffusion/query/DiffusionRenameHistoryQuery.php;120a7d91644753734bbdcdc7f8b50385829a0a5d$86

I guess the commit that was searched wasn't found in that repository (maybe commit was produced as a result of SVN copy from path, that wasn't tracked by that repository).

Revisions and Commits

rP Phabricator
Needs ReviewD10840 Fix parent commit detection for SVN repositories with sub-path

Related Objects

Event Timeline

aik099 created this task.Nov 12 2014, 5:39 PM
aik099 raised the priority of this task from to Needs Triage.
aik099 updated the task description. (Show Details)
aik099 added a project: Diffusion.
aik099 added a subscriber: aik099.
aik099 added a comment.Nov 12 2014, 5:46 PM

While debugging the issue I've figured out possible cause: in SVN the code, that detects previous commit on that file line somehow goes past sub-path of current repository (I wonder how this even possible) and commitIdentifier from another repository is found. When that commitIdentifier is combined with current repository callsign this results in bad commit.

aik099 added a comment.Nov 12 2014, 6:13 PM

Digging even deeper I've found out that diffusion.commitparentsquery API call is used to determine commit parent. However it seems to work like this for SVN repositories: parent of this commit is this commit minus one: https://secure.phabricator.com/diffusion/P/browse/master/src/applications/diffusion/query/lowlevel/DiffusionLowLevelParentsQuery.php;120a7d91644753734bbdcdc7f8b50385829a0a5d$73

This is a mistake because commit found that way might belong to another repository, which is happening exactly here.

Test scenario:

  1. create SVN repository with following structure:
project1
project2
  1. create 2 repositories in Phabricator, that would import only project1 and project2 sub-path respectively
  2. make commit to project1 folder
  3. make commit to project2 folder
  4. make commit to project1 folder
  5. open last commit in Diffusion in blame view and use Skip Past This Commit

You'll get exception, because previous commit that is discovered doesn't belong to this repository.

aik099 added a comment.EditedNov 12 2014, 6:27 PM

Solution:

  1. if we're importing full SVN repository, then current implementation is correct
  2. if we're importing sub-path only then we should do svn log svn://repo.url/sub/path --revision current_revision:1 --limit 1 command and get returned revision back

I can submit a patch for that if there is a need, but I'm unsure what is the best way to execute SVN command on remote repository with credentials.

aik099 added a revision: D10840: Fix parent commit detection for SVN repositories with sub-path.Nov 12 2014, 7:18 PM
aik099 added a comment.Nov 12 2014, 7:20 PM

I've submitted a Differential Revision with a fix to described issue.

chad triaged this task as Normal priority.Nov 15 2014, 2:05 AM
chad added a project: Subversion.
aik099 mentioned this in T5886: Commit imported into wrong repository (as duplicate).Nov 24 2014, 12:00 PM
aik099 added a subscriber: epriestley.Nov 24 2014, 2:38 PM

@epriestley, could you please confirm that a described behavior is a bug? If that is a bug, then please take a look at the submitted fix.

epriestley moved this task from Backlog to Subversion on the Diffusion board.Apr 15 2019, 3:38 PM
Herald added a subscriber: amckinley. · View Herald TranscriptApr 15 2019, 3:38 PM
Phabricator · Built by Phacility