Code review comment for lp:~doxxx/bzr/mergetools-commands

Argh, it looks like my review get lost :-(

Let's try again...

54 + mergetools.resolve_conflicts(tree.conflicts(), cmdline)
55 + return retval

mergetools.resolve_conflicts should update 'retval' no ?

86 + if resolve_using is not None:
87 + conf = _mod_config.GlobalConfig()
88 + cmdline = conf.find_merge_tool(resolve_using)
89 + if cmdline is None:
90 + raise errors.BzrCommandError(
91 + 'Unrecognized merge tool: %s' % resolve_using)
92 + if not mergetools.check_availability(cmdline):
93 + raise errors.BzrCommandError(
94 + 'External merge tool is not available: %s' %
95 + resolve_using)
96 + mergetools.resolve_conflicts(tree.conflicts(), cmdline)

Same remark, plus this code is screaming: 'put me in a helper !', you have almost the same block above. Or may be that block should just be embedded in resolve_conflicts ?

132 + action = 'tool'

Be bold ! s/tool/mergetools/ :)

141 + conf = config.GlobalConfig()
142 + cmdline = conf.find_merge_tool(using)
143 + if cmdline is None:
144 + raise errors.BzrCommandError(
145 + 'Unrecognized merge tool: %s' % using)
146 + if not mergetools.check_availability(cmdline):
147 + raise errors.BzrCommandError(
148 + 'Merge tool is not available: %s' % using)

Or may be it's really a distinct helper :) Whatever you chose, make sure a wt object is available, you use only GlobalConfig so far, but in the long term this would probably be a branch or wt config, both of which could be accessed via a wt.

161 + if verbose:
162 + ui.ui_factory.show_message('Invoking %s on %s...' %
163 + (merge_tool.get_name(), file))

Hmm, until we re-think the way we resolve conflicts I'd prefer that the feedback is the same for all the different ways to resolve a conflict. This particular message looks more like a trace.mutter() to me.

And since this is the only use of the 'verbose' option, I think we can just remove it.

168 + ui.ui_factory.show_message('%d conflict(s) resolved.' % resolved)
169 + unresolved = tree.conflicts()
170 + if len(unresolved) > 0:
171 + ui.ui_factory.show_message('Remaining conflicts:')
172 + for conflict in unresolved:
173 + ui.ui_factory.show_message(str(conflict))

This seems to depart from the way the conflicts are reported in the other cases for no good reason, can you unify it instead ?

More tests seem to be needed here, for testing the return codes of mergetools.resolve_conflicts with some simplified tool that can simulate resolving a conflict or not.

An then some more blackbox tests to make sure its integration in resolve is correct too.