vc api misses the sync() method

[thomas.moschny@…]

Since r5114, trac/env.py calls Repository.sync(). The versioncontrol api in trac/versioncontrol/api.py however doesn't mention this method. It should document that call and provide a default implementation that raises a NotImplementedError.

[cboos]

Ah, I knew there was a good reason to put this sync call in the CachedRepository constructor, initially. But it's better like it is now, as it is convenient in a number of cases to get the repository without trying to synchronize first.

So yes, I'll add the Repository.sync method, but I think a default implementation of pass is better so that vc backend that don't use the cache will not have to bother.

[cboos]

Fixed in r5125 (trunk) and r5126 (0.10-stable).

[cboos]

See also r5138 and r5139 (adds a feedback optional argument to the sync method).

[thomas.moschny@…]

The new docstring in source:trunk/trac/versioncontrol/api.py​ isn't that clear. What is the purpose of the callback? And, I think it describes the situation from the wrong perspective. "If specified, feedback is a callback, taking a rev parameter, that should be called ...", not?

But even then, it's not clear to me. The repository backend is called via sync(), and a feedback callback is given. Fine. Now, under what circumstances (or, more precisely, for which revisions) should the vc backend call that callback?

[cboos]

Well, the perspective is the one of the users of the API, not the one of the implementer of a subclass. I can try to give both perspectives.

Revised doc:

    def sync(self, feedback=None):
        """Perform a sync of the repository cache, if relevant.
        
        If given, `feedback` must be a callback taking a `rev` parameter.
        The backend will call this function for each `rev` it decided to
        synchronize, once the synchronization changes are committed to the 
        cache.
        """

[thomas.moschny@…]

Replying to cboos:

Well, the perspective is the one of the users of the API, not the one of the implementer of a subclass.

Oh, sure. My bad.

Revised doc:

This is clearer now.

[cboos]

Ok, doc updated in r5142:5143. Thanks for the feedback!

[thomas.moschny@…]

Shouldn't we have default implementations of pass for Repository.sync() and Repository.sync_changeset() (keeping the api changes minimal...)?

[cboos]

That's the case for sync() (well, the pass itself is actually missing, but since there's a docstring...) and I added a raise NotImplemented exception for
sync_changeset() (see comment:ticket:1892:11).

[thomas.moschny@…]

Replying to cboos:
Hm, I was even thinking about 0.11dev. Having a base class method without any implementation (not even raising a NotImplemented exception) doesn't seem to be good style.

[cboos]

Woops, I've just been hit by the #4100 issue, and no, I was using Firefox 2 and t.e.o is not using https ;-) I'll follow-up there.

Ok, let's write my comment again (making it shorter ;-) )

Sure, I'll add a "pass" for 0.11 and the NotImplemented for sync_changeset, and the s/feedback/rev_callback/ suggested by Matt, any other suggestion welcomed.

Note that the versioncontrol API will be revised a lot for 0.12 anyway, especially w.r.t. to the sync, which should become transparent to the plugin writer.

[thomas.moschny@…]

Replying to cboos:

Sure, I'll add a "pass" for 0.11 and the NotImplemented for sync_changeset, and the s/feedback/rev_callback/ suggested by Matt, any other suggestion welcomed.

Fine.

Note that the versioncontrol API will be revised a lot for 0.12 anyway, especially w.r.t. to the sync, which should become transparent to the plugin writer.

The main place to collect thoughts about that is VcRefactoring, I guess? It seems to me that the plan is to make all the caching transparent to the backend plugin. Note that for that to work, we'll have to support non-linear history graphs, which the current table(s) used for caching do not.

[cboos]

API fixed in r5151.

Yes, VcRefactoring contains some ideas that are still valid for 0.12, I think.