Merge lp:~vila/bzr/638451-malformed into lp:bzr

[Vincent Ladeuil]

Implement resolve --take-this and --take-other for text conflicts.

This fixes bug #638451 by implementing resolve --take-this and
--take-other for text conflicts.

While this is exactly the use case reported in the bug (ignore
the merge result, give me the content of the file as it was in
the OTHER branch), I'm a bit hesitant: is it really the thing to
do ?

The alternative would be to define:

- take-this choose THIS in conflicted regions
- take-other choose OTHER in conflicted regions
- take-both take THIS and OTHER in conflicted regions
- restore-this take THIS overriding all cleanly merged modifications
- restore-other take OTHER overriding all cleanly merged modifications

Thoughts ?

Implementing the other actions will be significantly more work
than this patch so I'd like to land it as a first step but with
an agreement on the action names.

[Martin von Gagern (gagern)]

I'd like "resolve --take-this name" with its old semantics. To me it means "take name.THIS, discarding the files name.OTHER, name.BASE and name". It's a shorthand for a manual copy followed by a resolve.

I think the alternative you suggest makes some sense as well, in particular for file formats I prefer to edit with an application different from a text editor. But you'll have to examine the file for reproducable results: a more intelligent merger might in the future merge things successfully that fail to merge today, leading to different behaviour in this case.

I think the current --take-this should keep its name for the sake of backwards compatibility as well. Perhaps --prefer-this would be an option for adding the other approach one day.

Given that such an option would have to parse the current file in order to identify which changes have been merged and which haven't, I'm not sure I'd always trust its output. Perhaps for this reason you'd rather want this an option to remerge than to resolve. The workflow I've got in mind would be:
merge (reports difference)
optionally examine file (see where the changes are, what to look for later on)
remerge --{prefer|take}-this (decides on conflicting regions)
examine result (e.g. open in app, check XML wellformedness, or something like this)
resolve (takes the file off my TODO)
commit.

[Martin von Gagern (gagern)]

Gave the branch a quick test and verified that both my shell script and the blackbox test from my lp:~gagern/bzr/bug638451-resolve-take-other branch work as expected. I've also updated my branch to merge that unscripted blackbox test into the test suite. As scripted tests are far more readable, I wonder whether I should turn it into such a scripted test, or whether you will do so. Currently your branch doesn't seem to actually have a blackbox test for --tahe-this or --take-other, does it?

[Vincent Ladeuil (vila)]

>>>>> Martin von Gagern <email address hidden> writes:

    > Gave the branch a quick test and verified that both my shell
    > script and the blackbox test from my
    > lp:~gagern/bzr/bug638451-resolve-take-other branch work as
    > expected.

Great, thanks !

    > I've also updated my branch to merge that unscripted blackbox test
    > into the test suite. As scripted tests are far more readable, I
    > wonder whether I should turn it into such a scripted test, or
    > whether you will do so.

    > Currently your branch doesn't seem to actually have a blackbox
    > test for --tahe-this or --take-other, does it?

No it doesn't, it has whitebox tests for it in
bt.test_conflicts.TestResolveTextConflicts.

These tests ensures that both --take-this and --take-other are covered as
well as merging both branches in both directions.

I didn't try to write blackbox tests ensuring the same coverage so far
and I'm not sure it would be the right approach. I agree they are more
readable and easier to write for people that doesn't have the needed
knowledge of the bzr internals but in the long run we don't want to
write blackbox tests if more precise tests can be written as they are
harder to debug when they fail (if only because stdin/stdout/stderr are
redirected which forbids using pdb...).

There are still a lot more tests to be written though, for example I
suspect that if the user delete the .OTHER file before running bzr
resolve --take-other bzr will issue a traceback instead of a nicer error
message but there is a related FIXME in bt.test_conflicts about
robustness.

[Neil Martinsen-Burrell (nmb)]

As someone who experienced this bug (or its more recent kin) I can say that what I expected was that "resolve --take-other" would be a direct synonym for "cp file.OTHER file; bzr resolved file". I think that giving a simple meaning to --take-this and --take-other for text conflicts is more valuable than discussing the possibility of a richer set of actions. In the interest of backwards compatibility and simplicity, I think that --take-this and --take-other should retain the simple meanings and other, more complex actions should get new names.

