Page MenuHomePhabricator

Add "requestedObjectPHID" to ReleephRequest
ClosedPublic

Authored by epriestley on Apr 20 2014, 3:54 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Dec 22, 10:08 PM
Unknown Object (File)
Sat, Dec 14, 12:53 AM
Unknown Object (File)
Wed, Dec 11, 5:28 PM
Unknown Object (File)
Sun, Dec 8, 12:12 AM
Unknown Object (File)
Wed, Dec 4, 6:19 AM
Unknown Object (File)
Fri, Nov 29, 8:18 PM
Unknown Object (File)
Nov 26 2024, 1:57 AM
Unknown Object (File)
Nov 22 2024, 11:27 PM
Subscribers

Details

Reviewers
btrahan
Maniphest Tasks
Restricted Maniphest Task
Commits
Restricted Diffusion Commit
rP1a3ac099751b: Add "requestedObjectPHID" to ReleephRequest
Summary

Ref T3551. Currently, ReleephRequests don't have a direct concept of the object being requested. You can request D123, but that is just a convenient way to write rXyyyy.

When the UI wants to display information about a revision, it deduces it by examining the commit.

This is primarily an attack on T3551, so we don't need to load <commit -> edge -> revision> (in an ad-hoc way) to get revisions. Instead, when you request a revision we keep track of it and can load it directly later.

Later, this will let us do more things: for example, if you request a branch, we can automatically update the commits (as GitHub does), etc. (Repository branches will need PHIDs first, of course.)

This adds and populates the column but doesn't use it yet. The second part of the migration could safely be run while Phabricator is up, although even for Facebook this table is probably quite small.

Test Plan
  • Ran migration.
  • Verified existing requests associated sensibly.
  • Created a new commit request.
  • Created a new revision request.

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

epriestley retitled this revision from to Add "requestedObjectPHID" to ReleephRequest.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: btrahan.
epriestley added a task: Restricted Maniphest Task.
btrahan edited edge metadata.

I only looked at the surrounding code briefly, but I do wonder if there's something that could be cleared up here re: inBranch vs pickStatus? Nothing important mind you - i.e. ignore me if you want, just geeking out a little since this has a lot of code quality stuff going on.

src/applications/releeph/storage/ReleephRequest.php
12–13

do these two really need to be separate?

I am asking 'cuz "setInBranch" calls seemed awkward while specifying branchID and attaching a branch...

This revision is now accepted and ready to land.Apr 20 2014, 4:56 PM

This stuff can definitely get cleaned up, but it's harder to move forward with because it will require changes to Conduit and the arc integration. The integration isn't even in the upstream, so I'm giving that stuff the widest berth I can for now and trying to only make trivial changes (e.g., call method X instead of method Y, but they both do effectively the same thing).

In the case of pickStatus/inBranch, they probably should be the same (i.e. pick status should imply inbranch), but I think it's currently possible to get an already-merged change back into an unpulled state, maybe. There's some code which suggests this can happen, at least.

The commitPHID and commitIdentifier also partially duplicate some of the information here, since they mean "the commit this request was merged as".

epriestley updated this revision to Diff 20940.

Closed by commit rP1a3ac099751b (authored by @epriestley).