Code review comment for lp:~spiv/bzr/merge-into-merger

John A Meinel wrote:
> 184 + def _entries_to_incorporate(self):
>
> ^- I wonder if this could be written without tree.inventory.

As it turns out: not really. I'll explain why:

> All the path2id calls can just be replaced with tree.path2id(). The call for
> "if root_id in inventory" can be replaced with "tree.id2path(file_id)" (we may
> need to trap an exception, though, which is a bit ugly.)
>
> This:
> subdir = other_inv[subdir_id]
>
> Gets replaced by:
> subdir = other_tree.iter_entries_by_dir(subdir_id).next()[1]
>
> Which isn't very pretty either, but should do the job.

This all works fine...

What doesn't work is replacing this:

        for ignored_path, entry in other_tree.inventory.iter_entries_by_dir(subdir_id):

Tree.iter_entries_by_dir doesn't have the same interface as
Inventory.iter_entries_by_dir; it lacks the from_dir parameter (which is what is
used at the moment). You can only pass specific_file_ids and yield_parents,
there's no way to iter just a subdirectory with Tree.iter_entries_by_dir.

This could probably be corrected fairly easily (and probably should be), but it
wouldn't matter anyway, because the implementation simply thunks through to
self.inventory.iter_entries_by_dir anyway. So the practical benefit to using
more complex code to avoid other_tree.inventory seems to be zero at the moment.
I don't see see any other methods on tree that can iterate a partial tree
without generating an inventory from the dirstate.

So, I don't think we should make this change right now, especially as you say:

> We don't have to, but it might be nice to get away from accessing the .inventory attribute.

Thanks for making me more familiar with the inner workings of our Tree code,
though ;)

On to Robert comments:

Robert Collins wrote:
[...]
>
> _finish_transform doesn't pair well with compute_transform. Perhaps finish_computing_transform?

Thanks, changed as you suggest.

> The new exception should be in errors.py until we change our policy on that.

We already did.
<http://doc.bazaar.canonical.com/bzr.dev/developers/HACKING.html> says:

“Many errors are defined in bzrlib/errors.py but it’s OK for new errors to be
added near the place where they are used.”

> Rather than _Wrapper I suggest MergeTypeParameteriser. Or something like that. Long but less 'zomg I need to read the code' than Wrapper ;)

Agreed. Changed.

> I think your tests really belong in the main merge test script, merge_into was
> at most a plugin name, and this is core code - its tests should be matching
> the module the code is in as per our convention for finding tests.

Good point. Changed.

> What do you think of making an existing-but-empty dir be replaced rather than conflicted ?

I'm ambivalent. I don't really have a real use case either way, although I can
imagine them. The conservative route (conflict) satisfies the immediate goal
(support taking debian/ dirs from parallel imports into the upstream
branch). If you have a use case or other strong reason then I'm happy to
change from conflict to replace.