[John A Meinel (jameinel)]

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

On 11/19/2010 9:27 AM, Neil Martinsen-Burrell wrote:
> As someone who experienced this bug (or its more recent kin) I can say that what I expected was that "resolve --take-other" would be a direct synonym for "cp file.OTHER file; bzr resolved file". I think that giving a simple meaning to --take-this and --take-other for text conflicts is more valuable than discussing the possibility of a richer set of actions. In the interest of backwards compatibility and simplicity, I think that --take-this and --take-other should retain the simple meanings and other, more complex actions should get new names.

Just to mention, that *my* belief would be to use the content from the
repository, rather than whatever was on disk. (In case someone modified
the files).

I don't know if that is how other people would see it, though.

John
=:->

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

iEYEARECAAYFAkzmp4kACgkQJdeBCYSNAAOHZACgtflEFqnEF/PVaZrPCoIF23jo
sFcAn32GnNw91vhtAukmymAnx1oCXwHd
=LWhY
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

>>>>> John A Meinel <email address hidden> writes:

    > On 11/19/2010 9:27 AM, Neil Martinsen-Burrell wrote:
    >> As someone who experienced this bug (or its more recent kin) I can say that what I expected was that "resolve --take-other" would be a direct synonym for "cp file.OTHER file; bzr resolved file". I think that giving a simple meaning to --take-this and --take-other for text conflicts is more valuable than discussing the possibility of a richer set of actions. In the interest of backwards compatibility and simplicity, I think that --take-this and --take-other should retain the simple meanings and other, more complex actions should get new names.

    > Just to mention, that *my* belief would be to use the content from the
    > repository, rather than whatever was on disk. (In case someone modified
    > the files).

Yeah, there is a lot to discuss here.

If the user hasn't modified the .OTHER file, then the end result is the
same (the .OTHER file was created from the repo).

If the user has modified the .OTHER file, then restoring from the repo
will get rid of the user modifications which will be surprising (which
of the two scenarios is the most surprising is open to debate IMHO).

In the end, after conflicts have been resolved, and before committing,
you're supposed to review the result so whatever we implement, the
outcome is still visible to the user.

I'm still slightly prefering taking the .OTHER file over restoring from
the repo as it's more in line with the resolve 'spirit': We don't try to
guess, we just take what the user gives us (the plan is still to
document what --take-other does (whatever we chose)).

Also keep in mind that when 'bzr resolve --take-other' is used, without
any file (and all the related bug reports were cases were people using
that form) essentially means:

- ok, bzr has merged everything it can and left conflicts,

- for those conflicts, the other side is right, take it.

So we are not doing a 'pull --overwrite OTHER', we have merged cleanly
as best as we could. The user may have already resolved some other
conflicts manually (or not) and want to complete his resolution process.

So there is a grey area: the user did "stuff" and we should respect
that.

In my mind taking the file is closer to what GUI conflict handling
allows *inside* a text file with conflicts: you can chose the THIS part
or the OTHER part or both *including* any editing you may have done to
resolve the conflict in a finer way.

All in all, both features make sense and both can implemented as actions
(and both can also be achieved by other means), so we have to decide
which one we implement.

Thanks to all for the feedback anyway and don't forget to vote ;)

[Martin Pool (mbp)]

My expectation is that it would probably come from the repo, even if
you have modified the OTHER file. At any rate that is a reasonable
question so it should be documented.

I think you should mention this in the user guide, under 'resolving
conflicts', even adding a stub of this section if there is not one
already. So 'needsfixing' for docs, at least.

I agree with MvG, we could use --prefer-this/other.

This increases the tension about the conflation of:

 * update the working copy for a conflicted file
 * mark this conflict as having been resolved
 * forget this conflict ever occurred

However we can get there later.

--
Martin

[John A Meinel (jameinel)]

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

