Page MenuHomePhabricator

Flexible differential revision tags
Closed, WontfixPublic


Phabricator's standard tag for code reviews is Differential Revision:. In the FreeBSD project we have a variety of existing commit metadata tags like PR: for bug reports, Reviewed by: for reveiewers, etc, and would like to be able to associate a commit with a code review using a tag in the same scheme.

FreeBSD developers have been using a variety of tags so far -- some examples:

CR:         D482
Phabric:    D528
Phabric: D525 (part of a larger patch)
Differential Revision:

I think we'd like to settle on the CR: form. We'd probably use just the D#### Differential ID, but could use the URL instead.

Would it be feasible to add an option to specify text other than 'Differential Revision'?

Event Timeline

emaste raised the priority of this task from to Needs Triage.
emaste updated the task description. (Show Details)
emaste added a project: Differential.
emaste added a subscriber: emaste.
epriestley claimed this task.

We aren't going to change this in the upstream since it's complicated and error prone and a rare use case.

You can locally allow DifferentialRevisionIDField to be disabled (change canDisableField() to return true), then disable it (in differential.fields in config), then copy/paste it and implement a new version of the field which does whatever you want. You can find some general discussion here:

Alternatively, you can just fork DifferentialRevisionIDField, implement getCommitMessageLabels() and have it recognize "CR", "Phabric", etc., and adjust parseRevisionIDFromURI() to be liberal (and possibly renderCommitMessageValue() to render only Dnnn).

(We require full URIs instead of just D123 so that importing a third-party repository which also uses Phabricator for code review won't associate with / close revisions. However, we also have this problem with Fixes T123 in commit messages, so we may need to pursue some other more general approach eventually.)

That actually may not work -- ArcanistDifferentialCommitMessage::newFromRawCorpus() hard-codes the "Differential Revision:" field in arc. Although it sounds like you probably aren't using arc anyway if you're ending up with a variety of manual message fields.

Some developers are using arc (I suspect the ones with Differential Revision in commit messages).

Good point about the URL vs Dnnn-only parsing, we have that problem today when referencing SVN revisions from contrib software (e.g. the copy of LLVM we have in the FreeBSD tree).