bzr-hookless-email doesn't work with bzr 2.6

[Vincent Ladeuil]

>>>>> Glenn Morris <rgm at gnu.org> writes:

<snip/>

    > We found this bug report:
    > https://bugs.launchpad.net/bzr-hookless-email/+bug/988195

    > Applying the patch from there made it work again, for a while.

    > Now it is failing for the Emacs repository, with this error:

    >     Traceback (most recent call last):
    >       File "/usr/src/bzr-hookless-email/bzr_hookless_email.py", line 347, in <module>
    >         main()
    >       File "/usr/src/bzr-hookless-email/bzr_hookless_email.py", line 79, in main
    >         branch.update()
    >       File "/usr/src/bzr-hookless-email/bzr_hookless_email.py", line 147, in update
    >         msg = self._compose_email(revision)
    >       File "/usr/src/bzr-hookless-email/bzr_hookless_email.py", line 210, in _compose_email
    >         rev1 = rev2 = self._branch.revision_id_to_revno(revid) or None
    >       File "<string>", line 4, in revision_id_to_revno_read_locked
    >       File "/usr/lib/python2.6/dist-packages/bzrlib/branch.py", line 2852, in revision_id_to_revno
    >         raise errors.NoSuchRevision(self, revision_id)
    >     bzrlib.errors.NoSuchRevision: BzrBranch7(file:///srv/bzr/emacs/trunk/) has no revision rgm at gnu.org-20130428174032-q60c2x7yqlpm469j

    > That revid does exist, but it is not on trunk, it is on the emacs-24 branch.
    > I recently merged that commit from emacs-24 to trunk.


    > I guess that is the cause. The last commit mail that appeared is for the
    > one prior to the merge (r112528 on Emacs trunk).
    > So I guess the bzr-hookless-email patch that we found doesn't handle merges.


    > Does anyone have any ideas about how to fix bzr-hookless-email?

Not with the information provided here (and reading the code didn't
triggered light bulbs), sorry :-/

If the revision has been merged, it is part of the branch history and is
present.  The revision_id_to_revno may hint that the constraint is that
the last revision sent should be on the "mainline" and should have a
simple revno instead of a dotted revno, but that's a wild guess :-/

Simply replacing 'revision_id_to_revno' with 'revision_id_to_dotted_revno'
may work but that would be a truly lucky guess...

    > I guess the only other option is to use the bzr-email plugin.

Not sure about your use case here, but from reading bzr-hookless-email
it seems to send one email by revision for all revisions since the last
mail has been sent. Whereas bzr-email will send only one mail without
tracking the revision. Does that still fit your use case ?

Also, if bzr-hookless-email want to send mails from the last sent
revision, may be you can just skip the problematic revision and set
'last_revision_mailed' to a newer one ? That may work around the issue
while searching for the proper fix ?

    > 
    > IIUC, the reason why Savannah does not use that is security
    > concerns:

    > http://lists.gnu.org/archive/html/savannah-hackers/2009-12/msg00009.html
    > http://lists.gnu.org/archive/html/savannah-hackers-public/2010-03/msg00028.html

AFAIUI, these threads are about "Setting up bzr+ssh on Savannah" which
seems to be a broader subject than just emailing about commits.

    > I don't really know what those concerns are.

Broadly speaking, if you allow any code from a branch to run as part of
a bzr action, you allow people with write access to a branch to run any
code they want.

    > I can understand not wanting to allow users to install arbitrary
    > plugins, but I don't really see the problem with a
    > system-installed plugin.

Yup, to counter the issue mentioned above, you should only run plugins
installed and controlled by the site admins.

I had a look at bzr-hookless-email but it uses too many pieces I don't
know so I can't really comment on how or why it fails here.

I know bzr-email a bit better though and have been using it for years.

    > Maybe the concern is that someone with write access to a branch's
    > bazaar.conf

Yeah, it's called branch.conf in this case but may define the same
options than bazaar.conf (and locations.conf for that matter), that may
be an entry point for malicious users but there are ways to avoid that
(see below).

    > could eg set post_commit_mailer to something malicious?

Not sure what, it has to be an external resource supposed to be a
mailer. Now, if post_commit_mailer is under the branch control, it can
point to some code added by the branch.

So, if there is need to counter this potential vulnerability,
post_commit_mailer should remain under site admins control.

    > I don't know. I suppose we'd have to try to hack the plugin to
    > read very few config options (basically, on or off for any
    > branch).  There's more hope of someone like me being able to do
    > that then to fix bzr-hookless-email.

Same here :-/

In bzr-email, the design is that a few hooks are defined and will
received a "config stack" from the branch they are run for.

This stack will search for the option values (in this order) defined:

- on the command line,
- in the locations.conf file
- in the branch.conf (for the branch involved) file,
- in the bazaar.conf file,
- as default.

So, it's unclear to me if/where you can define options in a way that an
arbitrary user cannot override as it depends on what policy savannah
enforces for the locations.conf and bazaar.conf files (if a user can
modify a branch he generally can modify branch.conf too).

If bzr is run as a specific user that only savannah admins have access
to, defining options in locations.conf (with a section valid for all
branches) will be a way to override whatever is defined in branch.conf.

If not, well, may be you can patch the plugin to use hard-coded values
instead of getting them from a config stack ? Or getting them from a
different config stack (which could be tailored to be as specific as
needed).

For reference, branch.conf (which is used by bzr-email-hooks,
bzr-hookless-email is less clear but could be upgraded to use the newer
API (the old one is still available for compatibility) is defined as:

class BranchStack(Stack):
    """Per-location options falling back to branch then global options stack.

    The following sections are queried:

    * command-line overrides,

    * the sections matching ``location`` in ``locations.conf``, the order being
      defined by the number of path components in the section glob, higher
      numbers first (from most specific section to most generic),

    * the no-name section in branch.conf,

    * the ``DEFAULT`` section in ``bazaar.conf``.

    This stack will use the no-name section in ``branch.conf`` as its
    MutableSection.
    """

    def __init__(self, branch):
        lstore = self.get_shared_store(LocationStore())
        bstore = branch._get_config_store()
        gstore = self.get_shared_store(GlobalStore())
        super(BranchStack, self).__init__(
            [self._get_overrides,
             LocationMatcher(lstore, branch.base).get_sections,
             NameMatcher(bstore, None).get_sections,
             NameMatcher(gstore, 'DEFAULT').get_sections],
            bstore)

which for savannah could be simplified to be just (untested code):

class SavannahStack():
    """Savannah options stack.

    The following sections are queried:

    * the no-name section in /whatever/savannah.conf,

    This stack will use the no-name section in ``savannah.conf`` as its
    MutableSection.
    """

    def __init__(self):
        sstore = self.get_shared_store(SavannahStore())
        super(SavannahStack, self).__init__(
            [NameMatcher(sstore, None).get_sections,
            sstore)

class SavannahStore(LockableIniFileStore):

    def __init__(self, possible_transports=None):
        t = transport.get_transport_from_path(
            '/whatever, possible_transports=possible_transports)
        super(SavannahStore, self).__init__(t, 'savannah.conf')

'/whatever' should be a directory under admins control and can be
read-only for everybody else.

    > I wish Savannah had just stuck with bzr 2.5. Is downgrading feasible?

It should be feasible. The only case I can think of that may break is a
plugin upgraded for 2.6 and not compatible with 2.5.

But they were probably reasons to upgrade to 2.6 in the first place that
I don't know.

    > TIA for any help.

Hope this helps,

     Vincent