Page MenuHomePhabricator

Arcanist allows to commit unreviewed changeset (SVN)
Closed, WontfixPublic

Description

I have configured Phabricator with Herald rules to pre commit code review & SVN hosted repo.
I did following:

  1. made changes in a file
  2. arc diff
  3. made another changes in same file
  4. accept changes in Differential
  5. arc commit

At the end I've got both changesets committed into SVN however only first changeset has been requested and accepted for review.
Is this expected behavior or not?

Event Timeline

jurgis raised the priority of this task from to Needs Triage.
jurgis updated the task description. (Show Details)
jurgis added a project: Arcanist.
jurgis added a subscriber: jurgis.

This is the expected behavior.

Well, is this for some reason? I guess it could be possible to commit only diff requested for review and keep all other changes as uncommitted (I know it's not possible with svn straightforward but seems TortoiseSVN client has done it http://tortoisesvn.net/docs/release/TortoiseSVN_en/tsvn-dug-commit.html#tsvn-dug-commit-restore).
Another option would be to prohibit commit saying changeset is wider then requested for review.

This comment was removed by chad.
epriestley claimed this task.
epriestley added a subscriber: epriestley.

This workflow is inherently unsafe, and can not be made safe.

Even if we did implement partial-commit, we can't run local unit tests on only some of your changes, because there's no way to tell PHP or Python or Node or make to only look at some of the changes. Your test results and manual testing may be incorrect because of additional changes you've made locally.

You can isolate changes completely by using multiple working copies, or using a VCS with local branching (like Git or Mercurial), or by using a Git-SVN bridge to use another VCS frontend. Of these approaches, simulating local branching by using multiple working copies is likely easiest (I used multiple working copies for the duration of my time at Facebook without issue).

We do make an effort to detect when the files differ, because this is straightforward, but we could only make this inherently unsafe workflow slightly safer by looking at changes within files, and doing so would be much more complex and inherently error-prone (we can't, in the general case, tell when a change is a typo fix vs a response to feedback vs an update from upstream vs a distinct change in the same working copy).

Specifically, the problematic workflow is this:

  1. Make some changes and arc diff them.
  2. Work on a different feature in the same working copy.

This workflow is inherently unsafe and can not be made safe. We don't plan to take further steps to try to make it safer.