Merge lp:~vila/bzr/conflict-manager into lp:bzr

This patch implements some simple actions to resolve tree shape conflicts (that is, all conflicts that are not text conflicts) by providing --keep-mine and --take-their
to the resolve command.
Just marking the conflict as resolved is still accessible via the --done default action.

It also documents (mostly via hidden comments in documentation, TODOs and FIXMEs in the code)
the next steps I plan to work on.

But since this is already a significant patch, I'd like feedback before going further.

This looks a bit weird to me. Haven't done a full review, but for two-way conflicts, bzr takes the changes from OTHER, so keep_mine should never be a no-op, and take_their should often be a no-op.

take_their should actually be take_theirs. ("my" is to "their" as "mine" is to "theirs".)

Also, the safest way to rename multiple files at once is using a TreeTransform.

There are some grammar issues in the proposed new text:
+may conflict with the current state of your working tree.
+
+When conflicts are present in your working tree (as shown by ``bzr
+conflicts``), you should resolve them and then inform bzr that the conflicts
+has been resolved.

"have been resolved"

+
+Resolving conflicts is sometimes not obvious. Either because the user that
+should resolve them is not the one responsible for their occurrence, as is the
+case when merging other people work or because some conflicts are presented in

"other people's work"

+a way that is not easy to understand.
+
+Bazaar try to avoid conflicts, its aim is to ask you to resolve the conflict

"Bazaar tries to avoid conflicts; its aim is to ask you to resolve the conflict"

+if and only if there's an actual conceptual conflict in the source tree.
+Because Bazaar doesn't understand the real meaning of the files being
+versioned it can, when faced with ambiguities, fall short in either direction

"versioned,"

(But it may be better to restructure the sentence instead of adding commas.)

+trying to resolve the conflict itself. Many kinds of changes can be combined
+programmatically, but sometimes only a human can determine the right thing to
+do.
+
+When Bazaar generates a conflict, it adds information into the working tree to
+present the conflicting versions and it's up to you to find the correct

"versions,"

+resolution.
+
+Whatever the conflict is, resolving it is roughly done in two steps:
+
+- modify the working tree content so that the conflicted item is now in the
+ state you want to keep,
+
+- inform Bazaar that the conflict is now solved and ask to cleanup any
+ remaining generated information (``bzr resolve <item>``).
+
+For most conflict types, there are some obvious ways to modify the working
+tree and put it into the desired state. For some types of conflicts, Bazaar
+itself already made a choice when possible.

I think "for some types...when possible" is too cautious, but at any rate, "Bazaar itself already made a choice when possible" is ungrammatical. It should be "Bazaar itself will have already made a choice, when possible"

+Yet, whether Bazaar made a choice or not, there are some other simple but
+alternative ways to resolve the conflict.

"Bazaar makes a choice"

("alternative" tastes funny here. Maybe "different"?)

+Various actions are available depending on the kind of conflict, for some of
+these actions, Bazaar can provide some help. In the end you should at least
+inform Bazaar that you're done with the conflict with::
+
+ ``bzr resolve FILE --action=done'
+
+Note that this is the default action when a single file is involved so you can
+simply use::
+
+ ``bzr resolve FILE``

Why tell them to use --action=done and then immediately contradict yourself? And w...

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/conflict-manager into lp:bzr with lp:~vila/bzr/cleanup-conflicts as a prerequisite.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> This patch implements some simple actions to resolve tree shape conflicts (that is, all conflicts that are not text conflicts) by providing --keep-mine and --take-their
> to the resolve command.
> Just marking the conflict as resolved is still accessible via the --done default action.

Meta-comment

We seem a bit confused as to whether 'bzr resolve' is a command that has
actions, or whether it is a command that records actions you've already
done. I'm worried that this takes it a step further. (resolve
- --interactive is quite different from what 'resolve' does today.)

I'm wondering if we should be considering how to split out the current
functionality into separate commands...

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAktCTvwACgkQJdeBCYSNAAN5rgCggVpL7OvYmZSUwPFJ0/UHfjLq
N14An0NCsQrKF6+Q6+7xR++1F2y2CB/2
=1KQw
-----END PGP SIGNATURE-----

>>>>> "Aaron" == Aaron Bentley <email address hidden> writes:

    Aaron> This looks a bit weird to me. Haven't done a full
    Aaron> review, but for two-way conflicts, bzr takes the
    Aaron> changes from OTHER, so keep_mine should never be a
    Aaron> no-op, and take_their should often be a no-op.

I didn't touch the code that generate the conflicts, so any
incoherency there was there. The overall code is already hard to
modify so I don't intend to change that sort of thing, yet.

But, yes, I was surprised to find that we don't always act the
same way when faced with conflicts and I'd like to provide a more
"regular" behavior (like using .THIS .OTHER instead of .new or
.diverted for example).

