Merge lp:~doxxx/bzr/mergetools-commands into lp:bzr

This is far clearer.

Making --detect or --list mandatory sounds weird though. --list should be default but doesn't it duplicate 'bzr config bzr.mergetools' ?

Or rather 'bzr config *mergetool*' to also get default_mergetool.

And then... why not use bzr.mergetools.default and forbid 'default' as a valid merge tool name in your other proposal ?

It would be nice to mention that --detect *also* set the mergetools in the the config (which one ?).

Which one is the default in that case ? None ?

If that's the case, this means the user must specify which merge tool he want to use no ?

Since you're providing access to pre-defined merge tools as soon as you have detected them, why not go a step further and make this '--detect' step optional and provides the default command-line for, say kdiff3, if the user try to use --resolve-using kdiff3 ?

On 11/25/2010 5:19 AM, Vincent Ladeuil wrote:
> Making --detect or --list mandatory sounds weird though. --list
> should be default but doesn't it duplicate 'bzr config
> bzr.mergetools' ?
> Or rather 'bzr config *mergetool*' to also get default_mergetool.

Making --list the default operation sounds like a good idea. Yes, this
is duplicating functionality from `bzr config`. Would you rather have a
single command called 'detect-merge-tools'? I think it's nice to have a
pretty-printed list of merge tools.

> And then... why not use bzr.mergetools.default and forbid 'default'
> as a valid merge tool name in your other proposal ?

I suppose I could. :)

> It would be nice to mention that --detect *also* set the mergetools
> in the the config (which one ?).
>
> Which one is the default in that case ? None ?

None, it doesn't set a default since the command-line doesn't need a
default, you always have to specify a name when you want to use a merge
tool with merge or remerge. The default only applies to qbzr and there
you have qconfig to set the default.

> If that's the case, this means the user must specify which merge tool
> he want to use no ?

Correct.

> Since you're providing access to pre-defined merge tools as soon as
> you have detected them, why not go a step further and make this
> '--detect' step optional and provides the default command-line for,
> say kdiff3, if the user try to use --resolve-using kdiff3 ?

Yes, I could try that. With an appropriate message to the effect of
"auto-detecting kdiff3 on PATH"? Or is that unnecessary?

>>>>> Gordon Tyler <email address hidden> writes:

    > On 11/25/2010 5:19 AM, Vincent Ladeuil wrote:
    >> Making --detect or --list mandatory sounds weird though. --list
    >> should be default but doesn't it duplicate 'bzr config
    >> bzr.mergetools' ?
    >> Or rather 'bzr config *mergetool*' to also get default_mergetool.

    > Making --list the default operation sounds like a good idea. Yes, this
    > is duplicating functionality from `bzr config`. Would you rather have a
    > single command called 'detect-merge-tools'?

May be, if we still need it.

Keep in mind that the results it produces will stick. So the resulting
config will be wrong in the following cases:

- when a new merge tool is installed by the user,
- when an existing merged tool is updated or uninstalled by the user,
- when a new bzr version provide new defaults for a given merge tool.

    > I think it's nice to have a pretty-printed list of merge tools.

But apart from shortening the merge tool option name, what is pretty
here ?

Morevoer it doesn't display *where* the command line is defined if the
user put them in locations.conf or branch.conf (again, getting away from
a core implementation means more work ;).

I think the features are good to have internally, but I'm not sure we
need to expose them at the command line level.

    >> And then... why not use bzr.mergetools.default and forbid 'default'
    >> as a valid merge tool name in your other proposal ?

    > I suppose I could. :)

    >> It would be nice to mention that --detect *also* set the mergetools
    >> in the the config (which one ?).
    >>
    >> Which one is the default in that case ? None ?

    > None, it doesn't set a default since the command-line doesn't need a
    > default, you always have to specify a name when you want to use a merge
    > tool with merge or remerge.

Too bad no ?

    > The default only applies to qbzr and there you have qconfig to set
    > the default.

