Code review comment for lp:~vila/bzr/conflict-manager

Revision history for this message
Aaron Bentley (abentley) wrote :

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 why --action=done instead of --done ?

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

Requiring --auto is a regression IMO.

+
+When the conflict is resolved, Bazaar deletes the previously generated
+``.BASE``, ``.THIS`` and ``.OTHER`` files if they are still present in the
+working tree.
+
+.. TODO: There are 6 ways to resolve text conflicts:
+ 1. Manually
+ 1. prefer-mine choose THIS in conflicted regions
+ 1. prefer-theirs choose OTHER in conflicted regions
+ 1. keep-both take THIS and OTHER in conflicted regions
+ 1. keep-mine take THIS overriding all cleanly merged modifications
+ 1. take-theirs take OTHER overriding all cleanly merged modifications

 Content conflicts
 -----------------
@@ -64,20 +119,31 @@

   Contents conflict in FILE

-To resolve this, use "bzr mv" to rename the file back to its normal name, and
-combine the changes manually. When you are satisfied, run "bzr resolve
-FILE". Bazaar cannot auto-detect when conflicts of this kind have been
-resolved.
+To resolve that kind of conflict, you should rebuild FILE from either version
+or a combination of both.
+
+``bzr resolve`` recognizes the following actions:
+
+- ``--action=keep-mine`` will issue ``bzr mv FILE.THIS FILE``,
+- ``--action=take-theirs`` will issue ``bzr mv FILE.OTHER FILE``,
+- ``--action=done`` will just mark the conflict as resolved.
+
+Any action will also delete the previously generated ``.BASE``, ``.THIS`` and
+``.OTHER`` files if they are still present in the working tree.
+
+Bazaar cannot auto-detect when conflicts of this kind have been resolved.
+.. TODO: if either .THIS or .OTHER doesn't exist anymore and FILE exists
+again, --auto may consider the conflict as resolved.

"--auto could consider"

("may" in this context has meaning 1: http://dictionary.reference.com/browse/may, i.e. "it is already possible that --auto will consider the conflict as resolved")

"consider the conflict to be resolved" or "mark the conflict as resolved".

« Back to merge proposal