Page MenuHomePhabricator
Create Task
Maniphest T5799

Flexible differential revision tags
Closed, WontfixPublic
Actions

Description

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: https://phabric.freebsd.org/D433
CR:         D482
Phabric:    D528
Phabric: D525 (part of a larger patch)
Phabric:    https://phabric.freebsd.org/D443
Differential Revision: https://phabric.freebsd.org/D521

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 created this task.Aug 5 2014, 6:20 PM
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 closed this task as Wontfix.Aug 5 2014, 6:30 PM
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:

https://secure.phabricator.com/book/phabricator/article/custom_fields/

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.)

epriestley added a comment.Aug 5 2014, 6:32 PM

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.

emaste added a comment.Aug 5 2014, 6:49 PM

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).

emaste added a project: FreeBSD.May 29 2015, 9:18 PM
emaste added a subscriber: eadler.May 29 2015, 9:22 PM
Phabricator · Built by Phacility