[PATCH 1 of 5] add hgmerge subsystem

Arve Knudsen arve.knudsen at gmail.com
Sun Feb 3 15:31:06 CST 2008


On Feb 1, 2008 6:16 PM, Matt Mackall <mpm at selenic.com> wrote:
>
> 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.

Removed.

> > +""" 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.

OK.

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

Changed.

> > +# 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.

Changed.

> > +    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.

Changed.

> > +    ''' 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.

They've probably _become_ unused. Removed.

> > +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!

This method also contained more code before.

> > +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)

Will we ever have so many tool definitions that this makes any difference?

Arve

> 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.
>
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>


More information about the Mercurial-devel mailing list