Also, if there is a conflict, it sounds acceptable that *both*
keep_mine and take_theirs should not be no-ops... with the
down-side that this will left the working tree in a worse state
than choosing between THIS or OTHER.

I'd welcome any feedback about why we do things the way they are
done today !

    Aaron> take_their should actually be take_theirs. ("my" is
    Aaron> to "their" as "mine" is to "theirs".)

Damn, I wrote it --take-theirs first :)

    Aaron> Also, the safest way to rename multiple files at once
    Aaron> is using a TreeTransform.

Thanks. I think there is a single place that deserves that, I'll check.

    Aaron> There are some grammar issues in the proposed new
    Aaron> text:

Thanks, I addressed them in my branch and only answer other
points below.

<snip/>

    Aaron> +Various actions are available depending on the kind of conflict, for some of
    Aaron> +these actions, Bazaar can provide some help. In the end you should at least
    Aaron> +inform Bazaar that you're done with the conflict with::
    Aaron> +
    Aaron> + ``bzr resolve FILE --action=done'
    Aaron> +
    Aaron> +Note that this is the default action when a single file is involved so you can
    Aaron> +simply use::
    Aaron> +
    Aaron> + ``bzr resolve FILE``

    Aaron> Why tell them to use --action=done and then
    Aaron> immediately contradict yourself?

Because the former is explicit why the later is implicit (and
don't contradict each other). I try to always use the explicit
form in docs.

    Aaron> And why --action=done instead of --done ?

Same.

    Aaron> +When you have resolved text conflicts, just run ``bzr
    Aaron> +resolve --auto``, and Bazaar will auto-detect which
    Aaron> +conflicts you have resolved.

    Aaron> Requiring --auto is a regression IMO.

Hehe, this text doesn't imply that --auto is *required* it just
mention it explicitly (same rule as above).

Well, I didn't change the behavior of --auto (finally), but I
intend to. The code that still implements the magic behavior of
--auto. But as said, I find that behavior pretty surprising and
I think we should get rid of it.

141 + # FIXME: There is a special case here related to the option
142 + # handling that could be clearer and easier to discover by
143 + # providing an --auto action (bug #344013 and #383396) and
144 + # make it mandatory instead of implicit and active only
145 + # when no file_list is provided -- vila 091229

If this patch is accepted then --aut...

>>>>> "jam" == John A Meinel <email address hidden> writes:

<snip/>

    jam> We seem a bit confused as to whether 'bzr resolve' is a
    jam> command that has actions, or whether it is a command
    jam> that records actions you've already done.

Yes. It's surprising. But 'resolved', which reflects more closely
the actual behavior is already declared as an alias.

    jam> I'm worried that this takes it a step further. (resolve
    jam> --interactive is quite different from what 'resolve'
    jam> does today.)

Well, it literally resolves the conflict instead of just deleting
it (and I abandon the --interactive idea in favor of handling a
list of files (as resolve was already), GUIs may choose
differently but at least the command line UI provides the basis).

So it makes sense to use 'resolve' for the actions and 'resolved'
for just marking the conflicts as resolved.

But then, in 99% of the actions you also want to mark the
conflict as resolved !

In the end, I considered that I was providing more ways to
resolve the conflicts than the actual --done action.

    jam> I'm wondering if we should be considering how to split
    jam> out the current functionality into separate commands...

Well, we currently have: bzr resolve [--auto] {--all|FILE+}.

There is not much that will be lost except that the magic
behavior associated with --auto (disabled when you use --all)
will become explicit.

Do you have better names to propose ?

   Vincent

I started reviewing this the other day and didn't get a long way. I'm not sure I'm best qualified to comment on the bigger picture here: I'd like Aaron to sign off on that before this lands.

I'm more than happy to review the code for logic bugs and the doc for readability if that helps.

Revno 4668 takes review comments into account:
- settle on using --take-this/--take-other instead of --keep-mine/--take-theirs,
- stop using rest in conflict-types.txt since it's displayed by
  ``bzr help conflict-types`` without processing.

Overall, it doesn't modify any existing behaviour but adds new features
on ``bzr resolve``.

The next steps are described at
 http://wiki.bazaar.canonical.com/VincentLadeuil/ConflictResolution

Discussions may still be needed before the next submissions but this patch
itself shouldn't be controversial.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Vincent Ladeuil wrote:
> Revno 4668 takes review comments into account:
> - settle on using --take-this/--take-other instead of --keep-mine/--take-theirs,
> - stop using rest in conflict-types.txt since it's displayed by
> ``bzr help conflict-types`` without processing.

I would probably have left in the ReST syntax, since it is how other
bits are formatted, and leave to a separate action creating a plain-text
formatter for the rest documentation. (help strings, etc are all already
ReST or meant to be ReST, so we need to handle it anyway.)

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAktplFsACgkQJdeBCYSNAAO8BgCfeC/fFAMUm0XGyrDKJRwdnlLy
sUwAnjX6ZYNufZNRIVZ9veuWZSXwHmt5
=h1WN
-----END PGP SIGNATURE-----

