Re: Please review Symmetric Merge

["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

Re: Please review Symmetric Merge

[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