Patchwork [5,of,5,RFC] patchbomb: add support for a file that configures default To and CC

login
register
mail settings
Submitter Augie Fackler
Date June 13, 2017, 10:12 p.m.
Message ID <f89290549dcefcb9d6be.1497391960@augie-macbookpro2.roam.corp.google.com>
Download mbox | patch
Permalink /patch/21365/
State Accepted
Headers show

Comments

Augie Fackler - June 13, 2017, 10:12 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1497387366 14400
#      Tue Jun 13 16:56:06 2017 -0400
# Node ID f89290549dcefcb9d6be2af966f870cb1dd1ead0
# Parent  c98f63a3e8ee15645646539ba08b7b7682237cab
patchbomb: add support for a file that configures default To and CC

This lets repositories configure a shorthand (I used devel in the
tests, but you could do anything) for their project list, and also
supports fileset-based "subscriptions" so that people can request
being CC'ed on certain changes by default.
Augie Fackler - June 13, 2017, 10:50 p.m.
Durham had the idea of a tweak to patchbomb that would let us not do https://www.mercurial-scm.org/wiki/Herald.

I implemented filesets for the RFC, but I think we should probably also allow reset-based subscriptions too (e.g. "histedit" as a keyword or similar). I'd appreciate any feedback people have, since this was a quick project and it feels like it might have some real utility.