On 11/23/2010 1:34 AM, Martin Pool wrote:
> My expectation is that it would probably come from the repo, even if
> you have modified the OTHER file. At any rate that is a reasonable
> question so it should be documented.
>
> I think you should mention this in the user guide, under 'resolving
> conflicts', even adding a stub of this section if there is not one
> already. So 'needsfixing' for docs, at least.
>
> I agree with MvG, we could use --prefer-this/other.
>
> This increases the tension about the conflation of:
>
> * update the working copy for a conflicted file
> * mark this conflict as having been resolved
> * forget this conflict ever occurred
>
> However we can get there later.
>

I think the best answer to that I've seen so far is to add a new command.

"bzr resolved" to do what 'bzr resolve' does today. (Mark something as
manually resolved.)

John
=:->

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

iEYEARECAAYFAkzr0ygACgkQJdeBCYSNAAMzZgCgpgYKKgqdwuGXB9DbAFZ/4zP9
HfwAoLHqHcLarXoZ418UI774cb0L6XqI
=2VoH
-----END PGP SIGNATURE-----

[Martin Pool (mbp)]

On 24 November 2010 01:43, John Arbash Meinel <email address hidden> wrote:
>> This increases the tension about the conflation of:
>>
>>  * update the working copy for a conflicted file
>>  * mark this conflict as having been resolved
>>  * forget this conflict ever occurred
>>
>> However we can get there later.
>>
>
> I think the best answer to that I've seen so far is to add a new command.
>
> "bzr resolved" to do what 'bzr resolve' does today. (Mark something as
> manually resolved.)

I think we do need one command for the first action, and one for the
other two. I don't think we should have those commands differ only in
grammatical tense or only by one letter.

--
Martin

[lp:~vila/bzr/638451-malformed]

4654. By Vincent Ladeuil on 2010-11-12

Merge reports-conflict-resolved into 638451-malformed

4655. By Vincent Ladeuil on 2010-11-12

Merge reports-conflict-resolved into 638451-malformed

4656. By Vincent Ladeuil on 2010-11-13

Merge cleanup into 638451-malformed

4657. By Vincent Ladeuil on 2010-11-24

Merge cleanup into 638451-malformed

4658. By Vincent Ladeuil on 2010-11-24

Describe --take-this and --take-other actions for text conflicts.

4659. By Vincent Ladeuil on 2010-11-24

Fix typo.

4660. By Vincent Ladeuil on 2010-12-03

Merge cleanup into 638451-malformed

4661. By Vincent Ladeuil on 2010-12-03

Fix the news entry.

Unmerged revisions

=== modified file 'bzrlib/conflicts.py'
--- bzrlib/conflicts.py	2010-11-07 16:09:43 +0000
+++ bzrlib/conflicts.py	2010-11-24 16:33:38 +0000
@@ -626,6 +626,37 @@
     def associated_filenames(self):
         return [self.path + suffix for suffix in CONFLICT_SUFFIXES]
 
