[PATCH 1 of 5] add hgmerge subsystem

Matt Mackall mpm at selenic.com
Fri Feb 1 11:16:28 CST 2008


On Thu, 2008-01-31 at 22:56 -0600, Steve Borho wrote:
> # HG changeset patch
> # User Steve Borho <steve at borho.org>
> # Date 1201840117 21600
> # Node ID d97cd00ba47b21050f1c85ccd0e06bba36d80eee
> # Parent  9f1e6ab76069641a5e3ff5b0831da0a3f83365cb
> add hgmerge subsystem
> 
> diff --git a/mercurial/hgmerge/__init__.py b/mercurial/hgmerge/__init__.py
> new file mode 100644
> --- /dev/null
> +++ b/mercurial/hgmerge/__init__.py
> @@ -0,0 +1,461 @@
> +# -*- coding: utf-8 -*-

Is there any utf-8 in this file? There shouldn't be.

> +""" Logic for merging changes in two versions of a file.
> + at var stocktools: Sequence of stock plug-in representations.
> +"""
> +import shutil
> +import StringIO
> +import filecmp
> +import random
> +import traceback
> +import os.path
> +import re

One line, please.

> +import mercurial.util as hgutil
> +import _simplemerge
> +import _plugins as hgmergeplugs
> +from _plugins import toolplugin
> +import _pluginapi as hgplugapi

Yuck?

> +# Initially not defined
> +plugins = None
> +
> +stocktools = hgmergeplugs.plugins
> +
> +def _eoltype(fctx):
> +    ''' The EOL type of the file.
> +    '''

Single line doc-strings might as well be a single line.

> +    if fctx.islink():
> +        return 'symlink'
> +    else:
> +        return hgutil.eoltype(fctx.fname)

'symlink' is an eoltype?

> +
> +class _optexception(Exception):
> +    pass
> +
> +class _invalidoptval(_optexception):
> +    ''' Invalid value for option.
> +    '''
> +    def __init__(self, option, val):
> +        _optexception.__init__(self, 'Invalid value for %s: %s' % (option, val))
> +        self.option, self.value = option, val
> +
> +class _missingopt(_optexception):
> +    ''' A required option is missing.
> +    '''
> +    def __init__(self, option):
> +        _optexception.__init__(self, 'Option %s is missing' % option)
> +        self.option = option
> +
> +class _pluginopts(object):
> +    ''' Plug-in options handler baseclass.
> +    '''
> +    def __init__(self, ui, sectname, name):
> +        self.attrs = {}
> +        self.__ui, self.__sect, self.__name = ui, sectname, name
> +        self._set_int('priority')
> +
> +    def _set_string(self, prop, **kwds):
> +        self._set_opt(prop, self.__ui.config, **kwds)
> +
> +    def _set_bool(self, prop, **kwds):
> +        self._set_opt(prop, self.__ui.configbool, **kwds)
> +
> +    def _set_list(self, prop, **kwds):
> +        self._set_opt(prop, self._configlist, **kwds)
> +
> +    def _set_int(self, prop, **kwds):
> +        self._set_opt(prop, self._configint, **kwds)
> +
> +    def _set_opt(self, prop, getfunc, attr=None):
> +        try:
> +            val = getfunc(self.__sect, '%s.%s' % (self.__name, prop))
> +        except ValueError:
> +            if getfunc is self.__ui.config:
> +                raise
> +            # Conversion error
> +            val = None
> +            raise _invalidoptval(prop, val)
> +        if val is None:
> +            return
> +
> +        if attr is None:
> +            attr = prop.replace('.', '_')
> +        self.attrs[attr] = val
> +
> +    def _configint(self, sect, field):
> +        val = self.__ui.config(sect, field)
> +        if val is not None:
> +            return int(val)
> +
> +    def _configlist(self, sect, field):
> +        ''' Replace ui.configlist since it doesn't seem to allow elements with
> +        whitespace.
> +        '''
> +        val = self.__ui.config(sect, field)
> +        if val is not None:
> +            import shlex
> +            return shlex.split(val)
> +
> +class _toolopts(_pluginopts):
> +    ''' Tool plug-in options handler.
> +    '''
> +    def __init__(self, ui, sectname, name):
> +        _pluginopts.__init__(self, ui, sectname, name)
> +
> +        self._set_string('executable')
> +        self._set_list('args')
> +        self._set_string('winreg_key')
> +        self._set_string('winreg_name')
> +        self._set_string('winreg_append')
> +        for prop in ('stdout', 'check_conflicts', 'symlink', 'binary'):
> +            self._set_bool(prop)

Still hopelessly overdesigned. Rather than setting an option property in
some object that exists pretty much only to hold properties, just look
the value up when you need it in the context it's needed in.

> +def _create_tool(name, opts, prevplug, ui):

_createtool, please. When given a name "foo bar", there should be one
consistent way to transform it into a function name, be it FooBar,
foo_bar, or foobar. In Mercurial, the latter is standard.

> +    ''' Create tool from user definition.
> +    '''
> +    def warn_invalid(reason):
> +        ui.warn('invalid tool definition: %s (%s)\n' % (name, reason))
> +
> +    plug = toolplugin(name, **opts.attrs)
> +    plug.userprops = opts.attrs.keys()
> +    return plug

Except really, this function is practically pointless. warn_invalid is
unused. And I'm not sure what's up with userprops, as we're already
passing opts.attrs to the constructor.

> +def _modify_plug(plug, opts, ui):
> +    plug.set_options(opts, ui)

This just blew my mind. Your helper function is longer than the direct
method!

> +def _setup_plugs(ui):
> +    def handle_conf(name, plugs):
> +        for p in plugs:
> +            if p.name.lower() == name.lower():

a) This is O(n^2)
b) Do we really want to be case-insensitive?

> +        # If this is a new plug-in, define a new plug-in, otherwise, apply to
> +        # existing

If you didn't create option objects solely to track these things, this
sort of complexity would be unnecessary.

(scrolls ahead a bit, attention span expires)

This is all still too much code for what is conceptually a very
straightforward thing. We choose the highest priority helper that's
available and call it, with some simple checks and prep before and
after.

-- 
Mathematics is the supreme nostalgia of our time.



More information about the Mercurial-devel mailing list