[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