>>>>> "jam" == John A Meinel <email address hidden> writes:

    jam> Vincent Ladeuil wrote:
    >> Revno 4668 takes review comments into account:
    >> - settle on using --take-this/--take-other instead of --keep-mine/--take-theirs,
    >> - stop using rest in conflict-types.txt since it's displayed by
    >> ``bzr help conflict-types`` without processing.

    jam> I would probably have left in the ReST syntax, since it is how other
    jam> bits are formatted, and leave to a separate action creating a plain-text
    jam> formatter for the rest documentation. (help strings, etc are all already
    jam> ReST or meant to be ReST, so we need to handle it anyway.)

And spit rest bits to the user when doing `bzr help conflict-types` ???

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Vincent Ladeuil wrote:
>>>>>> "jam" == John A Meinel <email address hidden> writes:
>
> jam> Vincent Ladeuil wrote:
> >> Revno 4668 takes review comments into account:
> >> - settle on using --take-this/--take-other instead of --keep-mine/--take-theirs,
> >> - stop using rest in conflict-types.txt since it's displayed by
> >> ``bzr help conflict-types`` without processing.
>
> jam> I would probably have left in the ReST syntax, since it is how other
> jam> bits are formatted, and leave to a separate action creating a plain-text
> jam> formatter for the rest documentation. (help strings, etc are all already
> jam> ReST or meant to be ReST, so we need to handle it anyway.)
>
> And spit rest bits to the user when doing `bzr help conflict-types` ???

Until we fix 'bzr help' we already do that for lots of other ones.

'bzr help init' shows a '::'
'bzr help init-repo' as well
'bzr help diff'
'bzr help log'

etc, etc, etc.

There is an open bug that we should use a plain-text formatter when
writing out ReST code, I'd rather fix that, then not have use use ReST
for help which gets translated into our HTML documentation.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAktpxSkACgkQJdeBCYSNAAMfCQCeJOtTHmoD7iy+nmhXH80AXD7I
8mgAoNV98mCOlfe2/62SdBnXFvGN47vF
=WUOX
-----END PGP SIGNATURE-----

John A Meinel wrote:
> Vincent Ladeuil wrote:

>> And spit rest bits to the user when doing `bzr help conflict-types` ???
>
> Until we fix 'bzr help' we already do that for lots of other ones.
>
> 'bzr help init' shows a '::'
> 'bzr help init-repo' as well
> 'bzr help diff'
> 'bzr help log'
>
> etc, etc, etc.
>
> There is an open bug that we should use a plain-text formatter when
> writing out ReST code, I'd rather fix that, then not have use use ReST
> for help which gets translated into our HTML documentation.

Some limited ReST translations gets done when outputting text help
currently, e.g.

:: at the ends of a line gets mapped to :
:doc:`xxx-help` gets mapped to `bzr help xxx`

We don't bundle DocTools or Sphinx but it is possible to write text
that is reasonable in both text output and html output given the rules
already in place. I'm sure we could tweak them further though.

Ian C.

>>>>> "Ian" == Ian Clatworthy <email address hidden> writes:

    Ian> John A Meinel wrote:
    >> Vincent Ladeuil wrote:

    >>> And spit rest bits to the user when doing `bzr help conflict-types` ???
    >>
    >> Until we fix 'bzr help' we already do that for lots of other ones.
    >>
    >> 'bzr help init' shows a '::'
    >> 'bzr help init-repo' as well
    >> 'bzr help diff'
    >> 'bzr help log'
    >>
    >> etc, etc, etc.
    >>
    >> There is an open bug that we should use a plain-text formatter when
    >> writing out ReST code, I'd rather fix that, then not have use use ReST
    >> for help which gets translated into our HTML documentation.

    Ian> Some limited ReST translations gets done when outputting text help
    Ian> currently, e.g.

    Ian> :: at the ends of a line gets mapped to :
    Ian> :doc:`xxx-help` gets mapped to `bzr help xxx`

    Ian> We don't bundle DocTools or Sphinx but it is possible to write text
    Ian> that is reasonable in both text output and html output given the rules
    Ian> already in place. I'm sure we could tweak them further though.

Ok, so I've tweaked back these changes, I originally revert the
rest changes because, thinking it *was* a rest document, I
embedded some TODO and FIXMEs in the comment which are *not* to
be displayed in a help output.

Now, can I have some *review* on the core of this patch or,
failing that, an agreement that since nobody cares enough and the
changes are unlikely to break anything I can just land it ?

Thanks in advance.