+    def _resolve(self, tt, winner_suffix):
+        """Resolve the conflict by copying one of .THIS or .OTHER into file.
+
+        :param tt: The TreeTransform where the conflict is resolved.
+        :param winner_suffix: Either 'THIS' or 'OTHER'
+
+        The resolution is symmetric, when taking THIS, item.THIS is renamed
+        into item and vice-versa. This takes one of the files as a whole
+        ignoring every difference that could have been merged cleanly.
+        """
+        # To avoid useless copies, we switch item and item.winner_suffix, only
+        # item will exist after the conflict has been resolved anyway.
+        item_tid = tt.trans_id_file_id(self.file_id)
+        item_parent_tid = tt.get_tree_parent(item_tid)
+        winner_path = self.path + '.' + winner_suffix
+        winner_tid = tt.trans_id_tree_path(winner_path)
+        winner_parent_tid = tt.get_tree_parent(winner_tid)
+        # Switch the paths to preserve the content
+        tt.adjust_path(self.path, winner_parent_tid, winner_tid)
+        tt.adjust_path(winner_path, item_parent_tid, item_tid)
+        # Associate the file_id to the right content
+        tt.unversion_file(item_tid)
+        tt.version_file(self.file_id, winner_tid)
+        tt.apply()
+
+    def action_take_this(self, tree):
+        self._resolve_with_cleanups(tree, 'THIS')
+
+    def action_take_other(self, tree):
+        self._resolve_with_cleanups(tree, 'OTHER')
+
 
 class HandledConflict(Conflict):
     """A path problem that has been provisionally resolved.

=== modified file 'bzrlib/help_topics/en/conflict-types.txt'
--- bzrlib/help_topics/en/conflict-types.txt	2010-10-15 14:57:42 +0000
+++ bzrlib/help_topics/en/conflict-types.txt	2010-11-24 16:33:38 +0000
@@ -87,11 +87,11 @@
 
 The correct resolution would be "Martin Pool released Bazaar."
 
-You can handle text conflicts either by editing the main copy of the file, or
-by invoking external tools on the THIS, OTHER and BASE versions.  It's worth
-mentioning that resolving text conflicts rarely involves picking one set of
-changes over the other.  More often, the two sets of changes must be
-intelligently combined.
+You can handle text conflicts either by editing the main copy of the file,
+or by invoking external tools on the THIS, OTHER and BASE versions.  It's
+worth mentioning that resolving text conflicts rarely involves picking one
+set of changes over the other (but see below when you encounter these
+cases).  More often, the two sets of changes must be intelligently combined.
 
 If you edit the main copy, be sure to remove the herringbone markers.  When
 you are done editing, the file should look like it never had a conflict, and be
@@ -104,6 +104,16 @@
 ``.BASE``, ``.THIS`` and ``.OTHER`` files if they are still present in the
 working tree.
 
+
+When you want to pick one set of changes over the other, you can use ``bzr
+resolve`` with one of the following actions:
+
+* ``--action=take-this`` will issue ``mv FILE.THIS FILE``,
+* ``--action=take-other`` will issue ``mv FILE.OTHER FILE``.
+
+Note that if you have modified ``FILE.THIS`` or ``FILE.OTHER``, these
+modifications will be taken into account.
+
 Content conflicts
 -----------------
 

=== modified file 'bzrlib/tests/blackbox/test_resolve.py'
--- bzrlib/tests/blackbox/test_resolve.py	2010-11-07 16:09:43 +0000
+++ bzrlib/tests/blackbox/test_resolve.py	2010-11-24 16:33:38 +0000
@@ -91,48 +91,3 @@
         self.build_tree_contents([('tree/file', 'a\n')])
         note = self.run_bzr('resolve', working_dir='tree')[1]
         self.assertContainsRe(note, 'All conflicts resolved.')
-
-
-class TestResolveSilentlyIgnore(script.TestCaseWithTransportAndScript):
-
-    def test_bug_646961(self):
-        self.run_script("""\
-            $ bzr init base
-            Created a standalone tree (format: 2a)
-            $ cd base
-            $ echo >file1
-            $ bzr add
-            adding file1
-            $ bzr ci -m "stuff"
-            2>Committing to: .../base/
-            2>added file1
-            2>Committed revision 1.
-            $ cd ..
-            $ bzr branch base branch
-            2>Branched 1 revision(s).
-            $ cd base
-            $ echo "1" >> file1
-            $ bzr ci -m "branch 1"
-            2>Committing to: .../base/
-            2>modified file1
-            2>Committed revision 2.
-            $ cd ../branch
-            $ echo "2" >> file1
-            $ bzr ci -m "branch 2"
-            2>Committing to: .../branch/
-            2>modified file1
-            2>Committed revision 2.
-            $ cd ../base
-            $ bzr merge ../branch
-            2> M  file1
-            2>Text conflict in file1
-            2>1 conflicts encountered.
-            # The following succeeds silently without resolving the conflict
-            $ bzr resolve file1 --take-other
-            2>0 conflict(s) resolved, 1 remaining
-            # The following wil fail when --take-other is implemented
-            # for text conflicts
-            $ bzr conflicts
-            Text conflict in file1
-            """)
-

=== modified file 'bzrlib/tests/test_conflicts.py'
--- bzrlib/tests/test_conflicts.py	2010-11-10 11:15:50 +0000
+++ bzrlib/tests/test_conflicts.py	2010-11-24 16:33:38 +0000
@@ -197,11 +197,6 @@
         self.run_script(self.preamble)
 
 
-class TestResolveTextConflicts(TestResolveConflicts):
-    # TBC
-    pass
-
-
 def mirror_scenarios(base_scenarios):
     """Return a list of mirrored scenarios.
 
