Edgewall Software
Modify

Opened 12 years ago

Closed 12 years ago

#5014 closed defect (fixed)

vc api misses the sync() method

Reported by: thomas.moschny@… Owned by: Christian Boos
Priority: normal Milestone: 0.10.4
Component: version control Version: devel
Severity: minor Keywords: cache sync
Cc: Branch:
Release Notes:
API Changes:

Description

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.

Attachments (0)

Change History (13)

comment:1 by Christian Boos, 12 years ago

Keywords: cache sync added
Milestone: 0.110.10.4

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.

comment:2 by Christian Boos, 12 years ago

Resolution: fixed
Status: newclosed

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

comment:3 by Christian Boos, 12 years ago

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

comment:4 by thomas.moschny@…, 12 years ago

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?

comment:5 by Christian Boos, 12 years ago

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.
        """

in reply to:  5 comment:6 by thomas.moschny@…, 12 years ago

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.

comment:7 by Christian Boos, 12 years ago

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

in reply to:  7 comment:8 by thomas.moschny@…, 12 years ago

Resolution: fixed
Status: closedreopened

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

comment:9 by Christian Boos, 12 years ago

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).

in reply to:  9 comment:10 by thomas.moschny@…, 12 years ago

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.

comment:11 by Christian Boos, 12 years ago

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.

in reply to:  11 comment:12 by thomas.moschny@…, 12 years ago

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.

comment:13 by Christian Boos, 12 years ago

Resolution: fixed
Status: reopenedclosed

API fixed in r5151.

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted.
to The owner will be changed from Christian Boos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.