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

Proposed by Gordon Tyler
Status: Work in progress
Proposed branch: lp:~doxxx/bzr/mergetools-commands
Merge into: lp:bzr
Prerequisite: lp:~doxxx/bzr/mergetools
Diff against target: 291 lines (+128/-19)
6 files modified
bzrlib/builtins.py (+41/-13)
bzrlib/conflicts.py (+45/-1)
bzrlib/mergetools.py (+18/-5)
bzrlib/tests/test_mergetools.py (+15/-0)
doc/en/release-notes/bzr-2.4.txt (+4/-0)
doc/en/whats-new/whats-new-in-2.4.txt (+5/-0)
To merge this branch: bzr merge lp:~doxxx/bzr/mergetools-commands
Reviewer Review Type Date Requested Status
Vincent Ladeuil Needs Fixing
Review via email: mp+40924@code.launchpad.net

Commit message

(doxxx) Added --resolve-using=ARG option to merge and remerge commands to resolve conflicts using an external merge tool.

Description of the change

Building on the new bzrlib.mergetools module added by lp:~doxxx/bzr/mergetools, this merge proposal adds a --resolve-using=ARG option to the ``bzr merge`` and ``bzr remerge`` commands to invoke the specified merge tool for each text conflict.

To post a comment you must log in.
Revision history for this message
Vincent Ladeuil (vila) wrote :

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 ?

review: Needs Information
Revision history for this message
Gordon Tyler (doxxx) wrote :

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?

Revision history for this message
Vincent Ladeuil (vila) wrote :

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

Revision history for this message
Gordon Tyler (doxxx) wrote :

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

Revision history for this message
Vincent Ladeuil (vila) wrote :
Download full text (3.6 KiB)

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

Read more...

review: Needs Fixing
Revision history for this message
Gordon Tyler (doxxx) wrote :
Download full text (4.3 KiB)

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

Read more...

Revision history for this message
Gordon Tyler (doxxx) wrote :

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.

Revision history for this message
Vincent Ladeuil (vila) wrote :

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

Revision history for this message
Gordon Tyler (doxxx) wrote :

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.

Revision history for this message
Gordon Tyler (doxxx) wrote :

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

lp:~doxxx/bzr/mergetools-commands updated
5420. By Gordon Tyler

Merged mergetools into mergetools-commands.

Revision history for this message
Gordon Tyler (doxxx) wrote :

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

Revision history for this message
Vincent Ladeuil (vila) wrote :

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.

review: Needs Fixing
Revision history for this message
Gordon Tyler (doxxx) wrote :

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

Revision history for this message
Gordon Tyler (doxxx) wrote :

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.

lp:~doxxx/bzr/mergetools-commands updated
5421. By Gordon Tyler

Merged from bzr.dev.

5422. By Gordon Tyler

resolve_conflicts returns 0 or first non-zero retval from mergetool.

5423. By Gordon Tyler

Cleanup according to vila's suggestions.

Revision history for this message
Vincent Ladeuil (vila) 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.
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.

Revision history for this message
Gordon Tyler (doxxx) wrote :

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.

Revision history for this message
Vincent Ladeuil (vila) wrote :

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

Revision history for this message
Gordon Tyler (doxxx) wrote :

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.

lp:~doxxx/bzr/mergetools-commands updated
5424. By Gordon Tyler

cmd_resolve: Changed mergetool user feedback to be consistent with auto mode.

5425. By Gordon Tyler

Make it clear that it's a merge tool action.

5426. By Gordon Tyler

Extracted helper function get_mergetool_cmdline.

Revision history for this message
Vincent Ladeuil (vila) wrote :

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

Unmerged revisions

5426. By Gordon Tyler

Extracted helper function get_mergetool_cmdline.

5425. By Gordon Tyler

Make it clear that it's a merge tool action.

5424. By Gordon Tyler

cmd_resolve: Changed mergetool user feedback to be consistent with auto mode.

5423. By Gordon Tyler

Cleanup according to vila's suggestions.

5422. By Gordon Tyler

