Page MenuHomePhabricator

Add "arc diff --draft"
ClosedPublic

Authored by epriestley on Oct 27 2017, 5:24 PM.

Details

Summary

Experimental branch. Ref T2543. Depends on D18742.

Add an "arc diff --draft" flag which holds revisions as drafts indefinitely.

Test Plan

Ran "arc diff --draft" when creating; ran "arc diff --draft" to try to update a revision and got a failure.

Diff Detail

Repository
rARC Arcanist
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley created this revision.Oct 27 2017, 5:24 PM
epriestley requested review of this revision.Oct 27 2017, 5:24 PM
amckinley accepted this revision.Oct 28 2017, 3:35 PM
amckinley added inline comments.
src/workflow/ArcanistDiffWorkflow.php
593–596

I guess we don't do too much of this in general, and we shouldn't force people to upgrade unless they're actually going to be using --draft, but I wonder if we should ever build an generic versioned API instead of explicitly doing checks like this.

This revision is now accepted and ready to land.Oct 28 2017, 3:35 PM

We had a versioned API at one point and moved away from it (there's still some code in ConduitConnectConduitAPIMethod). Some reasons include:

  • Prototypes, extensions, configuration, and the ability to install/uninstall applications mean that two installs of Phabricator at version X may expose different capabilities via the API.
  • Sometimes we'll add a feature for a third-party, and not use it in arc until some time later. With capability tests, this isn't a problem. With versioning, we'd need to bump the version on every change, or accept that arc will claim that some versions which have the capability (between when it was added, and when the version was bumped) don't have the capability, which is confusing to users.
  • The implementation forced upgrades after checking versions ("you are out of date, upgrade"), and users hated it. An implementation wouldn't have to do this, but if we replace capability tests with version tests and don't force upgrades, we end up with the same code, just if ($version >= $version_with_draft) instead of if ($has_draft_capability), but both blocks are still inline somewhere in the actual code along the execution pathway for the feature.
  • Often, we can navigate API changes transparently (e.g., file.upload uses modern stuff if available and falls back if not).
This revision was automatically updated to reflect the committed changes.