plugins should be able to override the defaults provided by bzr, but if
a user speficy it in some ways, it would be nice to respect that too
(core implementation again ;).

    >> If that's the case, this means the user must specify which merge tool
    >> he want to use no ?

    > Correct.

    >> Since you're providing access to pre-defined merge tools as soon as
    >> you have detected them, why not go a step further and make this
    >> '--detect' step optional and provides the default command-line for,
    >> say kdiff3, if the user try to use --resolve-using kdiff3 ?

    > Yes, I could try that. With an appropriate message to the effect of
    > "auto-detecting kdiff3 on PATH"? Or is that unnecessary?

I don't think it's necessary, a mutter() (i.e. in .bzr.log) may be enough.

On Thu, November 25, 2010 10:31 am, Vincent Ladeuil wrote:
> > Making --list the default operation sounds like a good idea. Yes,
> this
> > is duplicating functionality from `bzr config`. Would you rather
> have a
> > single command called 'detect-merge-tools'?
>
> May be, if we still need it.
>
> Keep in mind that the results it produces will stick. So the resulting
> config will be wrong in the following cases:
>
> - when a new merge tool is installed by the user,
> - when an existing merged tool is updated or uninstalled by the user,
> - when a new bzr version provide new defaults for a given merge tool.

How will it be wrong? The detection process doesn't overwrite existing
merge tools. It only adds merge tools that it detects for which there
isn't already a same-named merge tool in the config.

> > I think it's nice to have a pretty-printed list of merge tools.
>
> But apart from shortening the merge tool option name, what is pretty
> here ?
>
> Morevoer it doesn't display *where* the command line is defined if the
> user put them in locations.conf or branch.conf (again, getting away from
> a core implementation means more work ;).
>
> I think the features are good to have internally, but I'm not sure we
> need to expose them at the command line level.

However, removing the mergetools command entirely and trying to
auto-detect when they use --resolve-using=kdiff3 leaves me wondering how
are users going to know that bzr already can use kdiff3/meld/etc.? I
suppose we could list the known merge tools in the merge and remerge help,
or point them to the configuration help topic where we also list the known
merge tools and the commandlines that we use for them.

This also may look a little odd/ugly in the GUI since it will have to list
all the predefined known merge tools that can be found on the PATH in
addition to the user-defined merge tools and they may have defined their
own kdiff3/meld/etc. merge tool.

I just think it's nice to have a bzr command that helps the user define
the merge tools instead of just throwing them in the deep end (aka config
file).

> > None, it doesn't set a default since the command-line doesn't need a
> > default, you always have to specify a name when you want to use a
> merge
> > tool with merge or remerge.
>
> Too bad no ?

It's very awkward, if not impossible, to have a --resolve-using option
which has an optional value to indicate the tool to use. I suppose there
could be a --resolve-using-default option or treat --resolve-using=default
as referring to the default merge tool and not a merge tool called
'default'.

> > The default only applies to qbzr and there you have qconfig to set
> > the default.
>
> plugins should be able to override the defaults provided by bzr, but if
> a user speficy it in some ways, it would be nice to respect that too
> (core implementation again ;).

I don't understand...

> I think I've addressed all of the comments so far. Please have another look
> and let me know if I've missed anything.

Well, for a start you missed the ones I proposed earlier in
lp:~vila/bzr/mergetools :-D

The missing vertical spaces, the config options renaming and the
use of overrideAttr and they can't be re-merged now.

I think poolie mentioned early in these reviews that we try to
avoid the "smart" operators (__eq__, __ne__), see
doc/developers/code-style.txt 'Imitating standard objects'. The
rationale is it's often more work to maintain these methods than
having explicit comparison operators (they make the code clearer
and easier to maintain).

The arg splitting functions are not necessary, the MergeTool
object receives the commandline when built. It should just
preserve it as-is, no need to rebuild it (splitting and
un-spliting just add possible failure points). This would also
get rid of the three commandline accessors.

There is also a failing test:
ERROR: bzrlib.tests.test_osutils.TestFindExecutableInPath.test_other
----------------------------------------------------------------------
_StringException: Text attachment: log
------------

