Begin building out RepositoryIdentity indirection layer
ClosedPublic
Actions

Authored by amckinley on May 1 2018, 8:36 PM.

Details

Reviewers

epriestley

Maniphest Tasks

T12164: Put an indirection layer between author/committer strings and user accounts

Commits

rPcd84e53c4413: Begin building out RepositoryIdentity indirection layer

Summary

Ref T12164. Start building initial objects for managing RepositoryIdentity objects. This won't land until much more of the infrastructure is in place.

Test Plan

Ran bin/storage upgrade and observed expected table.

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

amckinley created this revision.May 1 2018, 8:36 PM

Owners added a subscriber: Restricted Owners Package.May 1 2018, 8:36 PM

Harbormaster completed remote builds in B20273: Diff 46463.May 1 2018, 8:37 PM

amckinley requested review of this revision.May 1 2018, 8:37 PM

Herald added a subscriber: Korvin. · View Herald TranscriptMay 1 2018, 8:37 PM

epriestley accepted this revision.May 1 2018, 9:15 PM

epriestley added inline comments.

src/applications/repository/storage/PhabricatorRepositoryIdentity.php
7	`phid` is added implicitly and doesn't need to be listed as a protected property. (If this was copy/paste, the other one can be cleaned up too.)
8	There's no natural epoch on identities so I think we can drop this.
9	Unless you have plans for it, I don't think we need an `identityType` -- there's only one type of identity.
20	We could let these have normal timestamps, I think. There shouldn't be all that many of them and I think they'll be fairly normal objects.
38–41	You can omit this -- it's added automatically. Some older tables had an unusual key name, so they still specify it manually, but newer tables can just use the default stuff.
46–49	In modern code, prefer overriding the slightly simpler `getPHIDType()`.

This revision is now accepted and ready to land.May 1 2018, 9:15 PM

amckinley marked 6 inline comments as done.May 1 2018, 9:18 PM

amckinley added inline comments.

src/applications/repository/storage/PhabricatorRepositoryIdentity.php
8	I can think of lots of reasons we'd want to track the creation/modification of identities, but I guess adding transaction support will take care of that.

amckinley marked an inline comment as done.May 1 2018, 9:19 PM

amckinley added inline comments.

src/applications/repository/storage/PhabricatorRepositoryIdentity.php
35	So should the unique column be on the hash or the raw value? Since we search by hash, I can see it making sense to make that column unique.

The key must be on the hash because keys may not be added on more than a certain number of characters (384 with current utf8 stuff, maybe) but you can have two identities like "XXX...XXXA" and "XXX...XXXB", where the first 400 characters are the same and only the 5382th character is different or whatever. No one "should" ever do this but user creativity is boundless.

The major reason for the hash is because we can't just put a key on an arbitrarily long column. The hash doesn't inherently give us a performance boost or anything, it just lets us effectively put a UNIQUE KEY on a gigantic blob of nonsense when, e.g., some creative user commits with a 1MB email address.

amckinley added inline comments.May 1 2018, 9:24 PM

src/applications/repository/storage/PhabricatorRepositoryIdentity.php
8	Oh, `CONFIG_TIMESTAMPS => true` fixes this.

epriestley added inline comments.May 1 2018, 9:26 PM

src/applications/repository/storage/PhabricatorRepositoryIdentity.php
8	Yeah, that plus transactions should give us pretty good records (and `true` should be the default so you can just omit the config completely, I believe).