You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2013/03/15 23:03:51 UTC
[PATCH] Fix issue #4316 - Merge errors out after resolving conflicts
The attached patch fixes issue #4316 'Merge errors out after resolving conflicts'.
I have a particular question about the notifications printed after applying this patch -- see the end of this mail.
The patch makes the libsvn_client merge function call the resolver for all conflicts raised while merging one revision range, and then continue with the merge of the next revision range if all the conflicts were resolved.
In a previous attempt I started to make 'svn' repeat the merge after resolving any conflicts, if the merge had bailed out early because of conflicts. Brane suggested that made for a poor merge API if the caller has to be prepared to figure out how much of the merging was done and how much more needs to be done and to call the merge again itself. Better for the merge API to do everything itself.
Of course the caller still has the possibility of *not* resolving conflicts, and in that case the merge is still going to bail out. But at least we have a good solution for the case where the user *does* resolve all the conflicts.
NOTIFICATIONS CHANGED
As I mentioned in my "Conflict resolver callback design" email, doing this does mean that the notification receiver will get a 'C' (conflict) notification for every conflict even if that conflict is going to be resolved to a pre-determined choice. In terms of the 'svn' client and the 'svn merge' command, this means that 'svn merge --accept=[mine-full, etc.]' will, if we don't take further action, print something like in this example:
[[[
--- Merging r3 through r4 into 'merge_tests-135/A2/mu':
C merge_tests-135/A2/mu
--- Recording mergeinfo for merge of r3 through r4 into 'merge_tests-135/A2/mu':
G merge_tests-135/A2/mu
Resolved conflicted state of 'merge_tests-135/A2/mu'
--- Merging r6 through r8 into 'merge_tests-135/A2/mu':
U merge_tests-135/A2/mu
--- Merging r10 through r11 into 'merge_tests-135/A2/mu':
U merge_tests-135/A2/mu
--- Recording mergeinfo for merge of r5 through r11 into 'merge_tests-135/A2/mu':
G merge_tests-135/A2/mu
Summary of conflicts:
Property conflicts: 1
]]]
I think if this was changed to print a slightly different summary, something like...
[[[
Summary of conflicts:
Property conflicts: 0 (and 1 already resolved)
]]]
... then it would be fine. I don't see that the interleaved 'Resolved ...' line is a problem.
Do others agree?
If so, I'll make it so. I would make the logic something like, for each kind of conflicts: if (number-remaining > 0 || number-already-resolved > 0) { print 'Property conflics: <number-remaining>'; if (already-resolved > 0) print ' (and <number-already-resolved> already resolved)'; }.
I notice that if 'svn update' or 'switch' is invoked with
'--accept=[mine-conflict, etc.]' then currently it produces first the
general update notifications including any 'C' status for conflicts,
then a 'Summary of conflicts', and after that it proceeds to resolve the conflicts and prints 'Resolved ...' for each one. I think (again as mentioned in that other email) it might make sense to move the calling of the resolver into the libsvn_client functions.
Thoughts? Any review of the patch is welcome too.
- Julian
--
Certified & Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download
Re: [PATCH] Fix issue #4316 - Merge errors out after resolving conflicts
Posted by Paul Burba <pt...@gmail.com>.
On Thu, Mar 21, 2013 at 1:40 PM, Julian Foad <ju...@btopenworld.com> wrote:
> Paul Burba wrote:
>
>> On Wed, Mar 20, 2013 at 4:15 PM, Julian Foad wrote:
>>>> I have committed a complete fix, with the Summary of Conflicts as
>>>> discussed here, in <http://svn.apache.org/r1459012>.
>>>
>>> Remaining issues:
>>>
>>> * There is an inconsistency with what rev ranges get recorded as merged,
>>> in a file merge vs. a dir merge -- see the comment and work-around in
>>> merge_tests.py 138.
>>
>> Hi Julian,
>>
>> I see the inconsistency, but I don't believe there is a bug (assuming
>> we are talking about the same thing -- if not please point me in the
>> right direction).
>
> Thanks for investigating, Paul. This is what I'm talking about:
>
> # ### Bug? The mergeinfo ranges recorded by the merge differ
> # slightly for a single-file merge vs. a dir merge. That's
> # incidental to the purpose of this test, so we work around it
> # with "target == file and 'X' or 'Y'".
> expect = expected_out_and_err(tgt_ospath,
> target == file and '3-4,5-11' or '3-4,6-11',
> ['3-4', '6-8,10-11'],
> prop_resolved=1, expect_error=False)
> try_merge(target, 4, [], expect, '3-11')
>
> When the target is a file, the mergeinfo notification says it's recording revisions 3-4 and then 5-11; with a dir merge, it says it's recording revisions 3-4 and then 6-11, *excluding* r5.
>
> I don't see the difference in recording r2, that you are showing me. Could that be an artifact of you running the test in a slightly different manner?
>
> I have just (r1459410) tweaked the test initialization (for this and several other tests) to change directory to the WC before starting, so that most of the local paths it prints are short and no longer start with 'svn-test-work/working_copies/merge_tests-138/', to make this kind of inspection easier.
>
> For the file (pasting from the test run verbose output):
> [[[
> I: ...svn merge '^/A/mu@11' A2/mu --accept mine-full --config-dir...
> I: --- Merging r3 through r4 into 'A2/mu':
> I: C A2/mu
> I: --- Recording mergeinfo for merge of r3 through r4 into 'A2/mu':
> I: G A2/mu
> I: Resolved conflicted state of 'A2/mu'
> I: --- Merging r6 through r8 into 'A2/mu':
> I: U A2/mu
> I: --- Merging r10 through r11 into 'A2/mu':
> I: U A2/mu
> I: --- Recording mergeinfo for merge of r5 through r11 into 'A2/mu':
> I: G A2/mu
> I: Summary of conflicts:
> I: Property conflicts: 0 remaining (and 1 already resolved)
> ]]]
>
> For the dir:
> [[[
> I: ...svn merge '^/A/B@11' A2/B --accept mine-full --config-dir...
> I: --- Merging r3 through r4 into 'A2/B':
> I: C A2/B
> I: --- Recording mergeinfo for merge of r3 through r4 into 'A2/B':
> I: G A2/B
> I: Resolved conflicted state of 'A2/B'
> I: --- Merging r6 through r8 into 'A2/B':
> I: U A2/B
> I: --- Merging r10 through r11 into 'A2/B':
> I: U A2/B
> I: --- Recording mergeinfo for merge of r6 through r11 into 'A2/B':
> I: G A2/B
> I: Summary of conflicts:
> I: Property conflicts: 0 remaining (and 1 already resolved)
> ]]]
Ok, I initially misunderstood the comment. r1459895 fixes this.
--
Paul T. Burba
CollabNet, Inc. -- www.collab.net -- Enterprise Cloud Development
Skype: ptburba
Re: [PATCH] Fix issue #4316 - Merge errors out after resolving conflicts
Posted by Julian Foad <ju...@btopenworld.com>.
Paul Burba wrote:
> On Wed, Mar 20, 2013 at 4:15 PM, Julian Foad wrote:
>>> I have committed a complete fix, with the Summary of Conflicts as
>>> discussed here, in <http://svn.apache.org/r1459012>.
>>
>> Remaining issues:
>>
>> * There is an inconsistency with what rev ranges get recorded as merged,
>> in a file merge vs. a dir merge -- see the comment and work-around in
>> merge_tests.py 138.
>
> Hi Julian,
>
> I see the inconsistency, but I don't believe there is a bug (assuming
> we are talking about the same thing -- if not please point me in the
> right direction).
Thanks for investigating, Paul. This is what I'm talking about:
# ### Bug? The mergeinfo ranges recorded by the merge differ
# slightly for a single-file merge vs. a dir merge. That's
# incidental to the purpose of this test, so we work around it
# with "target == file and 'X' or 'Y'".
expect = expected_out_and_err(tgt_ospath,
target == file and '3-4,5-11' or '3-4,6-11',
['3-4', '6-8,10-11'],
prop_resolved=1, expect_error=False)
try_merge(target, 4, [], expect, '3-11')
When the target is a file, the mergeinfo notification says it's recording revisions 3-4 and then 5-11; with a dir merge, it says it's recording revisions 3-4 and then 6-11, *excluding* r5.
I don't see the difference in recording r2, that you are showing me. Could that be an artifact of you running the test in a slightly different manner?
I have just (r1459410) tweaked the test initialization (for this and several other tests) to change directory to the WC before starting, so that most of the local paths it prints are short and no longer start with 'svn-test-work/working_copies/merge_tests-138/', to make this kind of inspection easier.
For the file (pasting from the test run verbose output):
[[[
I: ...svn merge '^/A/mu@11' A2/mu --accept mine-full --config-dir...
I: --- Merging r3 through r4 into 'A2/mu':
I: C A2/mu
I: --- Recording mergeinfo for merge of r3 through r4 into 'A2/mu':
I: G A2/mu
I: Resolved conflicted state of 'A2/mu'
I: --- Merging r6 through r8 into 'A2/mu':
I: U A2/mu
I: --- Merging r10 through r11 into 'A2/mu':
I: U A2/mu
I: --- Recording mergeinfo for merge of r5 through r11 into 'A2/mu':
I: G A2/mu
I: Summary of conflicts:
I: Property conflicts: 0 remaining (and 1 already resolved)
]]]
For the dir:
[[[
I: ...svn merge '^/A/B@11' A2/B --accept mine-full --config-dir...
I: --- Merging r3 through r4 into 'A2/B':
I: C A2/B
I: --- Recording mergeinfo for merge of r3 through r4 into 'A2/B':
I: G A2/B
I: Resolved conflicted state of 'A2/B'
I: --- Merging r6 through r8 into 'A2/B':
I: U A2/B
I: --- Merging r10 through r11 into 'A2/B':
I: U A2/B
I: --- Recording mergeinfo for merge of r6 through r11 into 'A2/B':
I: G A2/B
I: Summary of conflicts:
I: Property conflicts: 0 remaining (and 1 already resolved)
]]]
> Looking at the spot in the test where your comment
> is, the test currently does this merge:
[...]
> 1.8.0-dev>svn.exe merge ^^/A/mu@11 A2\mu --accept mine-full
> --- Merging r3 through r4 into 'A2\mu':
> C A2\mu
> --- Recording mergeinfo for merge of r3 through r4 into 'A2\mu':
> G A2\mu
> Resolved conflicted state of 'A2\mu'
> --- Merging r6 through r8 into 'A2\mu':
> U A2\mu
> --- Merging r10 through r11 into 'A2\mu':
> U A2\mu
> --- Recording mergeinfo for merge of r5 through r11 into 'A2\mu':
> G A2\mu
> Summary of conflicts:
> Property conflicts: 0 remaining (and 1 already resolved)
>
> The gaps in the mergeinfo are all filled:
>
> 1.8.0-dev>svn pg svn:mergeinfo -vR A2
> Properties on 'A2':
> svn:mergeinfo
> /A:5,9
> Properties on 'A2\mu':
> svn:mergeinfo
> /A/mu:3-11
Right, but in the second half of the merge it said it was recording mergeinfo for r5, whereas for the merge from ^/A/B to A2/B, it doesn't.
> If we instead try the above merge as a directory merge targeting
> A2/mu's parent, the merge starts with r2:
I'm not sure this is relevant, since the dir merges that the test tries are all merges of the 'B' subdirectory, not of the 'A'/'A2' parent directory.
[...]
> r3 is explained by the fact that your test makes a copy of a
> mixed-revision WC to create the branch in r3:
>
>
> 1.8.0-dev>svn st -v
> 1 1 jrandom .
> 1 1 jrandom A
> 2 2 jrandom A\B
> 1 1 jrandom A\B\E
> 1 1 jrandom A\B\E\alpha
> 1 1 jrandom A\B\E\beta
> 1 1 jrandom A\B\F
> 1 1 jrandom A\B\lambda
> 1 1 jrandom A\C
> 1 1 jrandom A\D
> 1 1 jrandom A\D\G
> 1 1 jrandom A\D\G\pi
> 1 1 jrandom A\D\G\rho
> 1 1 jrandom A\D\G\tau
> 1 1 jrandom A\D\H
> 1 1 jrandom A\D\H\chi
> 1 1 jrandom A\D\H\omega
> 1 1 jrandom A\D\H\psi
> 1 1 jrandom A\D\gamma
> 2 2 jrandom A\mu
> 1 1 jrandom iota
>
> 1.8.0-dev>svn copy A A2
> A A2
>
> 1.8.0-dev>svn ci -m ""
> Adding A2
> Adding A2\B
> Adding A2\B\E
> Adding A2\B\F
> Adding A2\B\lambda
> Adding A2\mu
>
> Committed revision 3.
>
> 1.8.0-dev>svn log -v -r3
> ------------------------------------------------------------------------
> r3 | pburba | 2013-03-21 11:19:39 -0400 (Thu, 21 Mar 2013) | 1 line
> Changed paths:
> A /A2 (from /A:1)
> R /A2/B (from /A/B:2)
> R /A2/mu (from /A/mu:2)
>
> Not sure if that was an oversight or purposeful.
An oversight, but should be irrelevant. I'll tidy it up by doing an "svn up" before the copy (committed, r1459419). It makes no difference to the r5 anomaly that I'm demonstrating.
- Julian
Re: [PATCH] Fix issue #4316 - Merge errors out after resolving conflicts
Posted by Paul Burba <pt...@gmail.com>.
On Wed, Mar 20, 2013 at 4:15 PM, Julian Foad <ju...@btopenworld.com> wrote:
>
> --
> Certified & Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download
> ----- Original Message -----
>> From: Julian Foad <ju...@btopenworld.com>
>> To: C. Michael Pilato <cm...@collab.net>
>> Cc: Subversion Development <de...@subversion.apache.org>
>> Sent: Wednesday, 20 March 2013, 15:38
>> Subject: Re: [PATCH] Fix issue #4316 - Merge errors out after resolving conflicts
>>
>> C. Michael Pilato wrote:
>>> On 03/15/2013 06:03 PM, Julian Foad wrote:
>>>> NOTIFICATIONS CHANGED
>>>>
>>>> As I mentioned in my "Conflict resolver callback design"
>> email,
>>>> doing this does mean that the notification receiver will get a
>> 'C'
>>>> (conflict) notification for every conflict even if that conflict is
>> going to be
>>>> resolved to a pre-determined choice. In terms of the 'svn'
>> client and
>>>> the 'svn merge' command, this means that 'svn merge
>>>> --accept=[mine-full, etc.]' will, if we don't take further
>> action, print
>>>> something like in this example:
>>>>
>>>> [[[
>>>> --- Merging r3 through r4 into 'merge_tests-135/A2/mu':
>>>> C merge_tests-135/A2/mu
>>>> --- Recording mergeinfo for merge of r3 through r4 into
>>>> 'merge_tests-135/A2/mu':
>>>> G merge_tests-135/A2/mu
>>>> Resolved conflicted state of 'merge_tests-135/A2/mu'
>>>> --- Merging r6 through r8 into 'merge_tests-135/A2/mu':
>>>> U merge_tests-135/A2/mu
>>>> --- Merging r10 through r11 into 'merge_tests-135/A2/mu':
>>>> U merge_tests-135/A2/mu
>>>> --- Recording mergeinfo for merge of r5 through r11 into
>>>> 'merge_tests-135/A2/mu':
>>>> G merge_tests-135/A2/mu
>>>> Summary of conflicts:
>>>> Property conflicts: 1
>>>> ]]]
>>>>
>>>> I think if this was changed to print a slightly different summary,
>>>> something like...
>>>>
>>>> [[[
>>>> Summary of conflicts:
>>>> Property conflicts: 0 (and 1 already resolved)
>>>> ]]]
>>>>
>>>> ... then it would be fine. I don't see that the interleaved
>>>> 'Resolved ...' line is a problem.
>>>>
>>>> Do others agree?
>>>
>>> The point of the summary section is to draw attention to details that might
>>> have whizzed by the screen. Given that, I agree it's a bit misleading
>> to
>>> alert the user to a problem which may not really be a problem any longer.
>>> So yeah, a change such as what you've suggested makes sense to me.
>>>
>>> (Sorry, no feedback on your actual patch.)
>>
>> I have committed a complete fix, with the Summary of Conflicts as discussed
>> here, in <http://svn.apache.org/r1459012>.
>
> Remaining issues:
>
> * There is an inconsistency with what rev ranges get recorded as merged, in a file merge vs. a dir merge -- see the comment and work-around in merge_tests.py 138.
Hi Julian,
I see the inconsistency, but I don't believe there is a bug (assuming
we are talking about the same thing -- if not please point me in the
right direction). Looking at the spot in the test where your comment
is, the test currently does this merge:
Uniform revision:
1.8.0-dev>svn up
Updating '.':
At revision 11.
Local prop mod that will conflict with the coming merge:
1.8.0-dev>svn st
M A2\mu
1.8.0-dev>svn diff
Index: A2/mu
===================================================================
--- A2/mu (revision 11)
+++ A2/mu (working copy)
Property changes on: A2/mu
___________________________________________________________________
Modified: prop-4
## -1 +1 ##
-Old pval 4
\ No newline at end of property
+Conflict pval 4
\ No newline at end of property
Fragmented mergeinfo inherited by the target...
1.8.0-dev>svn pg svn:mergeinfo -vR A2
Properties on 'A2':
svn:mergeinfo
/A:5,9
...breaks the merge into multiple editor drives, starting with r3:
1.8.0-dev>svn.exe merge ^^/A/mu@11 A2\mu --accept mine-full
--- Merging r3 through r4 into 'A2\mu':
C A2\mu
--- Recording mergeinfo for merge of r3 through r4 into 'A2\mu':
G A2\mu
Resolved conflicted state of 'A2\mu'
--- Merging r6 through r8 into 'A2\mu':
U A2\mu
--- Merging r10 through r11 into 'A2\mu':
U A2\mu
--- Recording mergeinfo for merge of r5 through r11 into 'A2\mu':
G A2\mu
Summary of conflicts:
Property conflicts: 0 remaining (and 1 already resolved)
The gaps in the mergeinfo are all filled:
1.8.0-dev>svn pg svn:mergeinfo -vR A2
Properties on 'A2':
svn:mergeinfo
/A:5,9
Properties on 'A2\mu':
svn:mergeinfo
/A/mu:3-11
If we instead try the above merge as a directory merge targeting
A2/mu's parent, the merge starts with r2:
1.8.0-dev>svn.exe merge ^^/A@11 A2 --accept mine-full
--- Merging r2 through r4 into 'A2':
C A2\B
C A2\mu
--- Recording mergeinfo for merge of r2 through r4 into 'A2':
U A2
Resolved conflicted state of 'A2\B'
Resolved conflicted state of 'A2\mu'
--- Merging r6 through r8 into 'A2':
U A2\B
U A2\mu
--- Merging r10 through r11 into 'A2':
U A2\B
U A2\mu
--- Recording mergeinfo for merge of r6 through r11 into 'A2':
G A2
Summary of conflicts:
Property conflicts: 0 remaining (and 2 already resolved)
1.8.0-dev>svn st
M A2
M A2\B
M A2\mu
And the recorded mergeinfo also starts with r2:
1.8.0-dev>svn pg svn:mergeinfo -vR A2
Properties on 'A2':
svn:mergeinfo
/A:2-11
If that is the inconsistency you mean? I don't think that's a
problem. Let's look at the history of ^/A2 vs ^/A2/mu:
1.8.0-dev>svn log -v -r1:HEAD ^^/A2
------------------------------------------------------------------------
r1 | jrandom | 2013-03-21 10:16:18 -0400 (Thu, 21 Mar 2013) | 1 line
Changed paths:
A /A
A /A/B
A /A/B/E
A /A/B/E/alpha
A /A/B/E/beta
A /A/B/F
A /A/B/lambda
A /A/C
A /A/D
A /A/D/G
A /A/D/G/pi
A /A/D/G/rho
A /A/D/G/tau
A /A/D/H
A /A/D/H/chi
A /A/D/H/omega
A /A/D/H/psi
A /A/D/gamma
A /A/mu
A /iota
Log message for revision 1.
------------------------------------------------------------------------
r3 | jrandom | 2013-03-21 10:16:26 -0400 (Thu, 21 Mar 2013) | 1 line
Changed paths:
A /A2 (from /A:1)
R /A2/B (from /A/B:2)
R /A2/mu (from /A/mu:2)
File 'C:\SVN\src-trunk-3\\subversion\tests\cmdline\merge_tests.py',
line 18796, in conflicted_split_merge_with_resolve
------------------------------------------------------------------------
r11 | jrandom | 2013-03-21 10:16:27 -0400 (Thu, 21 Mar 2013) | 1 line
Changed paths:
M /A2
M /A2/B
M /A2/mu
File 'C:\SVN\src-trunk-3\\subversion\tests\cmdline\merge_tests.py',
line 18814, in conflicted_split_merge_with_resolve
------------------------------------------------------------------------
Since ^/A2/mu was replaced with a copy of ^/A/mu@2 wouldn't we expect
a merge targeting the former (since ^/A2/mu@2 is the yca of the two
branches) to merge -r2:HEAD? Likewise, if we target /A2, the yca is
older, ^/A@1, so the merge is -r1:HEAD. This explains the mergeinfo
differences between the file and dir merge.
~~~~~
r3 is explained by the fact that your test makes a copy of a
mixed-revision WC to create the branch in r3:
1.8.0-dev>svn st -v
1 1 jrandom .
1 1 jrandom A
2 2 jrandom A\B
1 1 jrandom A\B\E
1 1 jrandom A\B\E\alpha
1 1 jrandom A\B\E\beta
1 1 jrandom A\B\F
1 1 jrandom A\B\lambda
1 1 jrandom A\C
1 1 jrandom A\D
1 1 jrandom A\D\G
1 1 jrandom A\D\G\pi
1 1 jrandom A\D\G\rho
1 1 jrandom A\D\G\tau
1 1 jrandom A\D\H
1 1 jrandom A\D\H\chi
1 1 jrandom A\D\H\omega
1 1 jrandom A\D\H\psi
1 1 jrandom A\D\gamma
2 2 jrandom A\mu
1 1 jrandom iota
1.8.0-dev>svn copy A A2
A A2
1.8.0-dev>svn ci -m ""
Adding A2
Adding A2\B
Adding A2\B\E
Adding A2\B\F
Adding A2\B\lambda
Adding A2\mu
Committed revision 3.
1.8.0-dev>svn log -v -r3
------------------------------------------------------------------------
r3 | pburba | 2013-03-21 11:19:39 -0400 (Thu, 21 Mar 2013) | 1 line
Changed paths:
A /A2 (from /A:1)
R /A2/B (from /A/B:2)
R /A2/mu (from /A/mu:2)
------------------------------------------------------------------------
Not sure if that was an oversight or purposeful.
--
Paul T. Burba
CollabNet, Inc. -- www.collab.net -- Enterprise Cloud Development
Skype: ptburba
Re: [PATCH] Fix issue #4316 - Merge errors out after resolving conflicts
Posted by Julian Foad <ju...@btopenworld.com>.
--
Certified & Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download
----- Original Message -----
> From: Julian Foad <ju...@btopenworld.com>
> To: C. Michael Pilato <cm...@collab.net>
> Cc: Subversion Development <de...@subversion.apache.org>
> Sent: Wednesday, 20 March 2013, 15:38
> Subject: Re: [PATCH] Fix issue #4316 - Merge errors out after resolving conflicts
>
> C. Michael Pilato wrote:
>> On 03/15/2013 06:03 PM, Julian Foad wrote:
>>> NOTIFICATIONS CHANGED
>>>
>>> As I mentioned in my "Conflict resolver callback design"
> email,
>>> doing this does mean that the notification receiver will get a
> 'C'
>>> (conflict) notification for every conflict even if that conflict is
> going to be
>>> resolved to a pre-determined choice. In terms of the 'svn'
> client and
>>> the 'svn merge' command, this means that 'svn merge
>>> --accept=[mine-full, etc.]' will, if we don't take further
> action, print
>>> something like in this example:
>>>
>>> [[[
>>> --- Merging r3 through r4 into 'merge_tests-135/A2/mu':
>>> C merge_tests-135/A2/mu
>>> --- Recording mergeinfo for merge of r3 through r4 into
>>> 'merge_tests-135/A2/mu':
>>> G merge_tests-135/A2/mu
>>> Resolved conflicted state of 'merge_tests-135/A2/mu'
>>> --- Merging r6 through r8 into 'merge_tests-135/A2/mu':
>>> U merge_tests-135/A2/mu
>>> --- Merging r10 through r11 into 'merge_tests-135/A2/mu':
>>> U merge_tests-135/A2/mu
>>> --- Recording mergeinfo for merge of r5 through r11 into
>>> 'merge_tests-135/A2/mu':
>>> G merge_tests-135/A2/mu
>>> Summary of conflicts:
>>> Property conflicts: 1
>>> ]]]
>>>
>>> I think if this was changed to print a slightly different summary,
>>> something like...
>>>
>>> [[[
>>> Summary of conflicts:
>>> Property conflicts: 0 (and 1 already resolved)
>>> ]]]
>>>
>>> ... then it would be fine. I don't see that the interleaved
>>> 'Resolved ...' line is a problem.
>>>
>>> Do others agree?
>>
>> The point of the summary section is to draw attention to details that might
>> have whizzed by the screen. Given that, I agree it's a bit misleading
> to
>> alert the user to a problem which may not really be a problem any longer.
>> So yeah, a change such as what you've suggested makes sense to me.
>>
>> (Sorry, no feedback on your actual patch.)
>
> I have committed a complete fix, with the Summary of Conflicts as discussed
> here, in <http://svn.apache.org/r1459012>.
Remaining issues:
* There is an inconsistency with what rev ranges get recorded as merged, in a file merge vs. a dir merge -- see the comment and work-around in merge_tests.py 138.
* Tweak the Summary of Conflicts to use the extended "%d remaining (and %d already resolved)" format for all three kinds of conflict iff it uses it for any kind. That would look better and be more plainly understandable, in a realistic merge where there will often be more than one kind of conflict raised.
* Update and switch should work the same way.
- Julian
Re: [PATCH] Fix issue #4316 - Merge errors out after resolving conflicts
Posted by Julian Foad <ju...@btopenworld.com>.
C. Michael Pilato wrote:
> On 03/15/2013 06:03 PM, Julian Foad wrote:
>> NOTIFICATIONS CHANGED
>>
>> As I mentioned in my "Conflict resolver callback design" email,
>> doing this does mean that the notification receiver will get a 'C'
>> (conflict) notification for every conflict even if that conflict is going to be
>> resolved to a pre-determined choice. In terms of the 'svn' client and
>> the 'svn merge' command, this means that 'svn merge
>> --accept=[mine-full, etc.]' will, if we don't take further action, print
>> something like in this example:
>>
>> [[[
>> --- Merging r3 through r4 into 'merge_tests-135/A2/mu':
>> C merge_tests-135/A2/mu
>> --- Recording mergeinfo for merge of r3 through r4 into
>> 'merge_tests-135/A2/mu':
>> G merge_tests-135/A2/mu
>> Resolved conflicted state of 'merge_tests-135/A2/mu'
>> --- Merging r6 through r8 into 'merge_tests-135/A2/mu':
>> U merge_tests-135/A2/mu
>> --- Merging r10 through r11 into 'merge_tests-135/A2/mu':
>> U merge_tests-135/A2/mu
>> --- Recording mergeinfo for merge of r5 through r11 into
>> 'merge_tests-135/A2/mu':
>> G merge_tests-135/A2/mu
>> Summary of conflicts:
>> Property conflicts: 1
>> ]]]
>>
>> I think if this was changed to print a slightly different summary,
>> something like...
>>
>> [[[
>> Summary of conflicts:
>> Property conflicts: 0 (and 1 already resolved)
>> ]]]
>>
>> ... then it would be fine. I don't see that the interleaved
>> 'Resolved ...' line is a problem.
>>
>> Do others agree?
>
> The point of the summary section is to draw attention to details that might
> have whizzed by the screen. Given that, I agree it's a bit misleading to
> alert the user to a problem which may not really be a problem any longer.
> So yeah, a change such as what you've suggested makes sense to me.
>
> (Sorry, no feedback on your actual patch.)
I have committed a complete fix, with the Summary of Conflicts as discussed here, in <http://svn.apache.org/r1459012>.
- Julian
Re: [PATCH] Fix issue #4316 - Merge errors out after resolving conflicts
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 03/15/2013 06:03 PM, Julian Foad wrote:
> NOTIFICATIONS CHANGED
>
> As I mentioned in my "Conflict resolver callback design" email, doing this does mean that the notification receiver will get a 'C' (conflict) notification for every conflict even if that conflict is going to be resolved to a pre-determined choice. In terms of the 'svn' client and the 'svn merge' command, this means that 'svn merge --accept=[mine-full, etc.]' will, if we don't take further action, print something like in this example:
>
> [[[
> --- Merging r3 through r4 into 'merge_tests-135/A2/mu':
> C merge_tests-135/A2/mu
> --- Recording mergeinfo for merge of r3 through r4 into 'merge_tests-135/A2/mu':
> G merge_tests-135/A2/mu
> Resolved conflicted state of 'merge_tests-135/A2/mu'
> --- Merging r6 through r8 into 'merge_tests-135/A2/mu':
> U merge_tests-135/A2/mu
> --- Merging r10 through r11 into 'merge_tests-135/A2/mu':
> U merge_tests-135/A2/mu
> --- Recording mergeinfo for merge of r5 through r11 into 'merge_tests-135/A2/mu':
> G merge_tests-135/A2/mu
> Summary of conflicts:
> Property conflicts: 1
> ]]]
>
> I think if this was changed to print a slightly different summary, something like...
>
> [[[
> Summary of conflicts:
> Property conflicts: 0 (and 1 already resolved)
> ]]]
>
> ... then it would be fine. I don't see that the interleaved 'Resolved ...' line is a problem.
>
> Do others agree?
The point of the summary section is to draw attention to details that might
have whizzed by the screen. Given that, I agree it's a bit misleading to
alert the user to a problem which may not really be a problem any longer.
So yeah, a change such as what you've suggested makes sense to me.
(Sorry, no feedback on your actual patch.)
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development