Improve clarity of commit and symbol handling in DiffusionRequest
ClosedPublic
Actions

Authored by epriestley on May 13 2014, 1:49 PM.

Details

Reviewers

btrahan

Maniphest Tasks

T2683: Improve the performance of Diffusion browse views

Commits

Restricted Diffusion Commit
rP347252fda85a: Improve clarity of commit and symbol handling in DiffusionRequest

Summary

Ref T2683. Currently, DiffusionRequest has four different "commitey" things:

commit
rawCommit
symbolicCommit
stableCommit

Of these, only two are actually distinct, useful values: symbolicCommit (which holds the value the request originally contained, if one existed) and stableCommit (which resolves that value, or the value implied by its omission, into a stable, permanent commit identifier).

rawCommit is equivalent to symbolicCommit and can be simply removed.
commit has some sketchy magic around it that needs to be pulled out before it can be jettisoned.

Test Plan

Viewed SVN, Git, and Mercurial repositories. Viewed brwose/history/change/tag/branch/etc views.

Diff Detail

Repository

rP Phabricator

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

epriestley updated this revision to Diff 21610.May 13 2014, 1:49 PM

epriestley retitled this revision from to Improve clarity of commit and symbol handling in DiffusionRequest.

epriestley updated this object.

epriestley edited the test plan for this revision. (Show Details)

epriestley added a reviewer: btrahan.

epriestley added a task: T2683: Improve the performance of Diffusion browse views.

Herald added a subscriber: epriestley. · View Herald TranscriptMay 13 2014, 1:49 PM

Harbormaster completed remote builds in B430: Diff 21610.May 13 2014, 1:51 PM

V. glad you're cleaning this up btw. This always confused me a bunch.

src/applications/diffusion/controller/DiffusionChangeController.php
56	should be Symbolic maybs?
src/applications/diffusion/controller/DiffusionController.php
101–111	what's up with stable vs symbolic here, given it was all raw before? I think stable makes more sense for a link and maybe we had bugs before where raw / symbolic were omitted. How come not just use it in both places?
src/applications/diffusion/controller/DiffusionHistoryController.php
155	D9091 merge issue methinks

This revision is now accepted and ready to land.May 13 2014, 4:48 PM

epriestley added inline comments.May 13 2014, 4:55 PM

src/applications/diffusion/controller/DiffusionController.php
101–111	These were theoretically buggy but they aren't normally reachable if the raw commit has a value which can trigger issues. The `commit` property could also get expanded sometimes and basically was unpredictable black magic through and through. I'm using stable for the name because rendering something like "rPHEAD^^^" in the UI doesn't make sense. I'm using symbolic for the link because if you're not pinned to a specific commit (or at "HEAD^^^" or something because you edited the URL or we add that kind of capability in the future) we don't (usually) want to pin you to one.