@@ -372,6 +367,50 @@
         check_other()
 
 
+class TestResolveTextConflicts(TestParametrizedResolveConflicts):
+
+    _conflict_type = conflicts.TextConflict
+
+    # Set by the scenarios
+    # path and file-id for the file involved in the conflict
+    _path = None
+    _file_id = None
+
+    scenarios = mirror_scenarios(
+        [
+            # File modified/deleted
+            (dict(_base_actions='create_file',
+                  _path='file', _file_id='file-id'),
+             ('filed_modified_A',
+              dict(actions='modify_file_A', check='file_has_content_A')),
+             ('file_modified_B',
+              dict(actions='modify_file_B', check='file_has_content_B')),),
+            ])
+
+    def do_create_file(self):
+        return [('add', ('file', 'file-id', 'file', 'trunk content\n'))]
+
+    def do_modify_file_A(self):
+        return [('modify', ('file-id', 'trunk content\nfeature A\n'))]
+
+    def do_modify_file_B(self):
+        return [('modify', ('file-id', 'trunk content\nfeature B\n'))]
+
+    def check_file_has_content_A(self):
+        self.assertFileEqual('trunk content\nfeature A\n', 'branch/file')
+
+    def check_file_has_content_B(self):
+        self.assertFileEqual('trunk content\nfeature B\n', 'branch/file')
+
+    def _get_resolve_path_arg(self, wt, action):
+        return self._path
+
+    def assertTextConflict(self, wt, c):
+        self.assertEqual(self._file_id, c.file_id)
+        self.assertEqual(self._path, c.path)
+    _assert_conflict = assertTextConflict
+
+
 class TestResolveContentsConflict(TestParametrizedResolveConflicts):
 
     _conflict_type = conflicts.ContentsConflict

=== modified file 'bzrlib/transform.py'
--- bzrlib/transform.py	2010-11-19 16:28:25 +0000
+++ bzrlib/transform.py	2010-11-24 16:33:38 +0000
@@ -130,7 +130,7 @@
             self._new_root = self.trans_id_tree_file_id(root_id)
         else:
             self._new_root = None
-        # Indictor of whether the transform has been applied
+        # Indicator of whether the transform has been applied
         self._done = False
         # A progress bar
         self._pb = pb

=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt	2010-11-22 03:35:24 +0000
+++ doc/en/release-notes/bzr-2.3.txt	2010-11-24 16:33:38 +0000
@@ -35,6 +35,11 @@
   options matching a given regular expression is now controlled via the
   ``--all`` option.  (Vincent Ladeuil, bug #670251)
 
+* ``bzr resolve`` now accepts ``--take-this`` and ``--take-other`` actions
+  for text conflicts. This *replace* the whole file with the content
+  designated by the action. This will *ignore* all differences that would
+  have been merge cleanly otherwise. (Vincent Ladeuil, #638451)
+
 * ``bzr resolve`` now reports the number of conflicts resolved and the
   number of remaining conflicts. This provides a better feedback about the
   whole resolution process. (Vincent Ladeuil)

=== modified file 'doc/en/whats-new/whats-new-in-2.3.txt'
--- doc/en/whats-new/whats-new-in-2.3.txt	2010-11-22 03:35:24 +0000
+++ doc/en/whats-new/whats-new-in-2.3.txt	2010-11-24 16:33:38 +0000
@@ -118,6 +118,12 @@
   default behaviour is specified (if needed) by setting the variable to
   ``conflict``.  (Vincent Ladeuil, #323111)
 
+* ``bzr resolve --take-this`` and ``bzr resolve --take-other`` can now be
+  used for text conflicts. This will ignore the differences that were merged
+  cleanly and replace the file with its content in the current branch
+  (``--take-this``) or with its content in the merged branch
+  (``--take-other``). (Vincent Ladeuil, #638451)
+
 * ``bzr resolve`` now provides more feedback about the conflicts just
   resolved and the remaining ones. (Vincent Ladeuil)