------------
Text attachment: traceback
------------
Traceback (most recent call last):
  File "/home/vila/lib/python/testtools/runtest.py", line 144, in _run_user
    return fn(*args)
  File "/home/vila/lib/python/testtools/testcase.py", line 487, in _run_test_method
    testMethod()
  File "/home/vila/src/bzr/reviews/mergetools/bzrlib/tests/test_osutils.py", line 2133, in test_other
    self.assertTrue(osutils.is_executable_on_path('sh') is not None)
AttributeError: 'module' object has no attribute 'is_executable_on_path'
------------

But this goes in the right direction nevertheless :)

So, I went ahead fixing the style issues, renaming the options,
making the command line splitting internal and deleting the
associated accessors (and tests). This means making the merge
tool 'name' mandatory which also meant making _KNOWN_MERGE_TOOLS
a dict, until it becomes a proper registry (you may want to fix
the module docstring until it really is a registry).

There are still some weird things around find_executable_on_path,
PATHEXT is windows specific but seems to be applied
unconditionnally. Also, I think windows search in the current
directory if PATH is empty (which we may not want to mimick but
is worth documenting too).

In is_available, you check only for the existence of the file and
not whether the x bit is is set. There is a grey area there that
deserves at least a FIXME comment or better, a fix with proper
multi-platform tests (I won't block on that as long as it's
documented (i.e. a FIXME will do)).

I still don't agree with set_merge_tools, it will hardwire
_KNOWN_MERGE_TOOLS in whatever config file it is used against,
defeating the purpose of having default values configured (and
kept up to date) by bzr itself.

I think the core feature here is to check the availability of a
merge tool, but this needs to be done when bzr is about to need
it, doing it ahead of time is wrong as things could change. A
tool that you registered may have disapeared and a tool that you
ignored may have appeared...

On 12/6/2010 10:31 AM, Vincent Ladeuil wrote:
> The missing vertical spaces, the config options renaming and the
> use of overrideAttr and they can't be re-merged now.

Woops, I thought the 'bzr.' prefix was some weird notation that you were
just using for the purposes of discussion. :)

And somehow I missed what you said about overrideAttr. That's cool...

> I think poolie mentioned early in these reviews that we try to
> avoid the "smart" operators (__eq__, __ne__), see
> doc/developers/code-style.txt 'Imitating standard objects'. The
> rationale is it's often more work to maintain these methods than
> having explicit comparison operators (they make the code clearer
> and easier to maintain).

I must have missed that. I jsut had a look through the code and I don't
think I'm using them anymore. I needed them for sorting MergeTools in
lexical order for comparing lists during tests, I think. But the tests
don't do that anymore.

> The arg splitting functions are not necessary, the MergeTool
> object receives the commandline when built. It should just
> preserve it as-is, no need to rebuild it (splitting and
> un-spliting just add possible failure points). This would also
> get rid of the three commandline accessors.

Since I'm using string.format to do filename substitution, you're right
that this can be done with a single string commandline form.

> There is also a failing test:
> ERROR: bzrlib.tests.test_osutils.TestFindExecutableInPath.test_other
> File "/home/vila/src/bzr/reviews/mergetools/bzrlib/tests/test_osutils.py", line 2133, in test_other
> self.assertTrue(osutils.is_executable_on_path('sh') is not None)
> AttributeError: 'module' object has no attribute 'is_executable_on_path'

My bad, missed a reference when I renamed the function.

> So, I went ahead fixing the style issues, renaming the options,
> making the command line splitting internal and deleting the
> associated accessors (and tests). This means making the merge
> tool 'name' mandatory which also meant making _KNOWN_MERGE_TOOLS
> a dict, until it becomes a proper registry (you may want to fix
> the module docstring until it really is a registry).

Ok.

> There are still some weird things around find_executable_on_path,
> PATHEXT is windows specific but seems to be applied
> unconditionnally. Also, I think windows search in the current
> directory if PATH is empty (which we may not want to mimick but
> is worth documenting too).

PATHEXT is applied if it exists. Since it won't exist on non-win32, that
shouldn't be a problem, but for safety sake I suppose we could add a
platform check.

> In is_available, you check only for the existence of the file and
> not whether the x bit is is set. There is a grey area there that
> deserves at least a FIXME comment or better, a fix with proper
> multi-platform tests (I won't block on that as long as it's
> documented (i.e. a FIXME will do)).

