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

On 2/9/2011 9:37 AM, Vincent Ladeuil wrote:
> mergetools.resolve_conflicts should update 'retval' no ?

Yup.

> 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 ?

I did at one stage, I'll see if I can put it back in there.

> 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.

Good point about the config. I'll change that.

> 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.

Done.

> 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 ?

It's the same way that the 'auto' action reports it.

> 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.

The tests for resolve_conflicts use a dummy invoker function which
doesn't actually call out to the subprocess. I can use this to fake retvals.

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

Ok.