[RFC] [PATCH] ui: differentiate empty configlist from None

[Greg Ward]

On Wed, Aug 11, 2010 at 9:05 AM, Alecs King <alecsk at gmail.com> wrote:
> This is spotted when using the pager extension.
>
> The doc specifies that,
>
> '''
> Setting pager.attend to an empty value will cause all commands to be
> paged.
> '''
>
> Currently pager tries to get pager.attend configuration, and if it's not
> set, it uses a default attended list:
>
>            attend = ui.configlist('pager', 'attend', attended)
>
> But at the same time in ui.configlist, it doesnt differentiate a
> set-but-empty config list from not-set-at-all.  So it never works as
> intended when setting pager.attend to empty -- default is set still.
>
> This patch tries to fix that.  Have tested it a bit but i'm not quite
> sure if there's other code that relies on this (mis)feature (or bug, if
> you agree).  So is this the correct way to do, or should we remain
> ui.configlist untouched but instead make change in pager itself?
>
>
>  mercurial/ui.py |  2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
>
> # HG changeset patch
> # User Alecs King <alecsk at gmail.com>
> # Date 1281529719 -28800
> # Branch my
> # Node ID b815f1650d03a054a1e9b40761880b97afaad9ed
> # Parent  3d4a8bf13a65ac1878774eaaa07a4ae1f944455d
> ui: differentiate empty configlist from None
>
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -218,7 +218,7 @@ class ui(object):
>         def _configlist(s):
>             s = s.rstrip(' ,')
>             if not s:
> -                return None
> +                return []

On the surface, this looks sensible.  I'm sure if you had sent this
when configlist() was 3 days old, it would be accepted without a
second thought.  But a couple of concerns:

  * you're fixing a bug in pager, but your fix could affect lots more than just
    pager -- so it's highly unlikely your patch would be accepted on the stable
    branch, meaning the bug you found will be there until 1.7.  If you
want to fix
    pager in the next 1.6.x release, send a narrow patch that only
touches pager.

  * did you audit the code to see who else is calling configlist() and might be
    affected by this change?  if no, you really should do that

  * did you run the entire test suite after your patch?  if no, you must do that

My guess is that if you audit the code and run the entire test suite,
and do not find any problems from either exercise, this patch *might*
be accepted on the default branch, i.e. for Mercurial 1.7.  Otherwise,
probably not.

Not trying to be discouraging, just realistic.

Greg