Code review comment for lp:~ian-clatworthy/bzr/eol-update-bug

Revision history for this message
Robert Collins (lifeless) wrote :

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.

review: Needs Fixing

« Back to merge proposal