Merge lp:~vila/bzr/646961-attr-error into lp:bzr/2.2

[Vincent Ladeuil]

Properly refuse to obey --take-this and --take-other for text conflicts.

The root cause of bug #646961 is that --take-this and
--take-other are not implemented for text conflicts (and the
reason for this is that users may be misunderstanding the
feature, see
http://wiki.bazaar.canonical.com/VincentLadeuil/ConflictResolution
for details).

This patch fixes the most urgent issue targeted to 2.2 without
adding a new feature (since 2.2 is stable) by fixing several
issues (whack-a-mole entertainment ahead):

- the TextConflict class was inheriting from PathConflict instead
  of Conflict. There was a FIXME about that but I didn't realize
  at the time that this was inheriting action_take_this and
  action_take_other and doing so, pretending to implement
  --take-this and --take-other leading to the bug,

- the _duplicate_entries conflict detection was trying to take
  deleted files into account which makes no sense since they
  don't have a name anymore (while the code path shouldn't have
  been triggered, this is worth fixing anyway).

The TextConflict doesn't inherit from PathConflict anymore which
makes the test complete without resolving the conflict at
all. It's unfortunate that we succeed silently here but fixing
that is out of scope as it's related to several other bugs.
The added test will fail when --take-other is implemented.

[Andrew Bennetts (spiv)]

I have some tweak requests, so:

 review needs-fixing

But basically this looks like a good incremental improvement, thanks!

Details follow.

Vincent Ladeuil wrote:
[...]
> The TextConflict doesn't inherit from PathConflict anymore which
> makes the test complete without resolving the conflict at
> all. It's unfortunate that we succeed silently here but fixing
> that is out of scope as it's related to several other bugs.
> The added test will fail when --take-other is implemented.

That's a shame, but at least it's an improvement over crashing.

How hard is it to emit a warning that not all conflicts were resolved?

> --
> https://code.launchpad.net/~vila/bzr/646961-attr-error/+merge/39852
> Your team bzr-core is requested to review the proposed merge of lp:~vila/bzr/646961-attr-error into lp:bzr/2.2.

> === modified file 'NEWS'
> --- NEWS 2010-10-30 06:59:55 +0000
> +++ NEWS 2010-11-02 14:35:59 +0000
> @@ -19,6 +19,9 @@
> Bug Fixes
> *********
>
> +* ``bzr resolve --take--other <file>`` will not crash anymore if ``<file>`` is
> + involved in a text conflict. (Vincent Ladeuil, #646961)

Typo: should be “--take-other” (three hyphens, not four).

If I understand the cover letter correctly, in this scenario
--take-other still won't resolve the conflict. It might be worth saying
so in the NEWS entry.

[...]
> +++ bzrlib/conflicts.py 2010-11-02 14:35:59 +0000
> @@ -600,12 +600,9 @@
> self._resolve_with_cleanups(tree, 'THIS')
>
>
> -# FIXME: TextConflict is about a single file-id, there never is a conflict_path
> -# attribute so we shouldn't inherit from PathConflict but simply from Conflict
> -
> # TODO: There should be a base revid attribute to better inform the user about
> # how the conflicts were generated.
> -class TextConflict(PathConflict):
> +class TextConflict(Conflict):

Sounds good. Does this mean that PathConflict.as_stanza no longer needs this
logic:

        if self.conflict_path is not None:
            s.add('conflict_path', self.conflict_path)

?

> === modified file 'bzrlib/tests/blackbox/test_conflicts.py'
[...]
> +class TestResolveSilentlyIgnore(script.TestCaseWithTransportAndScript):
> +
> + def test_bug_646961(self):
[...]
> + # The following wil fail when --take-other is implemented
> + # for text conflicts

Shouldn't this test be a KnownFailure, then?

-Andrew.

review: Needs Fixing

[Vincent Ladeuil (vila)]

>>>>> Andrew Bennetts <email address hidden> writes:

<snip/>

    > How hard is it to emit a warning that not all conflicts were
    > resolved?

Tedious more than hard, there is a code path that already mentions how
many conflicts were resolved :-/

This is certainly a feature worth generalizing, but it's not trivial
(yet).

Something like 'n conflicts resolved, m remaining conflicts' can be
added to the other code path. I'll look into adding it to a further
submission against trunk.

<snip/>

    > Typo: should be “--take-other” (three hyphens, not four).

Fixed.

    > If I understand the cover letter correctly, in this scenario
    > --take-other still won't resolve the conflict. It might be worth saying
    > so in the NEWS entry.

You're right, this is *still* not resolving the conflict but at least
two users missed this fact, so it's time to implement it but worth
reminding the lack of the feature in the NEWS.

<snip/>

    > Sounds good. Does this mean that PathConflict.as_stanza no longer needs this
    > logic:

    > if self.conflict_path is not None:
    > s.add('conflict_path', self.conflict_path)

    > ?

Probably, I have a note about it, I'll add tests to ensure this is true
in my next proposal.

    >> === modified file 'bzrlib/tests/blackbox/test_conflicts.py'
    > [...]
    >> +class TestResolveSilentlyIgnore(script.TestCaseWithTransportAndScript):
    >> +
    >> + def test_bug_646961(self):
    > [...]
    >> + # The following wil fail when --take-other is implemented
    >> + # for text conflicts

    > Shouldn't this test be a KnownFailure, then?

Well, I used a script test because 1) the bug reporter had written in a
way that I could reuse with only little tweaks 2) to help track the bug by
reproducing it and ensure my patch was fixing it.

