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

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