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

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.

>>>>> 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 !

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).

sent to pqm by email

sent to pqm by email

sent to pqm by email