Code review comment for lp:~vila/bzr/646961-attr-error

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.