Posted by sdboyer on October 9, 2008 at 6:03pm
| Project: | Version Control API |
| Version: | 6.x-2.x-dev |
| Component: | API module |
| Category: | task |
| Priority: | normal |
| Assigned: | Jonathan Webb |
| Status: | postponed |
| Issue tags: | git phase 2 leftovers |
Issue Summary
So, one thing. I believe I mentioned this before, but there are a few points where I think we could benefit - in terms of code brevity and (some, minor) speed - by switching to proper bitmasks for some of these constants. Quick example:
<?php
// VCS item types.
define('VERSIONCONTROL_ITEM_EMPTY', 0);
define('VERSIONCONTROL_ITEM_FILE', 1);
define('VERSIONCONTROL_ITEM_DIRECTORY', 2);
// "Deleted" item types are only used for items that don't exist in the
// repository (anymore), at least not in the given revision. That is mostly
// the case with items that were deleted by a commit and are returned as result
// by versioncontrol_get_operation_items(). A "deleted file" can also be
// returned by directory listings for CVS, representing "dead files".
define('VERSIONCONTROL_ITEM_FILE_DELETED', 3);
define('VERSIONCONTROL_ITEM_DIRECTORY_DELETED', 4);
?>Could become:
<?php
define('VERSIONCONTROL_ITEM_EMPTY', 0); // equivalent to {0x000}
define('VERSIONCONTROL_ITEM_FILE', 1); // equivalent to {0x001, 1 << 0}
define('VERSIONCONTROL_ITEM_DIRECTORY', 2); // equivalent to {0x002, 1 << 1}
define('VERSIONCONTROL_ITEM_DELETED', 4); // equivalent to {0x004, 1 << 2}
?>I'll need to see where it's actually of real use, but the bottom line is that we can divorce whether or not something's deleted from whether it's a file or directory. It can have both attributes at once - $type |= VERSIONCONTROL_ITEM_FILE | VERSIONCONTROL_ITEM_DELETED - and it can help to granularize some operations. So unless you horribly object, I'll keep an eye out for places where this can be done and do it in stages :)
Comments
#1
Oh hey, why didn't I notice this issue? Anyways, YEAH, you're totally right. The classical example where bitmasks make a lot of sense. No objections at all.
#2
new tasks on 6.x-2.x
#3
This involves removing VersioncontrolItem::deleted
#4
Tagging for Git Sprint 9
#5
#6
Here's a patch for review.
#7
#8
Fixed a minor issue with the output on the update hook.
#9
Realized after the fact that my minor fix was an just incremental patch (not enough caffeine, apparently). Here's the full patch. I've tested this with a couple of Git repos and it seems to work fine. Anyway sorry about the multiposting :|
#10
I love bitmasks, don't get me wrong, but keeping bitmask values in the database can be tricky, as it really significantly hampers conditional clauses - bitmask calculations are non-trivial for dbs. So even if the patch were flawless, and for the sake of argument let's assume it is, I still can't accept it right now because it changes the whole way that filtering on that column would work. In that way, this constitutes a notable API change & refactor to vcapi that I just can't handle at this point.
Thus, marking this postponed. Let's revisit it after we launch on Git. I can say even now, though, that I won't want to commit this until I see lots of tests for it.
#11
removing sprint 9 tag