Merge lp:~ian-clatworthy/bzr/eol-update-bug into lp:bzr/2.0

This patch fixes content filtering so that it works after most operations, not just after the initial branch/checkout. There were 2 code paths in transform.py that needed enhancing to check for content filters: one used by 'merging' and one used by 'reverting'. The merging code path is used by lots of operations including merge, pull, update and switch.

I've added several 'blackbox-style' tests as part of this patch. It probably wouldn't hurt to add some more for other operations, e.g. shelve/unshelve, though I'm optimistic that the 2 code paths above will cover 99% of them.

In any case, I'm submitting this for review now because:

1. Without these code changes, content filtering is effectively broken for most users.

2. The 2.0 code freeze is really close and I want to maximise the time available for review of what's fixed/tested already.

3. Those tests could come in a latter patch (after there's agreement on whether blackbox-style tests are acceptable here or not).

BTW, the bug report also mentions that after committing a tree with keywords expanded, the keywords aren't re-expanded after commit. I'm going to work on that issue now, either by adding a post-commit hook to the keywords plugin or by changing commit. The latter approach implies always paying a performance cost whether it's required or not so I'll try the post-commit approach first. Either way, I feel it's best to keep that issue out of scope of this patch.

>> BTW, the bug report also mentions that after committing a tree with keywords expanded, the keywords aren't re-expanded after commit.

Commit does not write to the users tree : a post commit hook isn't appropriate [for starters, other post commit hooks could interfere/see convenience forms themselves etc]. I suggest looking closely for the cause/some possible confusion is going on here.

I'm trying to channel Aaron here, and I don't know the transform code *all that well*. However, what you've done looks wrong to me.

Specifically, you look up a path in one tree, and use that to select filters in a different tree (this is at the very top of your patch).

This has the problem that when a containing directory is renamed in this tree, it will be matching on a different path than actually will be used.

Secondly, as filters are defined as being path based, *every* change to a transform that can cause a change to the path of something that can be filtered needs to recalculate the filter stack and possibly reapply it, for all those somethings. E.g. renaming a directory foo/baz to bar/baz, and foo was filtered and bar isn't.

I think the change to the API to pass in a path that is able to be wrong reduces clarity and shouldn't be done. We can probably get by (with a critical bug to be fixed asap after the release) with out updating filters appropriately.

Robert Collins wrote:
> Review: Needs Fixing

Thanks for the feedback. poolie is taking over this patch from here so I
can focus on the Windows installer. I'll let him look deeper but some
quick comments on your first point ...

> Specifically, you look up a path in one tree, and use that to select filters in a different tree (this is at the very top of your patch).

The patch mightn't be quite right. I can say however that this cross
tree stuff is tricky. Keep in mind that:

* in the case of reverting a removed file, its path doesn't exist
  in the working tree so the only place to find that is the rev-tree

* rules aren't stored historically so, IIRC, revtrees don't know
  what they are - only the WT knows that.

So looking up a path in the revtree and using the filters from the WT
may indeed be correct, in the case of revert at least.

Merging has similar challenges. A new path may only exist in the OTHER
tree say but the filters applied need to come from the working tree
being merged into.

Ian C.

Robert Collins wrote:
>>> BTW, the bug report also mentions that after committing a tree with keywords expanded, the keywords aren't re-expanded after commit.
>
> Commit does not write to the users tree : a post commit hook isn't appropriate [for starters, other post commit hooks could interfere/see convenience forms themselves etc]. I suggest looking closely for the cause/some possible confusion is going on here.

Plugins like keywords do need to update the working tree, if any, after
a commit so that files just committed get keywords values updated. As
the keywords plugin is currently designed, the only files needing
potential update are the newly committed ones.

I agree a post commit hook isn't the answer. I've put a separate patch
up for a finish-commit hook on mutable trees instead.

Ian C.

On Thu, 2009-09-03 at 00:57 +0000, Ian Clatworthy wrote:
>
> > Commit does not write to the users tree : a post commit hook isn't
> appropriate [for starters, other post commit hooks could interfere/see
> convenience forms themselves etc]. I suggest looking closely for the
> cause/some possible confusion is going on here.
>
> Plugins like keywords do need to update the working tree, if any,
> after
> a commit so that files just committed get keywords values updated. As
> the keywords plugin is currently designed, the only files needing
> potential update are the newly committed ones.
>
> I agree a post commit hook isn't the answer. I've put a separate patch
> up for a finish-commit hook on mutable trees instead.

Oh, that use case wasn't clear - I didn't get what was needed.

Branch.post_commit definitely isn't the right thing as its on branch.

There may be room for MutableTree.post_commit too - the difference being
whether or not <activity> should occur before the basis revision id is
changed.

So, the question to ask is 'should keywords update the files *after* the
basis changes, or *before*. I think the answer is *after*, but you've
spent more time staring at this problem: its up to you.

I do suggest that the hook name try to reflect where in the process its
happening, and if its after commit has done its stuff, its really just a
post commit hook on a different object, so lets use the namespaces and
call it MT.post_commit

-Rob

On Thu, 2009-09-03 at 00:54 +0000, Ian Clatworthy wrote:
>
> The patch mightn't be quite right. I can say however that this cross
> tree stuff is tricky. Keep in mind that:
>
> * in the case of reverting a removed file, its path doesn't exist
> in the working tree so the only place to find that is the rev-tree

It has a path that it will end up with in the output tree; thats the one
that should be used, right? Paths in other trees are a poor proxy for
that.

> So looking up a path in the revtree and using the filters from the WT
> may indeed be correct, in the case of revert at least.

I argue that it would only be accurate in the most trivial of cases;
solving the primary problem - that a transform may need to reapply
filters some N times - will solve the other cases as well.

> Merging has similar challenges. A new path may only exist in the OTHER
> tree say but the filters applied need to come from the working tree
> being merged into.

Right, its exactly what I'm driving at.

-Rob

I don't think this is safe for 2.0 but it might be ok for 2.1b1

280 +def create_from_tree(tt, trans_id, tree, file_id, bytes=None,
281 + filter_tree_path=None):
282 + """Create new file contents according to tree contents.
283 +
284 + :param filter_tree_path: the tree path to use to lookup
285 + content filters to apply to the bytes output in the working tree.
286 + This only applies if the working tree supports content filtering.
287 + """

I don't understand how that new parameter works:

Is it a single file? How can that work with tt working across a whole tree?

> I don't think this is safe for 2.0 but it might be ok for 2.1b1
>
> 280 +def create_from_tree(tt, trans_id, tree, file_id, bytes=None,
> 281 + filter_tree_path=None):
> 282 + """Create new file contents according to tree contents.
> 283 +
> 284 + :param filter_tree_path: the tree path to use to lookup
> 285 + content filters to apply to the bytes output in the working tree.
> 286 + This only applies if the working tree supports content filtering.
> 287 + """
>
> I don't understand how that new parameter works:
>
> Is it a single file? How can that work with tt working across a whole tree?

This API is called per file-id, it's not called per tree.