Page MenuHomeFreeBSD

Include Differential Revision field in subversion freebsd template
AbandonedPublic

Authored by xmj on Aug 13 2014, 8:23 AM.

Details

Reviewers
swills
emaste
xmj
koobs
mat
Group Reviewers
portmgr
Summary

Patch attached amends devel/subversion's FreeBSD template to include lines for
Differential Revision: that can reference Phabric differentials from subversion commits, and auto-close them.

Can be copied to devel/subversion17 easily; needs more work for
devel/subversion16 whose fbsd template is outdated.

Test Plan

compiled locally;
tested with poudriere on 11.0-CURRENT
http://xmj.me/freebsd/buildlogs/subversion-1.8.9_7.log
and on all other archs through redports
https://redports.org/buildarchive/20140813073721-70548/

Diff Detail

Repository
rP FreeBSD ports repository
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

xmj retitled this revision from to Include Code Review (CR:) field in subversion freebsd template.
xmj updated this object.
xmj edited the test plan for this revision. (Show Details)
xmj added reviewers: portmgr, koobs, swills.

I'd love to add Core Team, lev and ohauer as reviewers to this revision, but they don't seem to have Phabric accounts.

are there any workarounds to manually add reviewer's emails?

mat added inline comments.
devel/subversion/files/extra-patch-fbsd-template
26

I think it should be before PR:, I quite like having PR and Submitted by next to one another because you don't use PR without using Submitted by.

emaste requested changes to this revision.Aug 13 2014, 12:40 PM
emaste added a reviewer: emaste.
emaste added a subscriber: emaste.

The canonical version is:

Differential Revision: https://reviews.freebsd.org/D#####

as the last line of the template, as this integrates with Phabricator's auto-closing logic. See for example D529.

This revision now requires changes to proceed.Aug 13 2014, 12:40 PM
In D600#7, @emaste wrote:

The canonical version is:

Differential Revision: https://reviews.freebsd.org/D#####

as the last line of the template, as this integrates with Phabricator's auto-closing logic. See for example D529.

Oh, didn't know that was possible, that is a great thing.

Oh, didn't know that was possible, that is a great thing.

Yeah, it's nifty. I tried to get the upstream Phabricator team to implement flexible tags (so we could e.g. use CR: and omit the full url, using just D####), but they were not interested in making that change.

In D600#13, @emaste wrote:

Oh, didn't know that was possible, that is a great thing.

Yeah, it's nifty. I tried to get the upstream Phabricator team to implement flexible tags (so we could e.g. use CR: and omit the full url, using just D####), but they were not interested in making that change.

Are they willing to take patches? There are a lot of annoying things phabricator does that would be nice to fix.

they willing to take patches? There are a lot of annoying things phabricator does that would be nice to fix.

They're generally quite responsive for bug fixes. I think in general they would be receptive to patches for improvements too.

The specific case here was a design decision they don't agree with, I guess.

xmj edited edge metadata.

Refactor CR: into Differential Revision:,
put as last item, and add tabspacing.

xmj edited edge metadata.

Put Differential Revision: below PR:

Consistent placing of Differential Revision: in prefixes.

xmj retitled this revision from Include Code Review (CR:) field in subversion freebsd template to Include Differential Revision field in subversion freebsd template.Sep 10 2014, 1:15 PM
xmj updated this object.
mat added a reviewer: mat.

+1 on implementation
-1 on naming

Propose: "Differential Code Revision Review in Phabric:"

Upstream not accepting patches (or support trivial customisation) is a fairly weak position, especially given we have template patches for SVN (among other things)

koobs edited edge metadata.

I (mentor) approve the differential revision in syntax only

In D600#29, @koobs wrote:

+1 on implementation
-1 on naming

Propose: "Differential Code Revision Review in Phabric:"

Upstream not accepting patches (or support trivial customisation) is a fairly weak position, especially given we have template patches for SVN (among other things)

The problem is not upstream not accepting patches, it's that if you want to change Differential Revision to something else like CR you then have to change arc to use it, and then, our arc would not work with any other phabric, because it would use the wrong magic string.

xmj added a reviewer: xmj.

Committed revision 367920.

I also dislike Differential Revision:, but dislike the idea of maintaining our own patched Phabricator and arc even more. Beyond the arc issue @mat points out, Phabricator uses the tag for automatically closing the review upon commit.

Landed in 367920 and precised by @mat in 368454 -- can't be closed with prior rejects.