But I plan to get rid of it in the next submission so all I wanted was a
guarantee that it will fail once the feature is implemented and the
conflict resolved. So trying to inject a KnownFailure there sounded less
interesting than documenting that the conflict wasn't resolved.

Thanks for the prompt review !

[Vincent Ladeuil (vila)]

So, I will consider that some tweaks being addressed I can land this for 2.2 and will follow-up with a submission against *trunk* for the others (report conflicts resolved and as_stanza).

I'll make yet another submission to *implement* --take-this/--take-other for text conflicts (but I'd like to also add --prefer-this/--prefer-other/--take-both for that and it's a bit more work).

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'NEWS'
--- NEWS	2010-10-30 06:59:55 +0000
+++ NEWS	2010-11-07 10:22:42 +0000
@@ -19,6 +19,10 @@
 Bug Fixes
 *********
 
+* ``bzr resolve --take-other <file>`` will not crash anymore if ``<file>``
+  is involved in a text conflict (but the conflict is still not
+  resolved). (Vincent Ladeuil, #646961)
+
 * Correctly set the Content-Type  header when http POSTing to comply
   with stricter web frameworks. (Vincent Ladeuil, #655100)
 

=== modified file 'bzrlib/conflicts.py'
--- bzrlib/conflicts.py	2010-05-28 14:15:28 +0000
+++ bzrlib/conflicts.py	2010-11-07 10:22:42 +0000
@@ -600,12 +600,9 @@
         self._resolve_with_cleanups(tree, 'THIS')
 
 
-# FIXME: TextConflict is about a single file-id, there never is a conflict_path
-# attribute so we shouldn't inherit from PathConflict but simply from Conflict
-
 # TODO: There should be a base revid attribute to better inform the user about
 # how the conflicts were generated.
-class TextConflict(PathConflict):
+class TextConflict(Conflict):
     """The merge algorithm could not resolve all differences encountered."""
 
     has_files = True
@@ -614,6 +611,8 @@
 
     format = 'Text conflict in %(path)s'
 
+    rformat = '%(class)s(%(path)r, %(file_id)r)'
+
     def associated_filenames(self):
         return [self.path + suffix for suffix in CONFLICT_SUFFIXES]
 

=== modified file 'bzrlib/tests/blackbox/test_conflicts.py'
--- bzrlib/tests/blackbox/test_conflicts.py	2010-05-28 14:15:28 +0000
+++ bzrlib/tests/blackbox/test_conflicts.py	2010-11-07 10:22:42 +0000
@@ -19,6 +19,8 @@
     tests,
     workingtree,
     )
+from bzrlib.tests import script
+
 
 # FIXME: These don't really look at the output of the conflict commands, just
 # the number of lines - there should be more examination.
@@ -126,3 +128,46 @@
         self.assertEqual('', err)
         out, err = self.run_bzr('conflicts')
         self.assertEqual(0, len(out.splitlines()))
+
+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
+            # The following wil fail when --take-other is implemented
+            # for text conflicts
+            $ bzr conflicts
+            Text conflict in file1
+            """)
+

=== modified file 'bzrlib/tests/test_workingtree.py'
--- bzrlib/tests/test_workingtree.py	2010-06-20 11:18:38 +0000
+++ bzrlib/tests/test_workingtree.py	2010-11-07 10:22:42 +0000
@@ -349,28 +349,28 @@
         self.build_tree_contents([('this/hello', 'Hello World')])
         this.commit('Add World')
         this.merge_from_branch(other.branch)
-        self.assertEqual([conflicts.TextConflict('hello', None, 'hello_id')],
+        self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],
                          this.conflicts())
         this.auto_resolve()
-        self.assertEqual([conflicts.TextConflict('hello', None, 'hello_id')],
+        self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],
                          this.conflicts())
         self.build_tree_contents([('this/hello', '<<<<<<<')])
         this.auto_resolve()
-        self.assertEqual([conflicts.TextConflict('hello', None, 'hello_id')],
+        self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],
                          this.conflicts())
         self.build_tree_contents([('this/hello', '=======')])
         this.auto_resolve()
-        self.assertEqual([conflicts.TextConflict('hello', None, 'hello_id')],
+        self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],
                          this.conflicts())
         self.build_tree_contents([('this/hello', '\n>>>>>>>')])
         remaining, resolved = this.auto_resolve()
-        self.assertEqual([conflicts.TextConflict('hello', None, 'hello_id')],
+        self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],
                          this.conflicts())
         self.assertEqual([], resolved)
         self.build_tree_contents([('this/hello', 'hELLO wORLD')])
         remaining, resolved = this.auto_resolve()
         self.assertEqual([], this.conflicts())
-        self.assertEqual([conflicts.TextConflict('hello', None, 'hello_id')],
+        self.assertEqual([conflicts.TextConflict('hello', 'hello_id')],
                          resolved)
         self.failIfExists('this/hello.BASE')
 
@@ -378,7 +378,7 @@
         tree = self.make_branch_and_tree('tree')
         self.build_tree(['tree/hello/'])
         tree.add('hello', 'hello-id')
-        file_conflict = conflicts.TextConflict('file', None, 'hello-id')
+        file_conflict = conflicts.TextConflict('file', 'hello-id')
         tree.set_conflicts(conflicts.ConflictList([file_conflict]))
         tree.auto_resolve()
 

=== modified file 'bzrlib/transform.py'
--- bzrlib/transform.py	2010-07-21 09:15:47 +0000
+++ bzrlib/transform.py	2010-11-07 10:22:42 +0000
@@ -645,9 +645,14 @@
         if (self._new_name, self._new_parent) == ({}, {}):
             return conflicts
         for children in by_parent.itervalues():
-            name_ids = [(self.final_name(t), t) for t in children]
-            if not self._case_sensitive_target:
-                name_ids = [(n.lower(), t) for n, t in name_ids]
+            name_ids = []
+            for child_tid in children:
+                name = self.final_name(child_tid)
+                if name is not None:
+                    # Keep children only if they still exist in the end
+                    if not self._case_sensitive_target:
+                        name = name.lower()
+                    name_ids.append((name, child_tid))
             name_ids.sort()
             last_name = None
             last_trans_id = None