101 +class ResolveActionOption(option.RegistryOption):
102 +
103 + def __init__(self):
104 + super(ResolveActionOption, self).__init__(
105 + 'action', 'How to resolve the conflict.',
106 + value_switches=True,
107 + registry=resolve_action_registry)
108 +
109 +

^- This only seems useful if you were actually going to re-use the
ResolveActionOption.
I realize you have some tests for it, but it doesn't do anything beyond a
standard RegistryOption.

Were you thinking to have the '--auto' logic as part of the option object?

168 +def resolve(tree, paths=None, ignore_misses=False, recursive=False,
169 + action='done'):
^- Having a top-level function in builtins.py to do command specific actions
isn't particularly nice, though I realize this wasn't from you. Perhaps just a
comment that it should be moved into a bzrlib.resolve* module?

236 + def _do(self, action, tree):
237 + """Apply the specified action to the conflict.
238 +
239 + :param action: The method name to call.
240 +
241 + :param tree: The tree passed as a parameter to the method.
242 + """
243 + meth = getattr(self, action, None)
244 + if meth is None:
245 + raise NotImplementedError(self.__class__.__name__ + '.' + action)
246 + meth(tree)

^- To avoid namespace collisions, this would probably be better as:

meth = getattr(self, 'action_' + action, None)
...

=== modified file 'bzrlib/tests/test_conflicts.py'
971 --- bzrlib/tests/test_conflicts.py 2010-01-04 14:56:59 +0000
972 +++ bzrlib/tests/test_conflicts.py 2010-02-04 13:06:32 +0000
973 @@ -21,8 +21,10 @@
974 bzrdir,
975 conflicts,
976 errors,
977 + option,
978 tests,
979 )
980 +from bzrlib.tests import script

^- script tests don't seem to be a great match for whitebox testing. And
looking at the scripts themselves they certainly look like blackbox tests.
(using commit to assert that things are ok.) Versus even an "assert*" function.

The tests seem fine, but they look like they should belong in a
blackbox/test_resolve.py module.

> 101 +class ResolveActionOption(option.RegistryOption):
> 102 +
> 103 + def __init__(self):
> 104 + super(ResolveActionOption, self).__init__(
> 105 + 'action', 'How to resolve the conflict.',
> 106 + value_switches=True,
> 107 + registry=resolve_action_registry)
> 108 +
> 109 +
>
>
> ^- This only seems useful if you were actually going to re-use the
> ResolveActionOption.
> I realize you have some tests for it, but it doesn't do anything beyond a
> standard RegistryOption.

You're right, I introduced it for tests only.
The alternative was to define it in tests only too which was more likely to break in the future.

I can reconsider later if you feel strongly about it but overall I think it makes things simpler.

>
> Were you thinking to have the '--auto' logic as part of the option object?

Hopefully not :)

>
>
> 168 +def resolve(tree, paths=None, ignore_misses=False, recursive=False,
> 169 + action='done'):
> ^- Having a top-level function in builtins.py to do command specific actions
> isn't particularly nice, though I realize this wasn't from you. Perhaps just a
> comment that it should be moved into a bzrlib.resolve* module?

Meh, that's in conflicts.py and I don't think creating a resolve.py module is right. resolve is an action on conflict objects.

It *could* arguably become part of the MutableTree API and get a @needs_tree_write_lock
decorator or become a method of ConflictList.

>
>
> 236 + def _do(self, action, tree):
> 237 + """Apply the specified action to the conflict.
> 238 +
> 239 + :param action: The method name to call.
> 240 +
> 241 + :param tree: The tree passed as a parameter to the method.
> 242 + """
> 243 + meth = getattr(self, action, None)
> 244 + if meth is None:
> 245 + raise NotImplementedError(self.__class__.__name__ + '.' + action)
> 246 + meth(tree)
>
> ^- To avoid namespace collisions, this would probably be better as:
>
> meth = getattr(self, 'action_' + action, None)
> ...

Good point. Fixed.

>
>
> === modified file 'bzrlib/tests/test_conflicts.py'
> 971 --- bzrlib/tests/test_conflicts.py 2010-01-04 14:56:59 +0000
> 972 +++ bzrlib/tests/test_conflicts.py 2010-02-04 13:06:32 +0000
> 973 @@ -21,8 +21,10 @@
> 974 bzrdir,
> 975 conflicts,
> 976 errors,
> 977 + option,
> 978 tests,
> 979 )
> 980 +from bzrlib.tests import script
>
> ^- script tests don't seem to be a great match for whitebox testing. And
> looking at the scripts themselves they certainly look like blackbox tests.
> (using commit to assert that things are ok.) Versus even an "assert*"
> function.

>
> The tests seem fine, but they look like they should belong in a
> blackbox/test_resolve.py module.

Right, they served as a test bed for shell-like tests and like most shell-like tests they should be rewritten :) I'll add a TODO for that and will address it shortly.