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 2012/09/14 00:55:30 UTC
Please review Symmetric Merge
Hi, merge fans and QA fans.
As you've seen, the 'symmetric merge' code is live in trunk and destined to be in 1.8.
The principle is nice; however, the current implementation is a rather ugly hack, being just a layer on top of the existing 'sync' and 'reintegrate' merge code.
Please review it and send me all your criticism and suggestions. Seriously.
I intend to do some self-review as well, but there's nothing like one of you fellow developers looking at it to motivate me to fix it :-)
Thanks.
- Julian
Re: Please review Symmetric Merge
Posted by Julian Foad <ju...@btopenworld.com>.
Paul Burba wrote:
> On Fri, Nov 2, 2012 at 5:29 PM, Paul Burba <pt...@gmail.com> wrote:
>> What about all these two XFailing tests?
>>
>> merge_automatic_tests.py 16 'cherry2_fwd'
>> merge_automatic_tests.py 17 'cherry3_fwd
>>
>> Are you working on those? Are they blockers for 1.8? Any existing
>> issue they can be associated with?
>
> Julian - just pinging you about this. These two tests are among a
> handful left that are set to XFail but which aren't associated with
> any known issue. Having an associated issue allows us to see at a
> glance what XFailing tests are 1.8 blockers as we plod on towards a
> release.
Yup, thanks for the ping. Still got this and one other similar email from you marked to respond to this week. I'll create an issue for this one.
- Julian
Re: Please review Symmetric Merge
Posted by Paul Burba <pt...@gmail.com>.
On Thu, Nov 8, 2012 at 9:49 AM, Julian Foad <ju...@btopenworld.com> wrote:
> Paul Burba wrote:
>
>> On Fri, Nov 2, 2012 at 5:29 PM, Paul Burba wrote:
>>> What about all these two XFailing tests?
>>>
>>> merge_automatic_tests.py 16 'cherry2_fwd'
>>> merge_automatic_tests.py 17 'cherry3_fwd
>>>
>>> Are you working on those? Are they blockers for 1.8? Any existing
>>> issue they can be associated with?
>>
>> Julian - just pinging you about this. These two tests are among a
>> handful left that are set to XFail but which aren't associated with
>> any known issue. Having an associated issue allows us to see at a
>> glance what XFailing tests are 1.8 blockers as we plod on towards a
>> release.
>
> OK, I've associated them with new issue #4255, "Automatic merge ignores some cherry-picks, causing conflicts", and left them as XFail. (And I'm not working on fixing them, since it's a long-standing deficiency, not a regression.)
Great, thanks Julian. I set the target milestone on the issue to
unscheduled, since this isn't a 1.8 blocker (all those issues in the
tracker with no target milestones give me a rash since it makes it
hard to determine what needs to be fixed before 1.8 :-)
--
Paul T. Burba
CollabNet, Inc. -- www.collab.net -- Enterprise Cloud Development
Skype: ptburba
Re: Please review Symmetric Merge
Posted by Julian Foad <ju...@btopenworld.com>.
Paul Burba wrote:
> On Fri, Nov 2, 2012 at 5:29 PM, Paul Burba wrote:
>> What about all these two XFailing tests?
>>
>> merge_automatic_tests.py 16 'cherry2_fwd'
>> merge_automatic_tests.py 17 'cherry3_fwd
>>
>> Are you working on those? Are they blockers for 1.8? Any existing
>> issue they can be associated with?
>
> Julian - just pinging you about this. These two tests are among a
> handful left that are set to XFail but which aren't associated with
> any known issue. Having an associated issue allows us to see at a
> glance what XFailing tests are 1.8 blockers as we plod on towards a
> release.
OK, I've associated them with new issue #4255, "Automatic merge ignores some cherry-picks, causing conflicts", and left them as XFail. (And I'm not working on fixing them, since it's a long-standing deficiency, not a regression.)
- Julian
Re: Please review Symmetric Merge
Posted by Paul Burba <pt...@gmail.com>.
On Fri, Nov 2, 2012 at 5:29 PM, Paul Burba <pt...@gmail.com> wrote:
> On Thu, Sep 13, 2012 at 6:55 PM, Julian Foad <ju...@btopenworld.com> wrote:
>> Hi, merge fans and QA fans.
>>
>> As you've seen, the 'symmetric merge' code is live in trunk and destined to be in 1.8.
>>
>> The principle is nice; however, the current implementation is a rather ugly hack, being just a layer on top of the existing 'sync' and 'reintegrate' merge code.
>>
>> Please review it and send me all your criticism and suggestions. Seriously.
>
> Hi Julian,
>
> What about all these two XFailing tests?
>
> merge_automatic_tests.py 16 'cherry2_fwd'
> merge_automatic_tests.py 17 'cherry3_fwd
>
> Are you working on those? Are they blockers for 1.8? Any existing
> issue they can be associated with?
Julian - just pinging you about this. These two tests are among a
handful left that are set to XFail but which aren't associated with
any known issue. Having an associated issue allows us to see at a
glance what XFailing tests are 1.8 blockers as we plod on towards a
release.
> --
> Paul T. Burba
> CollabNet, Inc. -- www.collab.net -- Enterprise Cloud Development
> Skype: ptburba
>
>> I intend to do some self-review as well, but there's nothing like one of you fellow developers looking at it to motivate me to fix it :-)
>>
>> Thanks.
>>
>> - Julian
>>
--
Paul T. Burba
CollabNet, Inc. -- www.collab.net -- Enterprise Cloud Development
Skype: ptburba
Re: Please review Symmetric Merge
Posted by Paul Burba <pt...@gmail.com>.
On Thu, Sep 13, 2012 at 6:55 PM, Julian Foad <ju...@btopenworld.com> wrote:
> Hi, merge fans and QA fans.
>
> As you've seen, the 'symmetric merge' code is live in trunk and destined to be in 1.8.
>
> The principle is nice; however, the current implementation is a rather ugly hack, being just a layer on top of the existing 'sync' and 'reintegrate' merge code.
>
> Please review it and send me all your criticism and suggestions. Seriously.
Hi Julian,
What about all these two XFailing tests?
merge_automatic_tests.py 16 'cherry2_fwd'
merge_automatic_tests.py 17 'cherry3_fwd
Are you working on those? Are they blockers for 1.8? Any existing
issue they can be associated with?
--
Paul T. Burba
CollabNet, Inc. -- www.collab.net -- Enterprise Cloud Development
Skype: ptburba
> I intend to do some self-review as well, but there's nothing like one of you fellow developers looking at it to motivate me to fix it :-)
>
> Thanks.
>
> - Julian
>
Re: Please review Symmetric Merge
Posted by Julian Foad <ju...@btopenworld.com>.
C. Michael Pilato wrote:
> On 09/13/2012 06:55 PM, Julian Foad wrote:
>> Hi, merge fans and QA fans.
>>
>> As you've seen, the 'symmetric merge' code is live in trunk and
>> destined to be in 1.8.
>>
>> The principle is nice; however, the current implementation is a rather
>> ugly hack, being just a layer on top of the existing 'sync' and
>> 'reintegrate' merge code.
>>
>> Please review it and send me all your criticism and suggestions.
>> Seriously.
>>
>> I intend to do some self-review as well, but there's nothing like one
>> of you fellow developers looking at it to motivate me to fix it :-)
>
> Julian, can you give any pointers on reviewing this work? Are we talking
> about a straight code read? Perhaps there's a good pair of trunk revision
> the differences between which carry the lion's share of your changes? May I
> assume that all the pertinent details remain in libsvn_client/merge.c?
Hi Mike. Thanks for asking.
First, yes all the relevant code is in merge.c, mostly at and near the end, apart from the high-level caller in subversion/trunk/merge-cmd.c.
A code read is certainly one useful approach. Others are:
Reviewing the UI and the help text ('svn help merge') to see if it is understandable, addresses the real needs and doesn't break other kinds of merge scenarios that I might have overlooked (such as merge ignoring mergeinfo, merge from foreign-repo, cherry-picks, etc.).
Pointing out that The Book will need an update.
Looking for inconsistencies with other commands, inconsistencies in terminology, references to the --reintegrate option that should now be gone.
(I think we should call this the 'automatic' merge, NOT 'symmetric', in user-facing contexts. So I wonder if we should rename all the internals too?)
Reminding me of the rough edges marked with "###" in the code, and saying if they look like they need to be fixed before release and/or filed as issues.
I would recommend not looking at a diff, because I updated a lot of merge code along the way in order to make it easier to interface with but not significantly changed or specifically related to symmetric merge.
Instead, read hierarchically down the call graph from the top-level function svn_cl__merge(). It should be mostly pretty straightforward to see where the code is new and where it starts to call existing functions.
Thanks, and don't hesitate to ask further or to be picky.
- Julian
Re: Please review Symmetric Merge
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 09/13/2012 06:55 PM, Julian Foad wrote:
> Hi, merge fans and QA fans.
>
> As you've seen, the 'symmetric merge' code is live in trunk and destined
> to be in 1.8.
>
> The principle is nice; however, the current implementation is a rather
> ugly hack, being just a layer on top of the existing 'sync' and
> 'reintegrate' merge code.
>
> Please review it and send me all your criticism and suggestions.
> Seriously.
>
> I intend to do some self-review as well, but there's nothing like one of
> you fellow developers looking at it to motivate me to fix it :-)
Julian, can you give any pointers on reviewing this work? Are we talking
about a straight code read? Perhaps there's a good pair of trunk revision
the differences between which carry the lion's share of your changes? May I
assume that all the pertinent details remain in libsvn_client/merge.c?
--
C. Michael Pilato <cm...@collab.net>
CollabNet <> www.collab.net <> Enterprise Cloud Development