[MERGE] working tree stat cache updating during commit.

[Robert Collins]

On Tue, 2008-09-23 at 12:08 -0400, John Arbash Meinel wrote:
> John Arbash Meinel has voted tweak.
> Status is now: Conditionally approved
> Comment:
> I can understand the desire to make "_observed_sha" a private function. 
> However, as it is being used outside of WT.XXX I think we have to make 
> it public.

I disagree here - I don't think this is ready for use by things outside
of bzrlib. It may have performance implications too if it were called a
lot. Also, Commit is part of the WT API, so its not in any way
inappropriate for Commit to be calling an internal method.

> We've done this in the past, with "Repository._make_parents_provider". 
> In that case Repository.get_graph(other_repo) assumes that "other_repo" 
> has _make_parents_provider.

Sure, _make_parents_provider is part of the Repository implementation
contract; that doesn't mean it is-or-is-not for public consumption.

_ is too small a bitfield.

> Also, I know you added a no-op version, but I think we would be 
> well-served in doing double indirection for new functionality on WT, 
> Branch, and Repository. So you would have:
> 
> class WorkingTree:
>    def observed_sha(self, ...):
>      return self._do_observed_sha()
> 
>    def _do_observed_sha(self, ...):
>      # The default implementation is a no-op
>      pass
> 
> I'm certainly not strict on this, but I think it would help us maintain 
> compatibility in our WT/Branch/Repository apis, because we expect 3-rd 
> parties (like bzr-svn) to implement portions of this. "observed_sha" 
> isn't terribly likely, but I think it would be a good habit to practice.

This pattern is called Template Method, and its useful when you have
pre-or-post work that you want, so the subclass 'fills out' the
template. Well, we _might_ in the future have that, but frankly our code
would be fugly if we made every subclassable method always look like
that.
Further to that, while function calls cost what they do there are non
trivial consequences to making our code look like that - and with no
benefits [today] why pay that cost? I didn't want to pay for the the
indirection I already had to put in, let alone a do-nothing that purely
hands off to an actual do-the-job-function. 

> I'll also note that your change to Builder.record_entry_contents is also 
> not a backwards compatible change, and needs at least a NEWS entry.

I'll add one.

>  I'm pretty sure that bzr-svn *does* provide an implementation of Builder, so 
> we'll need to give Jelmer a heads up that it is changing.
> (Doing the 'do_record_entry_contents()' thing would let us update the 
> record_entry_contents api, and plug in 'None' for the missing file_sha 
> without requiring Jelmer to update his plugin.)

his commit builder currently returns nothing at all, its not compatible
with bzr's commit code as it stands, so this change is neither here nor
there. I don't know what you mean by 'do_record_entry_contents() thing'
here. [I'm guessing you mean rename the function and leave an older one
behind that strips off the extra return value.] I can do that if you
want - I have no strong feelings here, but AFAIK no one has
reimplemented bzrlib.commit, which is the user of this code path. So no
one should be adversely affected by this change.

> Otherwise, I'm *very* happy to see this change, and would love to see it 
> merged.

Thanks for the review, as I've disagreed with some of the tweaks, I'll
add a NEWS entry and put up for resubmit and further discussion.

-Rob
-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20080924/28037e36/attachment.pgp