Page MenuHomePhabricator

Phragment v0

Authored by hach-que on Dec 6 2013, 3:09 AM.
Referenced Files
F12185732: D7726.id17470.diff
Sat, Sep 16, 1:24 PM
F12181166: D7726.id17444.diff
Tue, Sep 12, 5:50 PM
F12177225: D7726.diff
Sun, Sep 10, 8:09 PM
F12161202: D7726.diff
Wed, Sep 6, 2:25 PM
F12160077: D7726.diff
Wed, Sep 6, 6:21 AM
F12154412: D7726.id17467.diff
Mon, Sep 4, 7:22 AM
F12145273: D7726.diff
Aug 31 2023, 6:05 PM
F12136818: D7726.id17469.diff
Aug 26 2023, 11:34 AM


Group Reviewers
Blessed Reviewers
Maniphest Tasks
T4205: Implement Phragment
Restricted Diffusion Commit
rP4c143ad3b2c4: Phragment v0

Ref T4205. This is an initial implementation of Phragment. You can create and browse fragments in the system (but you can't yet view a fragment's patches / history).

Test Plan

Clicked around and created fragments.

Diff Detail

Lint Skipped
Tests Skipped

Event Timeline

Some minor inlines.

How are you planning to version Fragments?

I think requiring N queries to load N parents is a pretty big weakeness. If we store a path instead (like Phriction does) we can load parents in one query. In particular, the visibility of x/y/z/ should depend on the visibility of x/y/, so we must load all parents to load any Fragment. By storing both path and depth, we can query children/grandchildren like Phriction does, too. Does that seem reasonable? Are there reasons parentPHID is better? I'm worried we're going to end up doing thousands of queries to list a page of, e.g., recently updated fragments with this approach.


You should be abel to do this with $header->setPolicyObject($fragment)

53–64 ↗(On Diff #17444)

As below, passing null will fatal.

79 ↗(On Diff #17444)

It's not possible for $old or $new to be null; PHP will raise a typehint warning which we'll convert into an exception when we enter the method. You can add = null in the method signature to allow it, at the cost of making the signature a bit awkward.

88–113 ↗(On Diff #17444)

Is this cleaner as:

if ($old_hash === $new_hash) {
  return null;

if ($old_hash !== self::EMTPY_HASH) {
  $old_content = $old->loadFileData();
} else {
  $old_content = "";

if ($new_hash !== self::EMTPY_HASH) {
  $new_content = $new->loadFileData();
} else {
  $new_content = "";


Fragments themselves can't be moved or renamed, so it seems perfectly fine to permanently store the path and depth in the fragment itself and the load based on that. There's no reasons to use parentPHID as far as I can see.

Patches are the versioning for a fragment; a fragment will always have at least one patch (going from hash 00000... to the hash of the current file). When a fragment is updated, we create a new patch to go from the old file to the new file, and update the fragment's hash and file PHID to point to the new file.

hach-que updated this revision to Unknown Object (????).Dec 6 2013, 5:22 AM

Updated based on feedback

hach-que updated this revision to Unknown Object (????).Dec 6 2013, 5:25 AM

Bug fix

I've retained the word "patch" for now since I don't want to use the word "version" (since that implies some human associated versioning). I'm open to better suggestions on what to call the fragment at each point in time.

Changing "Patch" to "Version"

hach-que updated this revision to Unknown Object (????).Dec 6 2013, 9:42 PM

Renamed patch to version

Some legitimate catches on database keys, and some minor quibbling.


Does giving these an owner really make sense? They seem more like a collective resource (like a repository) than a personal one (like a Paste or Revision).



UNIQUE KEY key_path (path)

This lets us find fragments by path, and ensures we don't have two with the same path.

It is possible (path, depth) or an additional (depth, path) key would also benefit some queries, but that one's murkier to me; we can wait until we have data.


Maybe make this nullable for the initial write. There's no practical consequence, but null is a little cleaner in concept.



UNIQUE KEY `key_phid` (phid)

The key_version should be:

(fragmentPHID, sequence)

...I think? Two reasons on that:

  1. Two versions with <fragmentPHID=X, filePHID=Y, sequence=3> and <fragmentPHID=X, filePHID=Z, sequence=3> are unique on this key, but not valid (they give the fragment two separate entires with sequence = 3).
  2. When ordered with fragmentPHID first, this key can be used to select a fragment's versions. With sequence first, it can't, and we do a table scan.

Equivalent to last($parents)? (Except I don't remember if last() returns false or null; it's probably false for consistency with end()).




Can be simplified slightly with last(), probably slightly more with basename().


It's mainly for policies so that there's always an owner who can recover it.

Maybe it's more suitable to just give administrators implicit access to view and edit fragments, what do you think?


Will fix.


In a later diff I reverse the logic here so that latestVersionPHID can be null on the fragment and the fragment gets saved before the first patch is assigned. Making latestVersionPHID nullable has to be done to support directory fragments, so I can bring that change forward into this diff if required.


Will fix.


Will fix.


It's literally a wrapper around end(), so it will return false on an empty array.

hach-que updated this revision to Unknown Object (????).Dec 7 2013, 1:28 AM

Changes requested in code review

Administrators can recover any object with bin/policy unlock, and ApplicationTransactions generally prevent you (partly in practice, partly in theory) from making edits which lock you out of objects. This is still possible, but not in a single step: you have to, e.g., set the policy to "members of project X", and then leave that project.

So far, these methods seem to be sufficient. If we get more feedback later on that they aren't, we'll have to deal with them globally anyway (maybe "admins can see anything" flag), so I think it's fine to not plan for this case.

hach-que updated this revision to Unknown Object (????).Dec 7 2013, 1:29 AM

Use nonempty()

(And don't worry about shipping changes around across diffs, I don't see much value in it and it's a pain to do.)

hach-que updated this revision to Unknown Object (????).Dec 7 2013, 1:41 AM

More changes