Revisit syncEvent() logic on main repository sync plugin

tagging

fixing title

basic smoke testing indicates this mostly working in the git6 environment. woohoo! commits make it into the db, are properly associated with users, and entries are made in the versioncontrol_event_log and versioncontrol_git_event_data tables. and they're done not just for normal pushes, but for tag & branch creation/deletion, even with no new commits pushed. we also seem to handle the case of tag reassignment fairly elegantly. excellent.

couple problems - we're still inserting the full array of commit hashes into the 'commits' text field, and not concatenating it to 5 when it's more than that, as we'd discussed. we're also still inserting ALL the hashes when a new branch is pushed...which is the biggest and riskiest case. like, i just saw 700 hashes show up when i pushed a new branch to versioncontrol. the approach there should be that we insert none of the hashes at all, leaving that array empty. we may want to entertain doing a bit of custom logic in order to determine the branch from which the new branch most likely descends, but that's quite optional and can be done later.

we also need to add a {versioncontrol_git_event_data}.commit_count field that records the total number of commits pushed to any given ref. yes, that's derivable info, but it's also totally nonvolatile and thus worth 'caching' it as the lookup is nontrivial to do during a normal request.

what tests did you run that indicated this is slower than syncFull()? and do you have any intuitions about why?

sounds good. i'll have a look at these changes as soon as i can, probably thursday.

great. i'm gonna dig in on this tonight, too, see what we can get done before scrum tomorrow.

Formatting pending list and updating it to reflect last comment.

ok, with those latest changes pulled, the token tests are now passing, great. now i've just got four failures on the git event data integrity. will investigate those.

i've fixed a number of problems. the first and most glaring one was that our tests, when simulating pushes, did not actually reflect what those pushes would/should have done in the repository. as a result, our history synchronizer could get quite confused, because it would receive a bunch of information about the state of the repository that was inconsistent with the data it directly fetched from the repository during the course of normal operation.

from there, some new problems emerged, primarily that the handling of branch deletions was kinda entirely broken. the primary issue is that we don't do the garbage collection we should - we don't clean out old commits that are no longer associated with any branch. Technically, this should be easy - it should just be the difference between:

      elseif ($ref->eventDeletedMe() && empty($label_repo) && ($label = $label_db)) {
        $label->delete();
      }

and

      elseif ($ref->eventDeletedMe() && empty($label_repo) && ($label = $label_db)) {
        $label->delete(array('nested' => TRUE));
      }

because we included support for this exact sort of thing in the base API itself. however, this is one of those places where i don't expect the base API to scale very well, as its approach to performing this delete is to load ALL of the commits on a branch, then iterate over them and check to see if they're only attached to the branch that's being deleted. that's an unacceptably large number, and i fully expect memory exhaustion to occur as a result. so basically, that's another todo for us, and i've created an issue - #1642310: Optimize logic for branch deletion with nested option.

