Page MenuHomePhabricator

Attach identities to commits and users to identities
ClosedPublic

Authored by amckinley on Jun 12 2018, 9:10 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Jul 23, 2:57 AM
Unknown Object (File)
Sun, Jul 21, 10:41 AM
Unknown Object (File)
Mon, Jul 15, 11:37 PM
Unknown Object (File)
Thu, Jul 4, 7:05 AM
Unknown Object (File)
Tue, Jul 2, 3:51 PM
Unknown Object (File)
Sat, Jun 29, 5:52 AM
Unknown Object (File)
Sat, Jun 29, 2:40 AM
Unknown Object (File)
Jun 17 2024, 3:11 AM

Details

Summary

Ref T12164. Make it easier to work with identity objects by attaching them to commits and attaching users to identities.

Test Plan

Loaded some commits with ->needIdentities(true) and checked the resulting objects.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley added inline comments.
src/applications/diffusion/query/DiffusionCommitQuery.php
407

Prefer using IdentityQuery, mostly just so the rules end up consistent if we add view policy stuff later, and sometimes we start doing implicit attachment or weird filtering or whatever.

(The guidance is roughly "use Query if it exists".)

src/applications/repository/query/PhabricatorRepositoryIdentityQuery.php
134–135

Prefer PeopleQuery, per above.

src/applications/repository/storage/PhabricatorRepositoryCommit.php
198

At least for now, allowing $author to = null may avoid some issue: for example, installs that haven't run rebuild-identities yet won't get an Author identity for older commits and will try to attach null, I think.

src/applications/repository/storage/PhabricatorRepositoryIdentity.php
24–29

Consider removing the boolean flag and just using hasEffectiveUser() at callsites -- this is a little more standard/readable, and getEffectiveUser(false) can be a little hard to guess the meaning of.

This revision is now accepted and ready to land.Jun 12 2018, 10:24 PM
amckinley added inline comments.
src/applications/repository/storage/PhabricatorRepositoryCommit.php
198

Yep, but I intend to land this after the migration. Does it still make sense to allow null under those circumstances?

This revision was automatically updated to reflect the committed changes.
joshuaspence added inline comments.
src/applications/repository/storage/PhabricatorRepositoryCommit.php
208

This should probably be getCOmmitterIdentity

src/applications/repository/storage/PhabricatorRepositoryCommit.php
208

Good catch!

src/applications/repository/storage/PhabricatorRepositoryCommit.php
208

I assume you figured it out, but the capitalization of "o" wasn't intentional.