Right. I should use the same os.access call that find_executable_on_path
uses.

> I still don't agree with set_merge_tools, it will hardwire
> _KNOWN_MERGE_TOOLS in whatever config file it is used against,
> defeating the purpose of having default values configured (and
> kept up to date)...

On 12/6/2010 7:22 PM, Gordon Tyler wrote:
> Since I'm using string.format to do filename substitution, you're right
> that this can be done with a single string commandline form.

Although, the way you're doing it in your branch is fine too. So I don't
think I'll change that.

>> In is_available, you check only for the existence of the file and
>> not whether the x bit is is set. There is a grey area there that
>> deserves at least a FIXME comment or better, a fix with proper
>> multi-platform tests (I won't block on that as long as it's
>> documented (i.e. a FIXME will do)).
>
> Right. I should use the same os.access call that find_executable_on_path
> uses.

This actually turned out to be more complicated than I thought since
os.access(path, os.X_OK) returns True for all extant files on win32. But
I believe I have it solved now.

I'm a bit lost here... in which mp are you handling this is_available related code ?

On 12/7/2010 6:44 AM, Vincent Ladeuil wrote:
> I'm a bit lost here... in which mp are you handling this is_available related code ?

This mp is purely for code changes directly related to modification of
bzr builtin commands like merge and remerge. Anything to do with base
mergetools module functionality like is_available is done in the other
mp, https://code.launchpad.net/~doxxx/bzr/mergetools/+merge/40923.

I think this is ready for review, now that the prereq has been approved.

Could I get some eyeballs on this? It makes the facilities added by pre-req branch actually usable on the command-line.

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.

Thanks for the review. I'll go over it soon.

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.

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

Sorry for being imprecise.

'auto' lists the conflicts resolved, I don't think you do.
The other cases mentions the number of remaining conflicts, I think you don't.

I'd like the 'auto' case to be fixed too but I haven't yet found the right balance there.

On 2/25/2011 7:35 AM, Vincent Ladeuil wrote:
>>> 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.
>
> Sorry for being imprecise.
>
> 'auto' lists the conflicts resolved, I don't think you do.

I do.

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

> The other cases mentions the number of remaining conflicts, I think you don't.

If you would prefer just the number of remaining conflicts, I can do
that instead.

> I do.
>
> + ui.ui_factory.show_message('%d conflict(s) resolved.' %
> resolved)
> + unresolved = tree.conflicts()
> + if len(unresolved) > 0:
> + ui.ui_factory.show_message('Remaining conflicts:')
> + for conflict in unresolved:
> + ui.ui_factory.show_message(str(conflict))
>

*blink*, I missed that, sorry.

Still, auto does:

                    for conflict in un_resolved:
                        trace.note(conflict)
                    return 1
                else:
                    trace.note('All conflicts resolved.')
                    return 0

So if you want to do the same, I'll be fine, but let's do exactly the same then, use trace.note() and return some corresponding value.

> > The other cases mentions the number of remaining conflicts, I think you
> don't.
>
> If you would prefer just the number of remaining conflicts, I can do
> that instead.

That's also an option, I think the idea with mentioning only the number is that it would satisfy users who prefer less verbose output and can still see the conflicts with 'bzr conflicts -v'.

In any case, addressing such unification can be done in a further submission as long as you don't introduce another variation (which you don't expect for the details raised above).

On 2/9/2011 9:37 AM, Vincent Ladeuil wrote:
> 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.

Is there a way to get a config from a wt object now? I looked but either
I'm looking in the wrong place or it's not currently available.

Anyway, I've extracted that code out into a helper which takes a
mergetool name and config object.

>>>>> Gordon Tyler <email address hidden> writes:

    > On 2/9/2011 9:37 AM, Vincent Ladeuil wrote:
    >> 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.

    > Is there a way to get a config from a wt object now?

No.

    > I looked but either I'm looking in the wrong place or it's not
    > currently available.

    > Anyway, I've extracted that code out into a helper which takes a
    > mergetool name and config object.

Don't forget to put your proposal status to 'Needs Review' when you're
happy with the result.

      Vincent