from there, we still had the problem that the subsequent branch/commits reconciliation logic simply didn't check to see if the branch being worked on had been deleted (if it had, it could be skipped). however, that led me to another problem - VersioncontrolGitRepositoryHistorySynchronizerDefault::getCommitInterval() is wrong:

  protected function getCommitInterval($start, $end) {
    if ($start == GIT_NULL_REV) {
      $range = $end;
    }
    elseif ($end == GIT_NULL_REV) {
      $range = "$start..";
    }
    else {
      $range = "$start..$end";
    }

    $command = "rev-list --reverse $range";

the idea here is basically to be a passthru to git rev-list , and if the null rev is passed for either start or end, it's simply dropped. dropping it is the problem, though - without the second argument, the .. notation assumes HEAD. which means the range being reported is the difference between the first SHA and whatever HEAD happens to be. which is nothing like what we're looking for there. so, i've switched out that second if statement with a new Exception i commushed to vcapi - VersioncontrolSynchronizationException. i'd like us to start using this exception - a lot - in order to make it clearer when and where our failures are occurring during synchronization. i've also opened an issue for that - #1642326: [meta] Improve repo locking & behavior on synchronization failures.

with that cleared up, we still have another potential memory exhaustion point here, for much the same reason that there was an issue with the deletion logic: a new branch being passed through that commit interval logic will get back ALL commits, all the way back to root. for core, loading all of those up only to discover that a scant few out of tens of thousands are actually new, is *not* an acceptable approach. so i'm finishing off a commit that introduces a heuristic trick with git merge-base. instead of going all the way back to the root, we merge-base against HEAD in order to determine a shared commit that is likely to be a good shared base, then generate the interval against that. we could pick any branch, but in mos repositories, the branch set as HEAD will be the most likely one from which branches are created, and so is likely to have the smallest interval. we could engage in more inspection to definitively determine which branch is closest, but it's not worth delaying this patch for that logic.

there are two cases i can think of in which this won't entirely work:

• if the push also updated the bare repo's HEAD, and the new branch diverges from HEAD at one of those new commits being pushed up, then we'll miss a couple commits in that sequence. however, this shouldn't be an issue (at least in the immediate term) because the later passthru which processes the HEAD ref will catch those missing commits. it may, however, become an issue later when we attempt to do weighting, as processing those commits out of sequence could be problematic.
• since we are so shitty about letting people set HEAD on d.o, it's quite possible that HEAD may not exist. this is eminently fixable, but it does need resolution, which we should do in a follow-up. it'd be resolved by adding the more complex nearest-parent logic.

ok, ran out of patience on this, time for sleep. there are still several failing tests, and i *cannot* figure out why. it's seeming like it's refusing to insert new branches into the db that really should be there.

we're pretty close...though i am again reminded just how not an exhaustive job we have done of planning out all the various situations this code path has to handle. it's visible in the spaghetti in that code, as well as in the lack of meta-level structure & organization in our tests.

it makes me very uncomfortable.

...and that approach creates a new problem, because we end up not creating the commit<->branch mapping in {versioncontrol_operation_labels}. ugh. it's also sticky because the merge-base i added returns something non-useful if the tip of the new branch == HEAD, or any ancestor of HEAD.

so, i'm still tinkering. i really want to separate & fully comment this logic, though, as it's very difficult to follow everything that's going on.

OK, i removed the merge-base logic and just let it go as-is. after some more fixes to the tests, everything is passing.

which does not mean all is well. we still have potential memory-busting problems in large repositories. and, of course, the longstanding problem of a swelling {versioncontrol_operation_labels} table.

i can't wait until we can switch to some system that's more reliant on fetching data directly from the repository at runtime.

the token tests pass, at least for me, so that's close enough to fixed for me. i've merged the branch back in, and am now working on the backport to 6.x.

oh, a note - i was meaning to try an array_chunk() approach to cutting down the number of commit objects we load simultaneously in order to decrease the likelihood of the memory-busting problem. if it works like it should, it could be a good enough solution that we don't really need to look at anything else. basically, we just array_chunk() the results of the rev-list that comes back from getCommitInterval(), then wrap the remaining logic in a foreach over the chunks. i think everything inside there is isolated from needing overall state, so the chunking should be transparent.

yep, i skipped that commit, and a couple others.

6.x-1.x backport is looking pretty good, though we're getting some weird errors on the $event->ref->refname being set to something bizarre and too long. tough to tell just yet what's causing it, as the code is really pretty nearly identical between 6 and 7.

whoops, xpost

About {versioncontrol_operation_labels}, yes, that's definitely a bottleneck, but I do not see a change for a direct repository access near the future of the module. Anyway why we would like a VCS abstraction? Mayeb it worths to remember that we are now optimizing the sync's, but that's not so usual as the read's, which I guess is our main target.

there are already 2 million rows in that table on live d.o, and it's just gonna get worse. a lot worse. it is currently the primary db-level weak link - at read or write time. write because index generation gets nasty, and read for the obvious reasons.

on a separate note, though, i've finished the backport (now that we've sorted out yet more weird issues with testing), pushed all of the changes up, and am now testing on git6. woot woot.

Forgot to add a space.

bit of a note, i guess, not sure where else to put it...i've made sync-tinkering branches in both vc_git and vcapi. there are just a *massive* number of additional improvements in there. they're quite near finished, but more importantly, are not the sort of changes that would block us up anyway. after a little more work (~30 minutes) to VersioncontrolRepository::syncEvent(), we'll be ready to test on git6 again, and i expect it'll go more smoothly than it did before (these changes solve one of the problems i was having on git6).

all these changes will need-forward-porting to d7, but it should be trivial cherry-picking.