Page MenuHomePhabricator
Create Task
Maniphest T6718

Diff created at commit time has wrong paths in Subversion with subpath set
Open, WishlistPublic
Actions

Description

When sending a diff (via arc diff), then paths in the diff are relative to working copy:

  • working copy path: projects/project-name
  • absolute path in repo: projects/project-name/sub-path/index.php
  • path in diff: sub-path/index.php

When diff is created as a result of commit, which have Differential Revision: .... text in it, then paths are different:

  • absolute path in repo: projects/project-name/sub-path/index.php
  • path in diff (for fully imported repo): projects/project-name/sub-path/index.php
  • path in diff (for sub-path only imported repo): project-name/sub-path/index.php (when sub-path is projects/)

If later we compare diff, that was last submitted via arc diff and one created automatically then we'll see mass remove and mass add, which isn't very helpful.

I know that Phabricator doesn't recognize branches/tags in SVN repositories, but at least what can be done is in https://secure.phabricator.com/diffusion/P/browse/master/src/applications/diffusion/query/rawdiff/DiffusionSvnRawDiffQuery.php;54f8aa8cd9f497c964b046d1de94704446a81469$23 line unify handling of repositories, that are fully and partially imported.

Event Timeline

aik099 created this task.Dec 9 2014, 3:48 PM
aik099 raised the priority of this task from to Needs Triage.
aik099 updated the task description. (Show Details)
aik099 added a project: Subversion.
aik099 added a subscriber: aik099.
epriestley triaged this task as Wishlist priority.Dec 9 2014, 3:53 PM
epriestley added a subscriber: epriestley.

We can't always use paths relative to the subpath because a commit may affect files under the subpath and also files outside of the subpath. In this case, we should faithfully represent the commit (show all files), not discard the changes outside of the subpath.

aik099 added a comment.Dec 9 2014, 5:21 PM

Are you sure? In the executed svn diff command the final argument would be svn://some.repo.com/sub-path/here if only sub-path/here is imported in a particular repository.

So in that case I'm not sure what svn diff will return: only changes in sub-path/here folder or also outside it.

epriestley renamed this task from Diff created at commit time has wrong paths to Diff created at commit time has wrong paths in Subversion with subpath set.Dec 9 2014, 5:28 PM

Are you sure?

Yes. I'm describing a product requirement, not a technical limitation. Obviously, it's trivially easy to discard files and show only the part of the commit beneath the subpath. This would also be confusing. We should not do this: we should always show the entire commit.

aik099 added a comment.Dec 20 2014, 12:37 PM

Unfortunately changing the DiffusionSvnRawDiffQuery::executeQuery as I originally suspected might be wrong because for example the DiffusionCommitController::buildRawDiffResponse specifically passes path parameter that it expects diff to be built against.

Alternative solution would be to pass path parameter in the PhabricatorRepositoryCommitMessageParserWorker::generateFinalDiff method (https://secure.phabricator.com/diffusion/P/browse/master/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php;0ce08b4d279b807ded7af58f4cc7a5af97fb5944$248-254), which will have:

  • / for SVN
  • . for Git/Mercurial

In fact we can only pass path parameter to the SVN repositories because Git/Mercurial will defaults to . anyway when empty path is given (or no path is given at all).

Phabricator · Built by Phacility