resolve_conflicts returns 0 or first non-zero retval from mergetool.

5421. By Gordon Tyler

Merged from bzr.dev.

5420. By Gordon Tyler

Merged mergetools into mergetools-commands.

5419. By Gordon Tyler

Moved info from whats-new-in-2.3.txt to whats-new-in-2.4.txt.

5418. By Gordon Tyler

Merged mergetools into mergetools-commands.

5417. By Gordon Tyler

Moved mergetools-commands NEWS from bzr-2.3.txt to bzr-2.4.txt.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/builtins.py'
2--- bzrlib/builtins.py 2011-02-18 12:00:14 +0000
3+++ bzrlib/builtins.py 2011-02-27 14:40:46 +0000
4@@ -39,6 +39,7 @@
5 log,
6 merge as _mod_merge,
7 merge_directive,
8+ mergetools,
9 osutils,
10 reconfigure,
11 rename_map,
12@@ -196,6 +197,17 @@
13 return bzrdir.BzrDir.open_containing_tree_or_branch(filename)
14
15
16+def get_mergetool_cmdline(tool_name, config):
17+ cmdline = config.find_merge_tool(tool_name)
18+ if cmdline is None:
19+ raise errors.BzrCommandError(
20+ 'Unrecognized merge tool: %s' % tool_name)
21+ if not mergetools.check_availability(cmdline):
22+ raise errors.BzrCommandError(
23+ 'External merge tool is not available: %s' % tool_name)
24+ return cmdline
25+
26+
27 # TODO: Make sure no commands unconditionally use the working directory as a
28 # branch. If a filename argument is used, the first of them should be used to
29 # specify the branch. (Perhaps this can be factored out into some kind of
30@@ -3942,7 +3954,9 @@
31 Option('preview', help='Instead of merging, show a diff of the'
32 ' merge.'),
33 Option('interactive', help='Select changes interactively.',
34- short_name='i')
35+ short_name='i'),
36+ Option('resolve-using', help='Invokes the external merge tool named '
37+ 'ARG for each merged file with conflicts.', type=str),
38 ]
39
40 def run(self, location=None, revision=None, force=False,
41@@ -3951,6 +3965,7 @@
42 directory=None,
43 preview=False,
44 interactive=False,
45+ resolve_using=None
46 ):
47 if merge_type is None:
48 merge_type = _mod_merge.Merge3Merger
49@@ -4027,12 +4042,17 @@
50 "This branch has no commits."
51 " (perhaps you would prefer 'bzr pull')")
52 if preview:
53- return self._do_preview(merger)
54+ retval = self._do_preview(merger)
55 elif interactive:
56- return self._do_interactive(merger)
57+ retval = self._do_interactive(merger)
58 else:
59- return self._do_merge(merger, change_reporter, allow_pending,
60- verified)
61+ retval = self._do_merge(merger, change_reporter, allow_pending,
62+ verified)
63+ if retval != 0 and resolve_using is not None:
64+ config = _mod_config.GlobalConfig()
65+ cmdline = get_mergetool_cmdline(resolve_using, config)
66+ retval = mergetools.resolve_conflicts(tree.conflicts(), cmdline)
67+ return retval
68
69 def _get_preview(self, merger):
70 tree_merger = merger.make_merger()
71@@ -4231,14 +4251,16 @@
72 """
73 takes_args = ['file*']
74 takes_options = [
75- 'merge-type',
76- 'reprocess',
77- Option('show-base',
78- help="Show base revision text in conflicts."),
79- ]
80+ 'merge-type',
81+ 'reprocess',
82+ Option('show-base',
83+ help="Show base revision text in conflicts."),
84+ Option('resolve-using', help='Invokes the external merge tool named '
85+ 'ARG for each merged file with conflicts.', type=str),
86+ ]
87
88 def run(self, file_list=None, merge_type=None, show_base=False,
89- reprocess=False):
90+ reprocess=False, resolve_using=None):
91 from bzrlib.conflicts import restore
92 if merge_type is None:
93 merge_type = _mod_merge.Merge3Merger
94@@ -4296,9 +4318,15 @@
95 finally:
96 tree.set_parent_ids(parents)
97 if conflicts > 0:
98- return 1
99+ if resolve_using is not None:
100+ config = _mod_config.GlobalConfig()
101+ cmdline = get_mergetool_cmdline(resolve_using, config)
102+ retval = mergetools.resolve_conflicts(tree.conflicts(), cmdline)
103+ else:
104+ retval = 1
105 else:
106- return 0
107+ retval = 0
108+ return retval
109
110
111 class cmd_revert(Command):
112
113=== modified file 'bzrlib/conflicts.py'
114--- bzrlib/conflicts.py 2011-02-08 13:56:49 +0000
115+++ bzrlib/conflicts.py 2011-02-27 14:40:46 +0000
116@@ -26,7 +26,9 @@
117 from bzrlib import (
118 cleanup,
119 commands,
120+ config,
121 errors,
122+ mergetools,
123 osutils,
124 rio,
125 trace,
126@@ -115,9 +117,14 @@
127 'directory',
128 option.Option('all', help='Resolve all conflicts in this tree.'),
129 ResolveActionOption(),
130+ option.Option('using', help='Resolve conflicts using an '
131+ 'external merge tool.', type=str),
132 ]
133 _see_also = ['conflicts']
134- def run(self, file_list=None, all=False, action=None, directory=None):
135+ def run(self, file_list=None, all=False, action=None, directory=None,
136+ using=None):
137+ if using is not None:
138+ action = 'mergetool'
139 if all:
140 if file_list:
141 raise errors.BzrCommandError("If --all is specified,"
142@@ -158,6 +165,43 @@
143 # refactoring to transfer tree.auto_resolve() to
144 # conflict.auto(tree) --vila 091242
145 pass
146+ elif action == 'mergetool':
147+ conf = config.GlobalConfig()
148+ cmdline = conf.find_merge_tool(using)
149+ if cmdline is None:
150+ raise errors.BzrCommandError(
151+ 'Unrecognized merge tool: %s' % using)
152+ if not mergetools.check_availability(cmdline):
153+ raise errors.BzrCommandError(
154+ 'Merge tool is not available: %s' % using)
155+ if all:
156+ file_list = []
157+ for conflict in tree.conflicts():
158+ file_list.append(conflict.path)
159+ if file_list is None:
160+ raise errors.BzrCommandError(
161+ 'Either FILE(s) or --all must be provided')
162+ resolved = 0
163+ for file in file_list:
164+ # to avoid unnecessary './' prefix on file names
165+ if directory != u'.' and directory is not None:
166+ file = os.path.join(directory, file)
167+ trace.mutter('Invoking mergetool %r on %r...' %
168+ (merge_tool.get_name(), file))
169+ retval = mergetools.invoke(cmdline, file)
170+ if retval == 0:
171+ resolve(tree, [file])
172+ resolved += 1
173+ unresolved = tree.conflicts()
174+ if len(unresolved) > 0:
175+ trace.note('%d conflict(s) resolved.' % resolved)
176+ trace.note('Remaining conflicts:')
177+ for conflict in unresolved:
178+ trace.note(str(conflict))
179+ return 1
180+ else:
181+ trace.note('All conflicts resolved.')
182+ return 0
183 else:
184 before, after = resolve(tree, file_list, action=action)
185 trace.note('%d conflict(s) resolved, %d remaining'
186
187=== modified file 'bzrlib/mergetools.py'
188--- bzrlib/mergetools.py 2011-01-21 23:51:15 +0000
189+++ bzrlib/mergetools.py 2011-02-27 14:40:46 +0000
190@@ -31,6 +31,8 @@
191 )
192 """)
193
194+from bzrlib.conflicts import TextConflict
195+
196
197 known_merge_tools = {
198 'bcompare': 'bcompare {this} {other} {base} {result}',
199@@ -68,9 +70,9 @@
200 invoker = subprocess_invoker
201 cmd_list = cmdline.split(command_line)
202 args, tmp_file = _subst_filename(cmd_list, filename)
203- def cleanup(retcode):
204+ def cleanup(retval):
205 if tmp_file is not None:
206- if retcode == 0: # on success, replace file with temp file
207+ if retval == 0: # on success, replace file with temp file
208 shutil.move(tmp_file, filename)
209 else: # otherwise, delete temp file
210 os.remove(tmp_file)
211@@ -111,6 +113,17 @@
212
213
214 def subprocess_invoker(executable, args, cleanup):
215- retcode = subprocess.call([executable] + args)
216- cleanup(retcode)
217- return retcode
218+ retval = subprocess.call([executable] + args)
219+ cleanup(retval)
220+ return retval
221+
222+
223+def resolve_conflicts(conflicts, command_line, invoker=None):
224+ for conflict in conflicts:
225+ if isinstance(conflict, TextConflict):
226+ retval = invoke(command_line, conflict.path, invoker=invoker)
227+ if retval != 0:
228+ trace.mutter('resolve_conflicts: %r exited with retval %d; '
229+ 'aborting' % (command_line, retval))
230+ return retval
231+ return 0
232
233=== modified file 'bzrlib/tests/test_mergetools.py'
234--- bzrlib/tests/test_mergetools.py 2011-01-20 04:44:14 +0000
235+++ bzrlib/tests/test_mergetools.py 2011-02-27 14:40:46 +0000
236@@ -24,6 +24,7 @@
237 tests
238 )
239 from bzrlib.tests.features import backslashdir_feature
240+from bzrlib.tests.test_conflicts import example_conflicts
241
242
243 class TestFilenameSubstitution(tests.TestCaseInTempDir):
244@@ -165,3 +166,17 @@
245 self.assertEqual(1, retcode)
246 self.assertEqual('tool', self._exe)
247 self.assertFileEqual('stuff', 'test.txt')
248+
249+
250+class TestResolve(tests.TestCaseInTempDir):
251+
252+ def test_resolve(self):
253+ self._commandlines = []
254+ def dummy_invoker(exe, args, cleanup):
255+ self._commandlines.append((exe, args))
256+ cleanup(0)
257+ return 0
258+ retval = mergetools.resolve_conflicts(example_conflicts, 'tool {result}',
259+ invoker=dummy_invoker)
260+ self.assertEqual([(u'tool', [u'p\xe5tha'])], self._commandlines)
261+ self.assertEqual(0, retval)
262
263=== modified file 'doc/en/release-notes/bzr-2.4.txt'
264--- doc/en/release-notes/bzr-2.4.txt 2011-02-25 02:01:51 +0000
265+++ doc/en/release-notes/bzr-2.4.txt 2011-02-27 14:40:46 +0000
266@@ -26,6 +26,10 @@
267 * External merge tools can now be configured in bazaar.conf. See
268 ``bzr help configuration`` for more information. (Gordon Tyler, #489915)
269
270+* Added ``--resolve-using`` option to ``bzr merge`` and ``bzr remerge``
271+ commands to invoke an external merge too to resolve text conflicts.
272+ (Gordon Tyler, #489915)
273+
274 * Configuration options can now use references to other options in the same
275 file by enclosing them with curly brackets (``{other_opt}``). This makes it
276 possible to use, for example,
277
278=== modified file 'doc/en/whats-new/whats-new-in-2.4.txt'
279--- doc/en/whats-new/whats-new-in-2.4.txt 2011-02-25 00:15:23 +0000
280+++ doc/en/whats-new/whats-new-in-2.4.txt 2011-02-27 14:40:46 +0000
281@@ -23,6 +23,11 @@
282 and commandline of one or more external merge tools can be defined in
283 bazaar.conf. See the help topic ``configuration`` for more details.
284
285+External merge tools can be invoked for files with text conflicts during
286+``bzr merge`` and ``bzr remerge`` operations by adding the ``--resolve-using``
287+option. They can also be used to resolve a text conflict with the ``--using``
288+option on the ``bzr resolve`` command.
289+
290 Tagged Revisions are Copied
291 ***************************
292