Download & Extend

Proper bitmasks for VersioncontrolItem::type

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

Version:5.x-2.x-dev» 6.x-2.x-dev

new tasks on 6.x-2.x

#3

Title:Proper bitmasks» Proper bitmasks for VersioncontrolItem::type
Assigned to:sdboyer» Anonymous

This involves removing VersioncontrolItem::deleted

#4

Issue tags:+git sprint 9

Tagging for Git Sprint 9

#5

Assigned to:Anonymous» Jonathan Webb

#6

Here's a patch for review.

  • Adds VERSIONCONTROL_ITEM_DELETED bitmask
    • Implements hook_update_N() to update values stored in {versioncontrol_item_revisions}.type
    • VERSIONCONTROL_ITEM_{FILE,DIRECTORY}_DELETED retained, based on the new bitmask
  • Alters VersioncontrolItem methods to utilize bitwise operations.
    • Return values changed from TRUE/FALSE to the result of the bitwise comparison op.
    • Re: comment #3 above, VersioncontrolItem::deleted is retained, just uses the new bitmask
  • Updated comments and schema descriptions where applicable
AttachmentSizeStatusTest resultOperations
versioncontrol-319153.patch8.77 KBTest request sentNoneView details

#7

Status:active» needs review

#8

Fixed a minor issue with the output on the update hook.

AttachmentSizeStatusTest resultOperations
versioncontrol-319153-2.patch1.15 KBTest request sentNoneView details

#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 :|

AttachmentSizeStatusTest resultOperations
versioncontrol-319153-3.patch9.09 KBTest request sentNoneView details

#10

Status:needs review» postponed
Issue tags:-git phase 2+git phase 2 leftovers

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

Issue tags:-git sprint 9

removing sprint 9 tag