[PATCH 3 of 3 v2] patchbomb: add support for a file that configures default To and CC

Yuya Nishihara yuya at tcha.org
Thu Jun 15 10:35:50 EDT 2017


On Wed, 14 Jun 2017 23:16:34 -0400, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie at google.com>
> # Date 1497387366 14400
> #      Tue Jun 13 16:56:06 2017 -0400
> # Node ID 5b5570529e237d1fd70ef3e6ba80739bd51a9d52
> # Parent  85fa6c9f000f374bebd40c401e4d0945d2194e1f
> patchbomb: add support for a file that configures default To and CC

The config syntax looks good to me, thanks. A few bugs found.

> -    to = getaddrs('To', ask=True)
> +    todefault = set()
> +    ccdefault = set(opts['cc'])
> +    tocctrustdefault = True
> +    if opts['to'] and repo.wvfs.exists('.hgemaildefaults'):
> +        mailconf = config.config()
> +        confpath = repo.wvfs.join('.hgemaildefaults')
> +        with repo.wvfs('.hgemaildefaults') as fp:
> +            mailconf.parse(confpath, fp.read())
> +        for toflag in opts['to']:
> +            # @ is unsupported in destination configs to avoid
> +            # collisions with routable email addresses.
> +            if '@' in toflag or toflag not in mailconf:
> +                todefault.add(toflag)
> +                continue
> +            todefault.add(mailconf.get(toflag, 'to', None))
> +            ccdefault.add(mailconf.get(toflag, 'cc', None))

Perhaps these should be parsed as lists to deduplicate to/cc addresses.

> +            # Find all the config entries in our current section,
> +            # minus reserved entries...
> +            subents = ((k, v) for k, v in mailconf.items(toflag)
> +                       if k not in ('cc', 'to', 'bcc'))
> +            # Group them by the part before the :
> +            grouped = itertools.groupby(
> +                subents, key=lambda s: s[0].split(':')[0])
> +            # Now we coalesce it all down to a dict of dicts
> +            subs = {subsec:
> +                    {key[len(subsec) + 1:]: value for key, value in values}
> +                    for subsec, values in grouped}
> +            for r in revs:
> +                ctx = repo[r]
> +                mod = set(ctx.getfileset('modified() or added() or removed()'))
> +                for name, sub in subs.iteritems():
> +                    match = False
> +                    if 'files' in sub:
> +                        match = bool(
> +                            mod.intersection(ctx.match(sub['files'])))

Since matcher requires a list of patterns, this would be parsed as
['s', 'e', 't', ':', ...]. Also, list(matcher) does not return matched files,
but a list of explicitly-specified patterns. I don't know the best practice
to match against changed files, but one way is

  [f for f in mod if m(f)]

And matcher is relative to cwd by default. We'll probably want to specify
matchmod.match(root, cwd='', ...) to force path resolution to relative to
repo.root.

> +        todefault = {t for t in todefault if t is not None}
> +        ccdefault = {t for t in ccdefault if t is not None}

Nit: could be todefault.discard(None)

> diff --git a/tests/test-patchbomb.t b/tests/test-patchbomb.t
> --- a/tests/test-patchbomb.t
> +++ b/tests/test-patchbomb.t
> @@ -85,9 +85,9 @@ email.to config setting. Same for --cc:
>    X-Mercurial-Node: 8580ff50825a50c8f716709acdf8de0deddcd6ab
>    X-Mercurial-Series-Index: 1
>    X-Mercurial-Series-Total: 1
> -  Message-Id: <8580ff50825a50c8f716.60 at augie-macbookpro2.roam.corp.google.com>
> -  X-Mercurial-Series-Id: <8580ff50825a50c8f716.60 at augie-macbookpro2.roam.corp.google.com>
> -  User-Agent: Mercurial-patchbomb/4.2.1+627-72f2cafb81e3
> +  Message-Id: <8580ff50825a50c8f716.*@*> (glob)
> +  X-Mercurial-Series-Id: <8580ff50825a50c8f716.*@*> (glob)
> +  User-Agent: Mercurial-patchbomb/* (glob)

This should belong to the previous patch.


More information about the Mercurial-devel mailing list