>>>>> 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.
>>>>> 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: conflict_ path', self.conflict_path)
> s.add('
> ?
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' ntlyIgnore( script. TestCaseWithTra nsportAndScript ): 646961( self):
> [...]
>> +class TestResolveSile
>> +
>> + def test_bug_
> [...]
>> + # 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 !