> On Jun 13, 2017, at 18:12, Augie Fackler <raf@durin42.com> wrote:
> 
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1497387366 14400
> #      Tue Jun 13 16:56:06 2017 -0400
> # Node ID f89290549dcefcb9d6be2af966f870cb1dd1ead0
> # Parent  c98f63a3e8ee15645646539ba08b7b7682237cab
> patchbomb: add support for a file that configures default To and CC
> 
> This lets repositories configure a shorthand (I used devel in the
> tests, but you could do anything) for their project list, and also
> supports fileset-based "subscriptions" so that people can request
> being CC'ed on certain changes by default.
> 
> diff --git a/hgext/patchbomb.py b/hgext/patchbomb.py
> --- a/hgext/patchbomb.py
> +++ b/hgext/patchbomb.py
> @@ -83,6 +83,7 @@ from mercurial.i18n import _
> from mercurial import (
>     cmdutil,
>     commands,
> +    config,
>     error,
>     formatter,
>     hg,
> @@ -641,7 +642,7 @@ def email(ui, repo, *revs, **opts):
> 
>     showaddrs = []
> 
> -    def getaddrs(header, ask=False, default=None):
> +    def getaddrs(header, ask=False, trustdefault=True, default=None):
>         configkey = header.lower()
>         opt = header.replace('-', '_').lower()
>         addrs = opts.get(opt)
> @@ -655,7 +656,7 @@ def email(ui, repo, *revs, **opts):
>         if not addr:
>             specified = (ui.hasconfig('email', configkey) or
>                          ui.hasconfig('patchbomb', configkey))
> -            if not specified and ask:
> +            if (not trustdefault or not specified) and ask:
>                 addr = prompt(ui, header, default=default)
>         if addr:
>             showaddrs.append('%s: %s' % (header, addr))
> @@ -665,11 +666,56 @@ def email(ui, repo, *revs, **opts):
>                 ui, [default], _charsets, opts.get('test'))
>         return []
> 
> -    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:
> +                todefault.add(toflag)
> +                continue
> +            setsubs = toflag + '.setsubscriptions'
> +            # Awkward dance time: we want to preserve the value of
> +            # this --to flag iff there are no config sections defined
> +            # for this name. For now, it's this simple. If we grow
> +            # other sections beyond setsubscriptions, make sure to
> +            # update this logic.
> +            if setsubs in mailconf:
> +                confdefault = None
> +            else:
> +                confdefault = toflag
> +            todefault.add(mailconf.get(toflag, 'to', confdefault))
> +            for fileset, wantcc in mailconf.items(setsubs):
> +                for r in revs:
> +                    q = ('(modified() or added() or removed()) and (%s)' %
> +                         fileset)
> +                    if repo[r].getfileset(q):
> +                        ui.debug(
> +                            'subscription fileset %s matched, adding cc: %s\n'
> +                            % (q, wantcc))
> +                        ccdefault.update(config.parselist(wantcc))
> +        todefault = {t for t in todefault if t is not None}
> +        # We've already handled the flags, so null the flag out. This
> +        # also means the user will be prompted for an address, which
> +        # is what we want, since the configuration can change as part
> +        # of an `hg pull --update` or similar.
> +        opts['to'] = []
> +        opts['cc'] = []
> +        tocctrustdefault = False
> +
> +    to = getaddrs('To', ask=True, default=', '.join(sorted(todefault)),
> +                  trustdefault=tocctrustdefault)
>     if not to:
>         # we can get here in non-interactive mode
>         raise error.Abort(_('no recipient addresses provided'))
> -    cc = getaddrs('Cc', ask=True, default='')
> +    cc = getaddrs('Cc', ask=True, default=', '.join(sorted(ccdefault)),
> +                  trustdefault=tocctrustdefault)
>     bcc = getaddrs('Bcc')
>     replyto = getaddrs('Reply-To')
> 
> diff --git a/tests/test-patchbomb.t b/tests/test-patchbomb.t
> --- a/tests/test-patchbomb.t
> +++ b/tests/test-patchbomb.t
> @@ -3036,12 +3036,17 @@ Test hg email defaults settings:
>> [devel.setsubscriptions]
>> c or d = c-or-d@example.com
>> a or b = a-or-b@example.com
> +  > [foo.setsubscriptions]
> +  > binary() = hates-binary-files@example.com
>> EOF
> -This change modifies file d
> -  $ hg email --date '1970-1-1 0:2' --to=devel --test -r 10 --config patchbomb.publicurl=
> +This change modifies file d. Note that the "foo" to disappears from
> +the To line because it has a setsubscription defined above.
> +  $ hg email --date '1970-1-1 0:2' --to=devel --to=foo --test -r 10 --config patchbomb.publicurl=
>   From [test]: test
>   this patch series consists of 1 patches.
> 
> +  To [mercurial-devel@example.com]: mercurial-devel@example.com
> +  Cc [c-or-d@example.com]: c-or-d@example.com
> 
>   displaying [PATCH] dd ...
>   Content-Type: text/plain; charset="us-ascii"
> @@ -3056,7 +3061,8 @@ This change modifies file d
>   User-Agent: Mercurial-patchbomb/* (glob)
>   Date: Thu, 01 Jan 1970 00:02:00 +0000
>   From: test
> -  To: devel
> +  To: mercurial-devel@example.com
> +  Cc: c-or-d@example.com
> 
>   # HG changeset patch
>   # User test
> @@ -3079,6 +3085,8 @@ This change modifies file b
>   From [test]: test
>   this patch series consists of 1 patches.
> 
> +  To [mercurial-devel@example.com]: mercurial-devel@example.com
> +  Cc [a-or-b@example.com]: a-or-b@example.com
> 
>   displaying [PATCH] b ...
>   Content-Type: text/plain; charset="us-ascii"
> @@ -3093,7 +3101,8 @@ This change modifies file b
>   User-Agent: Mercurial-patchbomb/* (glob)
>   Date: Thu, 01 Jan 1970 00:02:00 +0000
>   From: test
> -  To: devel
> +  To: mercurial-devel@example.com
> +  Cc: a-or-b@example.com
> 
>   # HG changeset patch
>   # User test
> @@ -3115,6 +3124,8 @@ This change modifies file b
>   this patch series consists of 2 patches.
> 
>   (optional) Subject: [PATCH 0 of 2] 
> +  To [mercurial-devel@example.com, specific-person@example.com]: mercurial-devel@example.com, specific-person@example.com
> +  Cc [a-or-b@example.com, c-or-d@example.com, this-person-also@example.com]: a-or-b@example.com, c-or-d@example.com, this-person-also@example.com
> 
>   displaying [PATCH 1 of 2] b ...
>   Content-Type: text/plain; charset="us-ascii"
> @@ -3129,8 +3140,8 @@ This change modifies file b
>   User-Agent: Mercurial-patchbomb/* (glob)
>   Date: Thu, 01 Jan 1970 00:02:00 +0000
>   From: test
> -  To: devel, specific-person@example.com
> -  Cc: this-person-also@example.com
> +  To: mercurial-devel@example.com, specific-person@example.com
> +  Cc: a-or-b@example.com, c-or-d@example.com, this-person-also@example.com
> 
>   # HG changeset patch
>   # User test
> @@ -3161,8 +3172,8 @@ This change modifies file b
>   User-Agent: Mercurial-patchbomb/* (glob)
>   Date: Thu, 01 Jan 1970 00:02:01 +0000
>   From: test
> -  To: devel, specific-person@example.com
> -  Cc: this-person-also@example.com
> +  To: mercurial-devel@example.com, specific-person@example.com
> +  Cc: a-or-b@example.com, c-or-d@example.com, this-person-also@example.com
> 
>   # HG changeset patch
>   # User test
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - June 14, 2017, 2:17 p.m.
On Tue, 13 Jun 2017 18:50:27 -0400, Augie Fackler wrote:
> Durham had the idea of a tweak to patchbomb that would let us not do https://www.mercurial-scm.org/wiki/Herald.
> 
> I implemented filesets for the RFC, but I think we should probably also allow reset-based subscriptions too (e.g. "histedit" as a keyword or similar). I'd appreciate any feedback people have, since this was a quick project and it feels like it might have some real utility.

Looks pretty nice.

> > @@ -3036,12 +3036,17 @@ Test hg email defaults settings:
> >> [devel.setsubscriptions]
> >> c or d = c-or-d@example.com
> >> a or b = a-or-b@example.com
> > +  > [foo.setsubscriptions]
> > +  > binary() = hates-binary-files@example.com

Perhaps the config syntax could be arbitrary_key:attr = value.

  hatebin:fileset = binary()
  hatebin:revset = keyword(binary)
  hatebin:cc = hates-binary-files@example.com

fileset and revset can be quite long. We wouldn't want to write them as config
keys.

And I prefer using matcher patterns instead of fileset for consistency with
the other commands.
Augie Fackler - June 14, 2017, 3:19 p.m.
> On Jun 14, 2017, at 10:17, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> On Tue, 13 Jun 2017 18:50:27 -0400, Augie Fackler wrote:
>> Durham had the idea of a tweak to patchbomb that would let us not do https://www.mercurial-scm.org/wiki/Herald.
>> 
>> I implemented filesets for the RFC, but I think we should probably also allow reset-based subscriptions too (e.g. "histedit" as a keyword or similar). I'd appreciate any feedback people have, since this was a quick project and it feels like it might have some real utility.
> 
> Looks pretty nice.
> 
>>> @@ -3036,12 +3036,17 @@ Test hg email defaults settings:
>>>> [devel.setsubscriptions]
>>>> c or d = c-or-d@example.com
>>>> a or b = a-or-b@example.com
>>> +  > [foo.setsubscriptions]
>>> +  > binary() = hates-binary-files@example.com
> 
> Perhaps the config syntax could be arbitrary_key:attr = value.
> 
>  hatebin:fileset = binary()
>  hatebin:revset = keyword(binary)
>  hatebin:cc = hates-binary-files@example.com

Hmm. This feels a little quirky to me somehow (though it's in-line with how we handle them in config files generally). Could we instead do something like this?

> [devel]
> to = devel@example.com
> cc = always-cc@example.com
> 
> [devel.hatebin]
> fileset = binary()
> revset = keyword(binary)
> cc = hates-binary-files@example.com
> 
> [devel.a-or-b]
> fileset = a or b
> cc = a-or-b@example.com

That looks a little cleaner to me, but I don't feel especially strongly. Just seems like we may as well use the fact that we're not in a main config file to our advantage and use sections in a slightly clearer way...

> fileset and revset can be quite long. We wouldn't want to write them as config
> keys.
> 
> And I prefer using matcher patterns instead of fileset for consistency with
> the other commands.

Should we name it `files` then, and if people want to use a set they can do the set: thing in-situ? That's fine with me (I just find filesets to be the best thing).

I can also add the revset match flavor, that seems nice.

(Will hold off on a v2 until we get some consensus around the config format, since I think that's the hard part here.)
Sean Farley - June 15, 2017, 1:15 a.m.
Augie Fackler <raf@durin42.com> writes:

>> On Jun 14, 2017, at 10:17, Yuya Nishihara <yuya@tcha.org> wrote:
>> 
>> On Tue, 13 Jun 2017 18:50:27 -0400, Augie Fackler wrote:
>>> Durham had the idea of a tweak to patchbomb that would let us not do https://www.mercurial-scm.org/wiki/Herald.
>>> 
>>> I implemented filesets for the RFC, but I think we should probably also allow reset-based subscriptions too (e.g. "histedit" as a keyword or similar). I'd appreciate any feedback people have, since this was a quick project and it feels like it might have some real utility.
>> 
>> Looks pretty nice.
>> 
>>>> @@ -3036,12 +3036,17 @@ Test hg email defaults settings:
>>>>> [devel.setsubscriptions]
>>>>> c or d = c-or-d@example.com
>>>>> a or b = a-or-b@example.com
>>>> +  > [foo.setsubscriptions]
>>>> +  > binary() = hates-binary-files@example.com
>> 
>> Perhaps the config syntax could be arbitrary_key:attr = value.
>> 
>>  hatebin:fileset = binary()
>>  hatebin:revset = keyword(binary)
>>  hatebin:cc = hates-binary-files@example.com
>
> Hmm. This feels a little quirky to me somehow (though it's in-line with how we handle them in config files generally). Could we instead do something like this?
>
>> [devel]
>> to = devel@example.com
>> cc = always-cc@example.com
>> 
>> [devel.hatebin]
>> fileset = binary()
>> revset = keyword(binary)
>> cc = hates-binary-files@example.com
>> 
>> [devel.a-or-b]
>> fileset = a or b
>> cc = a-or-b@example.com
>
> That looks a little cleaner to me, but I don't feel especially strongly. Just seems like we may as well use the fact that we're not in a main config file to our advantage and use sections in a slightly clearer way...

When I read Yuya's suggestion, it felt more natural than [devel.a-or-b].
I'd prefer to keep Mercurial's config-looking stuff to be a single
style. I don't mind if we wholesale switch to something like tomal; I
just don't want to have to support both.
Yuya Nishihara - June 15, 2017, 2:41 p.m.
On Wed, 14 Jun 2017 11:19:56 -0400, Augie Fackler wrote:
> 
> > On Jun 14, 2017, at 10:17, Yuya Nishihara <yuya@tcha.org> wrote:
> > 
> > On Tue, 13 Jun 2017 18:50:27 -0400, Augie Fackler wrote:
> >> Durham had the idea of a tweak to patchbomb that would let us not do https://www.mercurial-scm.org/wiki/Herald.
> >> 
> >> I implemented filesets for the RFC, but I think we should probably also allow reset-based subscriptions too (e.g. "histedit" as a keyword or similar). I'd appreciate any feedback people have, since this was a quick project and it feels like it might have some real utility.
> > 
> > Looks pretty nice.
> > 
> >>> @@ -3036,12 +3036,17 @@ Test hg email defaults settings:
> >>>> [devel.setsubscriptions]
> >>>> c or d = c-or-d@example.com
> >>>> a or b = a-or-b@example.com
> >>> +  > [foo.setsubscriptions]
> >>> +  > binary() = hates-binary-files@example.com
> > 
> > Perhaps the config syntax could be arbitrary_key:attr = value.
> > 
> >  hatebin:fileset = binary()
> >  hatebin:revset = keyword(binary)
> >  hatebin:cc = hates-binary-files@example.com
> 
> Hmm. This feels a little quirky to me somehow (though it's in-line with how we handle them in config files generally). Could we instead do something like this?
> 
> > [devel]
> > to = devel@example.com
> > cc = always-cc@example.com
> > 
> > [devel.hatebin]
> > fileset = binary()
> > revset = keyword(binary)
> > cc = hates-binary-files@example.com
> > 
> > [devel.a-or-b]
> > fileset = a or b
> > cc = a-or-b@example.com
> 
> That looks a little cleaner to me, but I don't feel especially strongly. Just seems like we may as well use the fact that we're not in a main config file to our advantage and use sections in a slightly clearer way...

This seems also good. I don't have strong opinion about :attr syntax, which
would be good for consistency with the main config, but implementation-wise
it isn't pretty.

> > fileset and revset can be quite long. We wouldn't want to write them as config
> > keys.
> > 
> > And I prefer using matcher patterns instead of fileset for consistency with
> > the other commands.
> 
> Should we name it `files` then, and if people want to use a set they can do the set: thing in-situ? That's fine with me (I just find filesets to be the best thing).
> 
> I can also add the revset match flavor, that seems nice.

Yeah, and 'revset' could be called as 'revs', too.

Patch

diff --git a/hgext/patchbomb.py b/hgext/patchbomb.py
--- a/hgext/patchbomb.py
+++ b/hgext/patchbomb.py
@@ -83,6 +83,7 @@  from mercurial.i18n import _
 from mercurial import (
     cmdutil,
     commands,
+    config,
     error,
     formatter,
     hg,
@@ -641,7 +642,7 @@  def email(ui, repo, *revs, **opts):
 
     showaddrs = []
 
-    def getaddrs(header, ask=False, default=None):
+    def getaddrs(header, ask=False, trustdefault=True, default=None):
         configkey = header.lower()
         opt = header.replace('-', '_').lower()
         addrs = opts.get(opt)
@@ -655,7 +656,7 @@  def email(ui, repo, *revs, **opts):
         if not addr:
             specified = (ui.hasconfig('email', configkey) or
                          ui.hasconfig('patchbomb', configkey))
-            if not specified and ask:
+            if (not trustdefault or not specified) and ask:
                 addr = prompt(ui, header, default=default)
         if addr:
             showaddrs.append('%s: %s' % (header, addr))
@@ -665,11 +666,56 @@  def email(ui, repo, *revs, **opts):
                 ui, [default], _charsets, opts.get('test'))
         return []
 
-    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:
+                todefault.add(toflag)
+                continue
+            setsubs = toflag + '.setsubscriptions'
+            # Awkward dance time: we want to preserve the value of
+            # this --to flag iff there are no config sections defined
+            # for this name. For now, it's this simple. If we grow
+            # other sections beyond setsubscriptions, make sure to
+            # update this logic.
+            if setsubs in mailconf:
+                confdefault = None
+            else:
+                confdefault = toflag
+            todefault.add(mailconf.get(toflag, 'to', confdefault))
+            for fileset, wantcc in mailconf.items(setsubs):
+                for r in revs:
+                    q = ('(modified() or added() or removed()) and (%s)' %
+                         fileset)
+                    if repo[r].getfileset(q):
+                        ui.debug(
+                            'subscription fileset %s matched, adding cc: %s\n'
+                            % (q, wantcc))
+                        ccdefault.update(config.parselist(wantcc))
+        todefault = {t for t in todefault if t is not None}
+        # We've already handled the flags, so null the flag out. This
+        # also means the user will be prompted for an address, which
+        # is what we want, since the configuration can change as part
+        # of an `hg pull --update` or similar.
+        opts['to'] = []
+        opts['cc'] = []
+        tocctrustdefault = False
+
+    to = getaddrs('To', ask=True, default=', '.join(sorted(todefault)),
+                  trustdefault=tocctrustdefault)
     if not to:
         # we can get here in non-interactive mode
         raise error.Abort(_('no recipient addresses provided'))
-    cc = getaddrs('Cc', ask=True, default='')
+    cc = getaddrs('Cc', ask=True, default=', '.join(sorted(ccdefault)),
+                  trustdefault=tocctrustdefault)
     bcc = getaddrs('Bcc')
     replyto = getaddrs('Reply-To')
 
diff --git a/tests/test-patchbomb.t b/tests/test-patchbomb.t
--- a/tests/test-patchbomb.t
+++ b/tests/test-patchbomb.t
@@ -3036,12 +3036,17 @@  Test hg email defaults settings:
   > [devel.setsubscriptions]
   > c or d = c-or-d@example.com
   > a or b = a-or-b@example.com
+  > [foo.setsubscriptions]
+  > binary() = hates-binary-files@example.com
   > EOF
-This change modifies file d
-  $ hg email --date '1970-1-1 0:2' --to=devel --test -r 10 --config patchbomb.publicurl=
+This change modifies file d. Note that the "foo" to disappears from
+the To line because it has a setsubscription defined above.
+  $ hg email --date '1970-1-1 0:2' --to=devel --to=foo --test -r 10 --config patchbomb.publicurl=
   From [test]: test
   this patch series consists of 1 patches.
   
+  To [mercurial-devel@example.com]: mercurial-devel@example.com
+  Cc [c-or-d@example.com]: c-or-d@example.com
   
   displaying [PATCH] dd ...
   Content-Type: text/plain; charset="us-ascii"
@@ -3056,7 +3061,8 @@  This change modifies file d
   User-Agent: Mercurial-patchbomb/* (glob)
   Date: Thu, 01 Jan 1970 00:02:00 +0000
   From: test
-  To: devel
+  To: mercurial-devel@example.com
+  Cc: c-or-d@example.com
   
   # HG changeset patch
   # User test
@@ -3079,6 +3085,8 @@  This change modifies file b
   From [test]: test
   this patch series consists of 1 patches.
   
+  To [mercurial-devel@example.com]: mercurial-devel@example.com
+  Cc [a-or-b@example.com]: a-or-b@example.com
   
   displaying [PATCH] b ...
   Content-Type: text/plain; charset="us-ascii"
@@ -3093,7 +3101,8 @@  This change modifies file b
   User-Agent: Mercurial-patchbomb/* (glob)
   Date: Thu, 01 Jan 1970 00:02:00 +0000
   From: test
-  To: devel
+  To: mercurial-devel@example.com
+  Cc: a-or-b@example.com
   
   # HG changeset patch
   # User test
@@ -3115,6 +3124,8 @@  This change modifies file b
   this patch series consists of 2 patches.
   
   (optional) Subject: [PATCH 0 of 2] 
+  To [mercurial-devel@example.com, specific-person@example.com]: mercurial-devel@example.com, specific-person@example.com
+  Cc [a-or-b@example.com, c-or-d@example.com, this-person-also@example.com]: a-or-b@example.com, c-or-d@example.com, this-person-also@example.com
   
   displaying [PATCH 1 of 2] b ...
   Content-Type: text/plain; charset="us-ascii"
@@ -3129,8 +3140,8 @@  This change modifies file b
   User-Agent: Mercurial-patchbomb/* (glob)
   Date: Thu, 01 Jan 1970 00:02:00 +0000
   From: test
-  To: devel, specific-person@example.com
-  Cc: this-person-also@example.com
+  To: mercurial-devel@example.com, specific-person@example.com
+  Cc: a-or-b@example.com, c-or-d@example.com, this-person-also@example.com
   
   # HG changeset patch
   # User test
@@ -3161,8 +3172,8 @@  This change modifies file b
   User-Agent: Mercurial-patchbomb/* (glob)
   Date: Thu, 01 Jan 1970 00:02:01 +0000
   From: test
-  To: devel, specific-person@example.com
-  Cc: this-person-also@example.com
+  To: mercurial-devel@example.com, specific-person@example.com
+  Cc: a-or-b@example.com, c-or-d@example.com, this-person-also@example.com
   
   # HG changeset patch
   # User test