{"thread":{"id":"34326","subject":"Review of git multimail","startedAt":"2013-07-02T19:23:39Z","lastAt":"2013-07-04T08:27:05Z","messageCount":15,"participants":["Ramkumar Ramachandra","John Keeping","Junio C Hamano","Michael Haggerty","Jed Brown","Matthieu Moy"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"222393","messageId":"1372793019-12162-1-git-send-email-artagnon@gmail.com","threadId":"34326","inReplyTo":null,"subject":"Review of git multimail","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-07-02T19:23:39Z","receivedAt":"2013-07-02T19:23:39Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nI figured that we should quickly read through git-multimail and give\nit an on-list review.  Hopefully, it'll educate the list about what\nthis is, and help improve the script itself.\n\nSources: https://github.com/mhagger/git-multimail\n\ngit_multimail.py wrote:\n> #! /usr/bin/env python2\n\nDo all distributions ship it as python2 now?\n\n> class CommandError(Exception):\n>     def __init__(self, cmd, retcode):\n>         self.cmd = cmd\n>         self.retcode = retcode\n>         Exception.__init__(\n>             self,\n>             'Command \"%s\" failed with retcode %s' % (' '.join(cmd), retcode,)\n\nSo cmd is a list.\n\n> class ConfigurationException(Exception):\n>     pass\n\nDead code?\n\n> def read_git_output(args, input=None, keepends=False, **kw):\n>     \"\"\"Read the output of a Git command.\"\"\"\n> \n>     return read_output(\n>         ['git', '-c', 'i18n.logoutputencoding=%s' % (ENCODING,)] + args,\n>         input=input, keepends=keepends, **kw\n>         )\n\nOkay, although I'm wondering what i18n.logoutputencoding has to do with anything.\n\n> def read_output(cmd, input=None, keepends=False, **kw):\n>     if input:\n>         stdin = subprocess.PIPE\n>     else:\n>         stdin = None\n>     p = subprocess.Popen(\n>         cmd, stdin=stdin, stdout=subprocess.PIPE, stderr=subprocess.PIPE, **kw\n>         )\n>     (out, err) = p.communicate(input)\n>     retcode = p.wait()\n>     if retcode:\n>         raise CommandError(cmd, retcode)\n>     if not keepends:\n>         out = out.rstrip('\\n\\r')\n>     return out\n\nHelper function that serves a single caller, read_git_output().\n\n> def read_git_lines(args, keepends=False, **kw):\n>     \"\"\"Return the lines output by Git command.\n> \n>     Return as single lines, with newlines stripped off.\"\"\"\n> \n>     return read_git_output(args, keepends=True, **kw).splitlines(keepends)\n\nOkay.\n\n> class Config(object):\n>     def __init__(self, section, git_config=None):\n>         \"\"\"Represent a section of the git configuration.\n> \n>         If git_config is specified, it is passed to \"git config\" in\n>         the GIT_CONFIG environment variable, meaning that \"git config\"\n>         will read the specified path rather than the Git default\n>         config paths.\"\"\"\n> \n>         self.section = section\n>         if git_config:\n>             self.env = os.environ.copy()\n>             self.env['GIT_CONFIG'] = git_config\n>         else:\n>             self.env = None\n\nOkay.\n\n>     @staticmethod\n>     def _split(s):\n>         \"\"\"Split NUL-terminated values.\"\"\"\n> \n>         words = s.split('\\0')\n>         assert words[-1] == ''\n>         return words[:-1]\n\nUgh.  Two callers of this poorly-defined static method: I wonder if\nwe'd be better off inlining it.\n\n>     def get(self, name, default=''):\n>         try:\n>             values = self._split(read_git_output(\n>                     ['config', '--get', '--null', '%s.%s' % (self.section, name)],\n>                     env=self.env, keepends=True,\n>                     ))\n\nWait, what is the point of using --null and then splitting by hand\nusing a poorly-defined static method?  Why not drop the --null and\nsplitlines() as usual?\n\n>             assert len(values) == 1\n\nWhen does this assert fail?\n\n>             return values[0]\n>         except CommandError:\n>             return default\n\nIf you're emulating the dictionary get method, default=None.  This is\nnot C, where all codepaths of the function must return the same type.\n\n>     def get_bool(self, name, default=None):\n>         try:\n>             value = read_git_output(\n>                 ['config', '--get', '--bool', '%s.%s' % (self.section, name)],\n>                 env=self.env,\n>                 )\n>         except CommandError:\n>             return default\n>         return value == 'true'\n\nCorrect.  On success, return bool.  On failure, return None.\n\n>     def get_all(self, name, default=None):\n>         \"\"\"Read a (possibly multivalued) setting from the configuration.\n> \n>         Return the result as a list of values, or default if the name\n>         is unset.\"\"\"\n> \n>         try:\n>             return self._split(read_git_output(\n>                 ['config', '--get-all', '--null', '%s.%s' % (self.section, name)],\n>                 env=self.env, keepends=True,\n>                 ))\n>         except CommandError, e:\n\nCommandError as e?\n\n>             if e.retcode == 1:\n\nWhat does this cryptic retcode mean?\n\n>                 return default\n>             else:\n>                 raise\n\nraise what?\n\nYou've instantiated the Config class in two places: user and\nmultimailhook sections.  Considering that you're going to read all the\nkeys in that section, why not --get-regexp, pre-load the configuration\ninto a dictionary and refer to that instead of spawning 'git config'\nevery time you need a configuration value?\n\n>     def get_recipients(self, name, default=None):\n>         \"\"\"Read a recipients list from the configuration.\n> \n>         Return the result as a comma-separated list of email\n>         addresses, or default if the option is unset.  If the setting\n>         has multiple values, concatenate them with comma separators.\"\"\"\n> \n>         lines = self.get_all(name, default=None)\n>         if lines is None:\n>             return default\n>         return ', '.join(line.strip() for line in lines)\n\nUgh.\n\n>     def set(self, name, value):\n>         read_git_output(\n>             ['config', '%s.%s' % (self.section, name), value],\n>             env=self.env,\n>             )\n>\n>     def add(self, name, value):\n>         read_git_output(\n>             ['config', '--add', '%s.%s' % (self.section, name), value],\n>             env=self.env,\n>             )\n> \n>     def has_key(self, name):\n>         return self.get_all(name, default=None) is not None\n> \n>     def unset_all(self, name):\n>         try:\n>             read_git_output(\n>                 ['config', '--unset-all', '%s.%s' % (self.section, name)],\n>                 env=self.env,\n>                 )\n>         except CommandError, e:\n>             if e.retcode == 5:\n>                 # The name doesn't exist, which is what we wanted anyway...\n>                 pass\n>             else:\n>                 raise\n> \n>     def set_recipients(self, name, value):\n>         self.unset_all(name)\n>         for pair in getaddresses([value]):\n>             self.add(name, formataddr(pair))\n\nDead code?\n\n> def generate_summaries(*log_args):\n>     \"\"\"Generate a brief summary for each revision requested.\n> \n>     log_args are strings that will be passed directly to \"git log\" as\n>     revision selectors.  Iterate over (sha1_short, subject) for each\n>     commit specified by log_args (subject is the first line of the\n>     commit message as a string without EOLs).\"\"\"\n> \n>     cmd = [\n>         'log', '--abbrev', '--format=%h %s',\n>         ] + list(log_args) + ['--']\n\nWhat is log_args if not a list?\n\nBut yeah, log is the best way to generate summaries.\n\n>     for line in read_git_lines(cmd):\n>         yield tuple(line.split(' ', 1))\n\nOkay, let's see how you use this iterator.\n\n> def limit_lines(lines, max_lines):\n>     for (index, line) in enumerate(lines):\n>         if index < max_lines:\n>             yield line\n> \n>     if index >= max_lines:\n>         yield '... %d lines suppressed ...\\n' % (index + 1 - max_lines,)\n\nRandom helper.\n\n> def limit_linelength(lines, max_linelength):\n>     for line in lines:\n>         # Don't forget that lines always include a trailing newline.\n>         if len(line) > max_linelength + 1:\n>             line = line[:max_linelength - 7] + ' [...]\\n'\n>         yield line\n\nRandom helper.\n\n> class GitObject(object):\n>     def __init__(self, sha1, type=None):\n>         if sha1 == ZEROS:\n>             self.sha1 = self.type = self.commit = None\n>         else:\n>             self.sha1 = sha1\n>             self.type = type or read_git_output(['cat-file', '-t', self.sha1])\n> \n>             if self.type == 'commit':\n>                 self.commit = self\n>             elif self.type == 'tag':\n>                 try:\n>                     self.commit = GitObject(\n>                         read_git_output(['rev-parse', '--verify', '%s^0' % (self.sha1,)]),\n>                         type='commit',\n>                         )\n>                 except CommandError:\n>                     self.commit = None\n>             else:\n>                 self.commit = None\n> \n>         self.short = read_git_output(['rev-parse', '--short', sha1])\n\nJust rev-parse --verify --short $SHA1^0: if it resolves, set\nself.short; one liner?\n\n>     def get_summary(self):\n>         \"\"\"Return (sha1_short, subject) for this commit.\"\"\"\n> \n>         if not self.sha1:\n>             raise ValueError('Empty commit has no summary')\n\nWhat is the point of letting the user instantiate a GitObject without\na valid .sha1 in the first place?\n\n>         return iter(generate_summaries('--no-walk', self.sha1)).next()\n\nNot exactly fond of this, but I don't have a concrete replacement at\nthe moment.\n\n>     def __eq__(self, other):\n>         return isinstance(other, GitObject) and self.sha1 == other.sha1\n> \n>     def __hash__(self):\n>         return hash(self.sha1)\n> \n>     def __nonzero__(self):\n>         return bool(self.sha1)\n\nOkay.\n\n>     def __str__(self):\n>         return self.sha1 or ZEROS\n\nI wonder what value this adds when .short is around.\n\n> class Change(object):\n>     \"\"\"A Change that has been made to the Git repository.\n> \n>     Abstract class from which both Revisions and ReferenceChanges are\n>     derived.  A Change knows how to generate a notification email\n>     describing itself.\"\"\"\n> \n>     def __init__(self, environment):\n>         self.environment = environment\n>         self._values = None\n> \n>     def _compute_values(self):\n>         \"\"\"Return a dictionary {keyword : expansion} for this Change.\n> \n>         Derived classes overload this method to add more entries to\n>         the return value.  This method is used internally by\n>         get_values().  The return value should always be a new\n>         dictionary.\"\"\"\n> \n>         return self.environment.get_values()\n\nWhy is this an \"internal function\"?  What is your criterion for\ninternal versus non-internal?\n\n>     def get_values(self, **extra_values):\n>         \"\"\"Return a dictionary {keyword : expansion} for this Change.\n> \n>         Return a dictionary mapping keywords to the values that they\n>         should be expanded to for this Change (used when interpolating\n>         template strings).  If any keyword arguments are supplied, add\n>         those to the return value as well.  The return value is always\n>         a new dictionary.\"\"\"\n> \n>         if self._values is None:\n>             self._values = self._compute_values()\n> \n>         values = self._values.copy()\n>         if extra_values:\n>             values.update(extra_values)\n>         return values\n\nUnsure what this is about.\n\n>     def expand(self, template, **extra_values):\n>         \"\"\"Expand template.\n> \n>         Expand the template (which should be a string) using string\n>         interpolation of the values for this Change.  If any keyword\n>         arguments are provided, also include those in the keywords\n>         available for interpolation.\"\"\"\n> \n>         return template % self.get_values(**extra_values)\n> \n>     def expand_lines(self, template, **extra_values):\n>         \"\"\"Break template into lines and expand each line.\"\"\"\n> \n>         values = self.get_values(**extra_values)\n>         for line in template.splitlines(True):\n>             yield line % values\n\nOkay.\n\n>     def expand_header_lines(self, template, **extra_values):\n>         \"\"\"Break template into lines and expand each line as an RFC 2822 header.\n> \n>         Encode values and split up lines that are too long.  Silently\n>         skip lines that contain references to unknown variables.\"\"\"\n> \n>         values = self.get_values(**extra_values)\n>         for line in template.splitlines(True):\n>             (name, value) = line.split(':', 1)\n>             value = value.rstrip('\\n\\r')\n\nDoesn't splitlines() make the rstrip() redundant?\n\n>             try:\n>                 value = value % values\n>             except KeyError, e:\n>                 if DEBUG:\n>                     sys.stderr.write(\n>                         'Warning: unknown variable %r in the following line; line skipped:\\n'\n>                         '    %s'\n>                         % (e.args[0], line,)\n>                         )\n\nIf DEBUG isn't on, you risk leaving the value string interpolated\nwithout even telling the user.  What does it mean to the end user?\n\n>             else:\n>                 try:\n>                     h = Header(value, header_name=name)\n>                 except UnicodeDecodeError:\n>                     h = Header(value, header_name=name, charset=CHARSET, errors='replace')\n>                 for splitline in ('%s: %s\\n' % (name, h.encode(),)).splitlines(True):\n>                     yield splitline\n\nNot elated by this exception cascading, but I suppose it's cheaper\nthan actually checking everything.\n\n>     def generate_email_header(self):\n>         \"\"\"Generate the RFC 2822 email headers for this Change, a line at a time.\n> \n>         The output should not include the trailing blank line.\"\"\"\n> \n>         raise NotImplementedError()\n> \n>     def generate_email_intro(self):\n>         \"\"\"Generate the email intro for this Change, a line at a time.\n> \n>         The output will be used as the standard boilerplate at the top\n>         of the email body.\"\"\"\n> \n>         raise NotImplementedError()\n> \n>     def generate_email_body(self):\n>         \"\"\"Generate the main part of the email body, a line at a time.\n> \n>         The text in the body might be truncated after a specified\n>         number of lines (see multimailhook.emailmaxlines).\"\"\"\n> \n>         raise NotImplementedError()\n> \n>     def generate_email_footer(self):\n>         \"\"\"Generate the footer of the email, a line at a time.\n> \n>         The footer is always included, irrespective of\n>         multimailhook.emailmaxlines.\"\"\"\n> \n>         raise NotImplementedError()\n\nUnsure what these are about.\n\n>     def generate_email(self, push, body_filter=None):\n>         \"\"\"Generate an email describing this change.\n> \n>         Iterate over the lines (including the header lines) of an\n>         email describing this change.  If body_filter is not None,\n>         then use it to filter the lines that are intended for the\n>         email body.\"\"\"\n> \n>         for line in self.generate_email_header():\n>             yield line\n>         yield '\\n'\n>         for line in self.generate_email_intro():\n>             yield line\n> \n>         body = self.generate_email_body(push)\n>         if body_filter is not None:\n\nRedundant \"is not None\".\n\n>             body = body_filter(body)\n>         for line in body:\n>             yield line\n> \n>         for line in self.generate_email_footer():\n>             yield line\n\nNicely done with yield.\n\n> class Revision(Change):\n>     \"\"\"A Change consisting of a single git commit.\"\"\"\n> \n>     def __init__(self, reference_change, rev, num, tot):\n>         Change.__init__(self, reference_change.environment)\n\nsuper?\n\n>         self.reference_change = reference_change\n>         self.rev = rev\n>         self.change_type = self.reference_change.change_type\n>         self.refname = self.reference_change.refname\n>         self.num = num\n>         self.tot = tot\n>         self.author = read_git_output(['log', '--no-walk', '--format=%aN <%aE>', self.rev.sha1])\n\nDetermining author using log --format.  Okay.\n\n>         self.recipients = self.environment.get_revision_recipients(self)\n> \n>     def _compute_values(self):\n>         values = Change._compute_values(self)\n\nsuper.\n\n>         # First line of commit message:\n>         try:\n>             oneline = read_git_output(\n>                 ['log', '--format=%s', '--no-walk', self.rev.sha1]\n>                 )\n>         except CommandError:\n>             oneline = self.rev.sha1\n\nWhat does this mean?  When will you get a CommandError?  And how do\nyou respond to it?\n\n>         values['rev'] = self.rev.sha1\n>         values['rev_short'] = self.rev.short\n>         values['change_type'] = self.change_type\n>         values['refname'] = self.refname\n>         values['short_refname'] = self.reference_change.short_refname\n>         values['refname_type'] = self.reference_change.refname_type\n>         values['reply_to_msgid'] = self.reference_change.msgid\n>         values['num'] = self.num\n>         values['tot'] = self.tot\n>         values['recipients'] = self.recipients\n>         values['oneline'] = oneline\n>         values['author'] = self.author\n\nUgh.  Use\n\n  { rev: self.rev.sha1,\n    rev_short: self.rev.short\n    ...\n  }\n\nand merge it with the existing dictionary.  Unsure why you're building\na dictionary in the first place.\n\n>     def generate_email_header(self):\n>         for line in self.expand_header_lines(REVISION_HEADER_TEMPLATE):\n>             yield line\n> \n>     def generate_email_intro(self):\n>         for line in self.expand_lines(REVISION_INTRO_TEMPLATE):\n>             yield line\n>\n>     def generate_email_body(self, push):\n>         \"\"\"Show this revision.\"\"\"\n> \n>         return read_git_lines(\n>             [\n>                 'log', '-C',\n>                  '--stat', '-p', '--cc',\n>                 '-1', self.rev.sha1,\n>                 ],\n>             keepends=True,\n>             )\n>\n>     def generate_email_footer(self):\n>         return self.expand_lines(REVISION_FOOTER_TEMPLATE)\n\nOkay.\n\n> class ReferenceChange(Change):\n>     \"\"\"A Change to a Git reference.\n> \n>     An abstract class representing a create, update, or delete of a\n>     Git reference.  Derived classes handle specific types of reference\n>     (e.g., tags vs. branches).  These classes generate the main\n>     reference change email summarizing the reference change and\n>     whether it caused any any commits to be added or removed.\n> \n>     ReferenceChange objects are usually created using the static\n>     create() method, which has the logic to decide which derived class\n>     to instantiate.\"\"\"\n> \n>     REF_RE = re.compile(r'^refs\\/(?P<area>[^\\/]+)\\/(?P<shortname>.*)$')\n\nOkay.\n\n>     @staticmethod\n\nUnsure what such a huge static method is doing here, but we'll find\nout soon enough.\n\n>     def create(environment, oldrev, newrev, refname):\n>         \"\"\"Return a ReferenceChange object representing the change.\n> \n>         Return an object that represents the type of change that is being\n>         made. oldrev and newrev should be SHA1s or ZEROS.\"\"\"\n\nLike I said before, use the typesystem effectively: why is using a\nstring with 40 zeros somehow better than None in your program _logic_?\nI can understand converting None to 40 zeros for display purposes.\n\n>         old = GitObject(oldrev)\n>         new = GitObject(newrev)\n>         rev = new or old\n> \n>         # The revision type tells us what type the commit is, combined with\n>         # the location of the ref we can decide between\n>         #  - working branch\n>         #  - tracking branch\n>         #  - unannotated tag\n>         #  - annotated tag\n\nCould be simpler.\n\n>         return klass(\n>             environment,\n>             refname=refname, short_refname=short_refname,\n>             old=old, new=new, rev=rev,\n>             )\n\nEverything inherits from ReferenceChange anyway, so it should be safe.\n\n>     def __init__(self, environment, refname, short_refname, old, new, rev):\n>         Change.__init__(self, environment)\n>         self.change_type = {\n>             (False, True) : 'create',\n>             (True, True) : 'update',\n>             (True, False) : 'delete',\n>             }[bool(old), bool(new)]\n\nAs a general principle, avoid casting: if new is a dictionary, what\ndoes bool(new) even mean?  You just have to trust types, and let go of\nthat much safety.\n\n>     def get_subject(self):\n>         template = {\n>             'create' : REF_CREATED_SUBJECT_TEMPLATE,\n>             'update' : REF_UPDATED_SUBJECT_TEMPLATE,\n>             'delete' : REF_DELETED_SUBJECT_TEMPLATE,\n>             }[self.change_type]\n>         return self.expand(template)\n> \n>     def generate_email_header(self):\n>         for line in self.expand_header_lines(\n>             REFCHANGE_HEADER_TEMPLATE, subject=self.get_subject(),\n>             ):\n>             yield line\n> \n>     def generate_email_intro(self):\n>         for line in self.expand_lines(REFCHANGE_INTRO_TEMPLATE):\n>             yield line\n> \n>     def generate_email_body(self, push):\n>         \"\"\"Call the appropriate body-generation routine.\n> \n>         Call one of generate_create_summary() /\n>         generate_update_summary() / generate_delete_summary().\"\"\"\n> \n>         change_summary = {\n>             'create' : self.generate_create_summary,\n>             'delete' : self.generate_delete_summary,\n>             'update' : self.generate_update_summary,\n>             }[self.change_type](push)\n>         for line in change_summary:\n>             yield line\n> \n>         for line in self.generate_revision_change_summary(push):\n>             yield line\n> \n>     def generate_email_footer(self):\n>         return self.expand_lines(FOOTER_TEMPLATE)\n\nMostly boring string interpolation.  Okay.\n\n>     def generate_revision_change_log(self, new_commits_list):\n>         if self.showlog:\n>             yield '\\n'\n>             yield 'Detailed log of new commits:\\n\\n'\n>             for line in read_git_lines(\n>                     ['log', '--no-walk']\n>                     + self.logopts\n>                     + new_commits_list\n>                     + ['--'],\n>                     keepends=True,\n>                 ):\n>                 yield line\n\nOkay.\n\nGot bored.  Skipping to the next class.\n\n> class BranchChange(ReferenceChange):\n>     refname_type = 'branch'\n\nUnsure what new information this conveys over the type.\n\n> class AnnotatedTagChange(ReferenceChange):\n>     refname_type = 'annotated tag'\n> \n>     def __init__(self, environment, refname, short_refname, old, new, rev):\n>         ReferenceChange.__init__(\n>             self, environment,\n>             refname=refname, short_refname=short_refname,\n>             old=old, new=new, rev=rev,\n>             )\n>         self.recipients = environment.get_announce_recipients(self)\n>         self.show_shortlog = environment.announce_show_shortlog\n> \n>     ANNOTATED_TAG_FORMAT = (\n>         '%(*objectname)\\n'\n>         '%(*objecttype)\\n'\n>         '%(taggername)\\n'\n>         '%(taggerdate)'\n>         )\n\nNow I'm curious what you get by differentiating between annotated and\nunannotated tags.\n\n>     def describe_tag(self, push):\n>         \"\"\"Describe the new value of an annotated tag.\"\"\"\n> \n>         # Use git for-each-ref to pull out the individual fields from\n>         # the tag\n>         [tagobject, tagtype, tagger, tagged] = read_git_lines(\n>             ['for-each-ref', '--format=%s' % (self.ANNOTATED_TAG_FORMAT,), self.refname],\n>             )\n\nYou could've saved yourself a lot of trouble by running one f-e-r on\nrefs/tags and filtering that.  I don't know what you're gaining from\nthis overzealous object-orientation.\n\n>         yield self.expand(\n>             BRIEF_SUMMARY_TEMPLATE, action='tagging',\n>             rev_short=tagobject, text='(%s)' % (tagtype,),\n>             )\n>         if tagtype == 'commit':\n>             # If the tagged object is a commit, then we assume this is a\n>             # release, and so we calculate which tag this tag is\n>             # replacing\n>             try:\n>                 prevtag = read_git_output(['describe', '--abbrev=0', '%s^' % (self.new,)])\n>             except CommandError:\n>                 prevtag = None\n>             if prevtag:\n>                 yield '  replaces  %s\\n' % (prevtag,)\n>         else:\n>             prevtag = None\n>             yield '    length  %s bytes\\n' % (read_git_output(['cat-file', '-s', tagobject]),)\n> \n>         yield ' tagged by  %s\\n' % (tagger,)\n>         yield '        on  %s\\n' % (tagged,)\n>         yield '\\n'\n\nOkay, this information isn't present in an unannotated tag.  So you\ndifferentiate to exploit the additional information you can get.\n\n>         # Show the content of the tag message; this might contain a\n>         # change log or release notes so is worth displaying.\n>         yield LOGBEGIN\n>         contents = list(read_git_lines(['cat-file', 'tag', self.new.sha1], keepends=True))\n\nYou could've easily batched this.\n\n>         contents = contents[contents.index('\\n') + 1:]\n>         if contents and contents[-1][-1:] != '\\n':\n>             contents.append('\\n')\n>         for line in contents:\n>             yield line\n> \n>         if self.show_shortlog and tagtype == 'commit':\n>             # Only commit tags make sense to have rev-list operations\n>             # performed on them\n>             yield '\\n'\n>             if prevtag:\n>                 # Show changes since the previous release\n>                 revlist = read_git_output(\n>                     ['rev-list', '--pretty=short', '%s..%s' % (prevtag, self.new,)],\n>                     keepends=True,\n>                     )\n>             else:\n>                 # No previous tag, show all the changes since time\n>                 # began\n>                 revlist = read_git_output(\n>                     ['rev-list', '--pretty=short', '%s' % (self.new,)],\n>                     keepends=True,\n>                     )\n>             for line in read_git_lines(['shortlog'], input=revlist, keepends=True):\n>                 yield line\n> \n>         yield LOGEND\n>         yield '\\n'\n\nWay too many git invocations, I think.\n\n> class OtherReferenceChange(ReferenceChange):\n>     refname_type = 'reference'\n> \n>     def __init__(self, environment, refname, short_refname, old, new, rev):\n>         # We use the full refname as short_refname, because otherwise\n>         # the full name of the reference would not be obvious from the\n>         # text of the email.\n>         ReferenceChange.__init__(\n>             self, environment,\n>             refname=refname, short_refname=refname,\n>             old=old, new=new, rev=rev,\n>             )\n>         self.recipients = environment.get_refchange_recipients(self)\n\nWhat is the point of this?  Why not just use ReferenceChange directly?\n\n> class Mailer(object):\n>     \"\"\"An object that can send emails.\"\"\"\n> \n>     def send(self, lines, to_addrs):\n>         \"\"\"Send an email consisting of lines.\n> \n>         lines must be an iterable over the lines constituting the\n>         header and body of the email.  to_addrs is a list of recipient\n>         addresses (can be needed even if lines already contains a\n>         \"To:\" field).  It can be either a string (comma-separated list\n>         of email addresses) or a Python list of individual email\n>         addresses.\n> \n>         \"\"\"\n> \n>         raise NotImplementedError()\n\nAbstract base class (abc)?  Or do you want to support Python <2.6?\n\n> class SendMailer(Mailer):\n>     \"\"\"Send emails using '/usr/sbin/sendmail -t'.\"\"\"\n> \n>     def __init__(self, command=None, envelopesender=None):\n>         \"\"\"Construct a SendMailer instance.\n> \n>         command should be the command and arguments used to invoke\n>         sendmail, as a list of strings.  If an envelopesender is\n>         provided, it will also be passed to the command, via '-f\n>         envelopesender'.\"\"\"\n> \n>         if command:\n>             self.command = command[:]\n>         else:\n>             self.command = ['/usr/sbin/sendmail', '-t']\n\nIf you want to DWIM when the configuration variable is missing, do it\nfully using a list of good candidates like /usr/lib/sendmail,\n/usr/sbin/sendmail, /usr/ucblib/sendmail, /usr/bin/msmtp.  Also, what\nhappened to our faithful 'git send-email' Perl script?  Isn't that\nmost likely to be installed?\n\n>         if envelopesender:\n>             self.command.extend(['-f', envelopesender])\n> \n>     def send(self, lines, to_addrs):\n>         try:\n>             p = subprocess.Popen(self.command, stdin=subprocess.PIPE)\n>         except OSError, e:\n>             sys.stderr.write(\n>                 '*** Cannot execute command: %s\\n' % ' '.join(self.command)\n>                 + '*** %s\\n' % str(e)\n>                 + '*** Try setting multimailhook.mailer to \"smtp\"\\n'\n>                 '*** to send emails without using the sendmail command.\\n'\n>                 )\n>             sys.exit(1)\n\nWhy do you need to concatenate strings using +?  This can take a list of strings, no?\n\n> class SMTPMailer(Mailer):\n>     \"\"\"Send emails using Python's smtplib.\"\"\"\n> \n>     def __init__(self, envelopesender, smtpserver):\n>         if not envelopesender:\n>             sys.stderr.write(\n>                 'fatal: git_multimail: cannot use SMTPMailer without a sender address.\\n'\n>                 'please set either multimailhook.envelopeSender or user.email\\n'\n>                 )\n>             sys.exit(1)\n>         self.envelopesender = envelopesender\n>         self.smtpserver = smtpserver\n>         try:\n>             self.smtp = smtplib.SMTP(self.smtpserver)\n>         except Exception, e:\n>             sys.stderr.write('*** Error establishing SMTP connection to %s***\\n' % self.smtpserver)\n>             sys.stderr.write('*** %s\\n' % str(e))\n>             sys.exit(1)\n\nLet's hope Python's smtplib is robust.\n\n>     def __del__(self):\n>         self.smtp.quit()\n\nSo you close the connection when the object is destroyed by the GC.\n\n>     def send(self, lines, to_addrs):\n>         try:\n>             msg = ''.join(lines)\n>             # turn comma-separated list into Python list if needed.\n>             if isinstance(to_addrs, basestring):\n>                 to_addrs = [email for (name, email) in getaddresses([to_addrs])]\n>             self.smtp.sendmail(self.envelopesender, to_addrs, msg)\n>         except Exception, e:\n>             sys.stderr.write('*** Error sending email***\\n')\n>             sys.stderr.write('*** %s\\n' % str(e))\n>             self.smtp.quit()\n>             sys.exit(1)\n\nOkay.\n\n> class OutputMailer(Mailer):\n>     \"\"\"Write emails to an output stream, bracketed by lines of '=' characters.\n> \n>     This is intended for debugging purposes.\"\"\"\n> \n>     SEPARATOR = '=' * 75 + '\\n'\n> \n>     def __init__(self, f):\n>         self.f = f\n> \n>     def send(self, lines, to_addrs):\n>         self.f.write(self.SEPARATOR)\n>         self.f.writelines(lines)\n>         self.f.write(self.SEPARATOR)\n\nUnsure what this is.\n\n> def get_git_dir():\n>     \"\"\"Determine GIT_DIR.\n> \n>     Determine GIT_DIR either from the GIT_DIR environment variable or\n>     from the working directory, using Git's usual rules.\"\"\"\n> \n>     try:\n>         return read_git_output(['rev-parse', '--git-dir'])\n>     except CommandError:\n>         sys.stderr.write('fatal: git_multimail: not in a git working copy\\n')\n>         sys.exit(1)\n\nWhy do you need a working copy?  Will a bare repository not suffice?\n\n> class Environment(object):\n\nNew-style class.  I wonder why you suddenly switched.\n\n>     REPO_NAME_RE = re.compile(r'^(?P<name>.+?)(?:\\.git)$')\n> \n>     def __init__(self, osenv=None):\n>         self.osenv = osenv or os.environ\n>         self.announce_show_shortlog = False\n>         self.maxcommitemails = 500\n>         self.diffopts = ['--stat', '--summary', '--find-copies-harder']\n>         self.logopts = []\n>         self.refchange_showlog = False\n> \n>         self.COMPUTED_KEYS = [\n>             'administrator',\n>             'charset',\n>             'emailprefix',\n>             'fromaddr',\n>             'pusher',\n>             'pusher_email',\n>             'repo_path',\n>             'repo_shortname',\n>             'sender',\n>             ]\n> \n>         self._values = None\n\nOkay.\n\n> [...]\n\nSeems to be some boilerplate thing.  I'll skip to the next class.\n\n> class ConfigEnvironmentMixin(Environment):\n>     \"\"\"A mixin that sets self.config to its constructor's config argument.\n> \n>     This class's constructor consumes the \"config\" argument.\n> \n>     Mixins that need to inspect the config should inherit from this\n>     class (1) to make sure that \"config\" is still in the constructor\n>     arguments with its own constructor runs and/or (2) to be sure that\n>     self.config is set after construction.\"\"\"\n> \n>     def __init__(self, config, **kw):\n>         super(ConfigEnvironmentMixin, self).__init__(**kw)\n>         self.config = config\n\nOverdoing the OO factories, much?\n\nI'll skip a few boring factory classes.\n\n> class GenericEnvironment(\n>     ProjectdescEnvironmentMixin,\n>     ConfigMaxlinesEnvironmentMixin,\n>     ConfigFilterLinesEnvironmentMixin,\n>     ConfigRecipientsEnvironmentMixin,\n>     PusherDomainEnvironmentMixin,\n>     ConfigOptionsEnvironmentMixin,\n>     GenericEnvironmentMixin,\n>     Environment,\n>     ):\n>     pass\n\nSigh.  I might as well be reading some Java now :/\n\nSorry, I'm exhausted.\n\nLet's take a step back and look at what this gigantic script is doing.\nIt uses the information from a push to string-interpolate a template\nand generate emails, right?  The rest of the script is about churning\non the updated refs to prettify the emails.\n\n>From my quick reading, it seems to be unnecessarily complicated and\ninefficient.  Why are there so many factories, and why do you call out\nto git at every opportunity, instead of cleanly separating computation\nfrom rendering?\n"},{"id":"222402","messageId":"20130702205143.GC9161@serenity.lan","threadId":"34326","inReplyTo":"1372793019-12162-1-git-send-email-artagnon@gmail.com","subject":"Re: Review of git multimail","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-07-02T20:51:43Z","receivedAt":"2013-07-02T20:51:43Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Jul 03, 2013 at 12:53:39AM +0530, Ramkumar Ramachandra wrote:\n> > class CommandError(Exception):\n> >     def __init__(self, cmd, retcode):\n> >         self.cmd = cmd\n> >         self.retcode = retcode\n> >         Exception.__init__(\n> >             self,\n> >             'Command \"%s\" failed with retcode %s' % (' '.join(cmd), retcode,)\n> \n> So cmd is a list.\n> \n> > class ConfigurationException(Exception):\n> >     pass\n> \n> Dead code?\n\nHuh?  ConfigurationException is used elsewhere.\n\n> > def read_git_output(args, input=None, keepends=False, **kw):\n> >     \"\"\"Read the output of a Git command.\"\"\"\n> > \n> >     return read_output(\n> >         ['git', '-c', 'i18n.logoutputencoding=%s' % (ENCODING,)] + args,\n> >         input=input, keepends=keepends, **kw\n> >         )\n> \n> Okay, although I'm wondering what i18n.logoutputencoding has to do with anything.\n\nMaking sure the output is what's expected and not influenced by\nenvironment variables?\n\n> > def read_output(cmd, input=None, keepends=False, **kw):\n> >     if input:\n> >         stdin = subprocess.PIPE\n> >     else:\n> >         stdin = None\n> >     p = subprocess.Popen(\n> >         cmd, stdin=stdin, stdout=subprocess.PIPE, stderr=subprocess.PIPE, **kw\n> >         )\n> >     (out, err) = p.communicate(input)\n> >     retcode = p.wait()\n> >     if retcode:\n> >         raise CommandError(cmd, retcode)\n> >     if not keepends:\n> >         out = out.rstrip('\\n\\r')\n> >     return out\n> \n> Helper function that serves a single caller, read_git_output().\n> \n> > def read_git_lines(args, keepends=False, **kw):\n> >     \"\"\"Return the lines output by Git command.\n> > \n> >     Return as single lines, with newlines stripped off.\"\"\"\n> > \n> >     return read_git_output(args, keepends=True, **kw).splitlines(keepends)\n> \n> Okay.\n> \n> > class Config(object):\n> >     def __init__(self, section, git_config=None):\n> >         \"\"\"Represent a section of the git configuration.\n> > \n> >         If git_config is specified, it is passed to \"git config\" in\n> >         the GIT_CONFIG environment variable, meaning that \"git config\"\n> >         will read the specified path rather than the Git default\n> >         config paths.\"\"\"\n> > \n> >         self.section = section\n> >         if git_config:\n> >             self.env = os.environ.copy()\n> >             self.env['GIT_CONFIG'] = git_config\n> >         else:\n> >             self.env = None\n> \n> Okay.\n> \n> >     @staticmethod\n> >     def _split(s):\n> >         \"\"\"Split NUL-terminated values.\"\"\"\n> > \n> >         words = s.split('\\0')\n> >         assert words[-1] == ''\n> >         return words[:-1]\n> \n> Ugh.  Two callers of this poorly-defined static method: I wonder if\n> we'd be better off inlining it.\n\nIn what way poorly defined?  Personally I'd make it _split_null at the\ntop level but it seems sensible.\n\n> >     def get(self, name, default=''):\n> >         try:\n> >             values = self._split(read_git_output(\n> >                     ['config', '--get', '--null', '%s.%s' % (self.section, name)],\n> >                     env=self.env, keepends=True,\n> >                     ))\n> \n> Wait, what is the point of using --null and then splitting by hand\n> using a poorly-defined static method?  Why not drop the --null and\n> splitlines() as usual?\n> \n> >             assert len(values) == 1\n> \n> When does this assert fail?\n\nIn can't, which is presumably why it's an assert - it checks that we're\nprocessing the Git output as expected.\n\n> >             return values[0]\n> >         except CommandError:\n> >             return default\n> \n> If you're emulating the dictionary get method, default=None.  This is\n> not C, where all codepaths of the function must return the same type.\n> \n> >     def get_bool(self, name, default=None):\n> >         try:\n> >             value = read_git_output(\n> >                 ['config', '--get', '--bool', '%s.%s' % (self.section, name)],\n> >                 env=self.env,\n> >                 )\n> >         except CommandError:\n> >             return default\n> >         return value == 'true'\n> \n> Correct.  On success, return bool.  On failure, return None.\n> \n> >     def get_all(self, name, default=None):\n> >         \"\"\"Read a (possibly multivalued) setting from the configuration.\n> > \n> >         Return the result as a list of values, or default if the name\n> >         is unset.\"\"\"\n> > \n> >         try:\n> >             return self._split(read_git_output(\n> >                 ['config', '--get-all', '--null', '%s.%s' % (self.section, name)],\n> >                 env=self.env, keepends=True,\n> >                 ))\n> >         except CommandError, e:\n> \n> CommandError as e?\n\nNot before Python 2.6.\n\n> >             if e.retcode == 1:\n> \n> What does this cryptic retcode mean?\n\nIt mirrors subprocess.CalledProcessError, retcode is the return code of\nthe process.\n\n> >                 return default\n> >             else:\n> >                 raise\n> \n> raise what?\n\nThe current exception - this is pretty idiomatic Python.\n\n> You've instantiated the Config class in two places: user and\n> multimailhook sections.  Considering that you're going to read all the\n> keys in that section, why not --get-regexp, pre-load the configuration\n> into a dictionary and refer to that instead of spawning 'git config'\n> every time you need a configuration value?\n> \n> >     def get_recipients(self, name, default=None):\n> >         \"\"\"Read a recipients list from the configuration.\n> > \n> >         Return the result as a comma-separated list of email\n> >         addresses, or default if the option is unset.  If the setting\n> >         has multiple values, concatenate them with comma separators.\"\"\"\n> > \n> >         lines = self.get_all(name, default=None)\n> >         if lines is None:\n> >             return default\n> >         return ', '.join(line.strip() for line in lines)\n> \n> Ugh.\n\nWhat's so bad here?  This seems pretty clear to me.\n\n> >     def set(self, name, value):\n> >         read_git_output(\n> >             ['config', '%s.%s' % (self.section, name), value],\n> >             env=self.env,\n> >             )\n> >\n> >     def add(self, name, value):\n> >         read_git_output(\n> >             ['config', '--add', '%s.%s' % (self.section, name), value],\n> >             env=self.env,\n> >             )\n> > \n> >     def has_key(self, name):\n> >         return self.get_all(name, default=None) is not None\n> > \n> >     def unset_all(self, name):\n> >         try:\n> >             read_git_output(\n> >                 ['config', '--unset-all', '%s.%s' % (self.section, name)],\n> >                 env=self.env,\n> >                 )\n> >         except CommandError, e:\n> >             if e.retcode == 5:\n> >                 # The name doesn't exist, which is what we wanted anyway...\n> >                 pass\n> >             else:\n> >                 raise\n> > \n> >     def set_recipients(self, name, value):\n> >         self.unset_all(name)\n> >         for pair in getaddresses([value]):\n> >             self.add(name, formataddr(pair))\n> \n> Dead code?\n> \n> > def generate_summaries(*log_args):\n> >     \"\"\"Generate a brief summary for each revision requested.\n> > \n> >     log_args are strings that will be passed directly to \"git log\" as\n> >     revision selectors.  Iterate over (sha1_short, subject) for each\n> >     commit specified by log_args (subject is the first line of the\n> >     commit message as a string without EOLs).\"\"\"\n> > \n> >     cmd = [\n> >         'log', '--abbrev', '--format=%h %s',\n> >         ] + list(log_args) + ['--']\n> \n> What is log_args if not a list?\n\nGiven the function signature, how can it not be a list?\n\n> But yeah, log is the best way to generate summaries.\n> \n> >     for line in read_git_lines(cmd):\n> >         yield tuple(line.split(' ', 1))\n> \n> Okay, let's see how you use this iterator.\n\n[technically a generator...]\n\n> > def limit_lines(lines, max_lines):\n> >     for (index, line) in enumerate(lines):\n> >         if index < max_lines:\n> >             yield line\n> > \n> >     if index >= max_lines:\n> >         yield '... %d lines suppressed ...\\n' % (index + 1 - max_lines,)\n> \n> Random helper.\n> \n> > def limit_linelength(lines, max_linelength):\n> >     for line in lines:\n> >         # Don't forget that lines always include a trailing newline.\n> >         if len(line) > max_linelength + 1:\n> >             line = line[:max_linelength - 7] + ' [...]\\n'\n> >         yield line\n> \n> Random helper.\n> \n> > class GitObject(object):\n> >     def __init__(self, sha1, type=None):\n> >         if sha1 == ZEROS:\n> >             self.sha1 = self.type = self.commit = None\n> >         else:\n> >             self.sha1 = sha1\n> >             self.type = type or read_git_output(['cat-file', '-t', self.sha1])\n> > \n> >             if self.type == 'commit':\n> >                 self.commit = self\n> >             elif self.type == 'tag':\n> >                 try:\n> >                     self.commit = GitObject(\n> >                         read_git_output(['rev-parse', '--verify', '%s^0' % (self.sha1,)]),\n> >                         type='commit',\n> >                         )\n> >                 except CommandError:\n> >                     self.commit = None\n> >             else:\n> >                 self.commit = None\n> > \n> >         self.short = read_git_output(['rev-parse', '--short', sha1])\n> \n> Just rev-parse --verify --short $SHA1^0: if it resolves, set\n> self.short; one liner?\n\nBecause it can't create the recursive tag structure if it does that?\nDid you miss the fact the in the 'tag' case it assigns a new GitObject\nto self.commit?\n\n> >     def get_summary(self):\n> >         \"\"\"Return (sha1_short, subject) for this commit.\"\"\"\n> > \n> >         if not self.sha1:\n> >             raise ValueError('Empty commit has no summary')\n> \n> What is the point of letting the user instantiate a GitObject without\n> a valid .sha1 in the first place?\n\nIt looks like this is used for input from the hook, in which case it's\npossible for either newrev or oldref to be the null SHA-1, which creates\nthis empty object.\n\n> >         return iter(generate_summaries('--no-walk', self.sha1)).next()\n> \n> Not exactly fond of this, but I don't have a concrete replacement at\n> the moment.\n> \n> >     def __eq__(self, other):\n> >         return isinstance(other, GitObject) and self.sha1 == other.sha1\n> > \n> >     def __hash__(self):\n> >         return hash(self.sha1)\n> > \n> >     def __nonzero__(self):\n> >         return bool(self.sha1)\n> \n> Okay.\n> \n> >     def __str__(self):\n> >         return self.sha1 or ZEROS\n> \n> I wonder what value this adds when .short is around.\n\nDebugging?  A friendlier API?  Having a oneline __str__ method isn't\nexactly expensive and it can make some things much simpler.\n\n> > class Change(object):\n> >     \"\"\"A Change that has been made to the Git repository.\n> > \n> >     Abstract class from which both Revisions and ReferenceChanges are\n> >     derived.  A Change knows how to generate a notification email\n> >     describing itself.\"\"\"\n> > \n> >     def __init__(self, environment):\n> >         self.environment = environment\n> >         self._values = None\n> > \n> >     def _compute_values(self):\n> >         \"\"\"Return a dictionary {keyword : expansion} for this Change.\n> > \n> >         Derived classes overload this method to add more entries to\n> >         the return value.  This method is used internally by\n> >         get_values().  The return value should always be a new\n> >         dictionary.\"\"\"\n> > \n> >         return self.environment.get_values()\n> \n> Why is this an \"internal function\"?  What is your criterion for\n> internal versus non-internal?\n> \n> >     def get_values(self, **extra_values):\n> >         \"\"\"Return a dictionary {keyword : expansion} for this Change.\n> > \n> >         Return a dictionary mapping keywords to the values that they\n> >         should be expanded to for this Change (used when interpolating\n> >         template strings).  If any keyword arguments are supplied, add\n> >         those to the return value as well.  The return value is always\n> >         a new dictionary.\"\"\"\n> > \n> >         if self._values is None:\n> >             self._values = self._compute_values()\n> > \n> >         values = self._values.copy()\n> >         if extra_values:\n> >             values.update(extra_values)\n> >         return values\n> \n> Unsure what this is about.\n> \n> >     def expand(self, template, **extra_values):\n> >         \"\"\"Expand template.\n> > \n> >         Expand the template (which should be a string) using string\n> >         interpolation of the values for this Change.  If any keyword\n> >         arguments are provided, also include those in the keywords\n> >         available for interpolation.\"\"\"\n> > \n> >         return template % self.get_values(**extra_values)\n> > \n> >     def expand_lines(self, template, **extra_values):\n> >         \"\"\"Break template into lines and expand each line.\"\"\"\n> > \n> >         values = self.get_values(**extra_values)\n> >         for line in template.splitlines(True):\n> >             yield line % values\n> \n> Okay.\n> \n> >     def expand_header_lines(self, template, **extra_values):\n> >         \"\"\"Break template into lines and expand each line as an RFC 2822 header.\n> > \n> >         Encode values and split up lines that are too long.  Silently\n> >         skip lines that contain references to unknown variables.\"\"\"\n> > \n> >         values = self.get_values(**extra_values)\n> >         for line in template.splitlines(True):\n> >             (name, value) = line.split(':', 1)\n> >             value = value.rstrip('\\n\\r')\n> \n> Doesn't splitlines() make the rstrip() redundant?\n> \n> >             try:\n> >                 value = value % values\n> >             except KeyError, e:\n> >                 if DEBUG:\n> >                     sys.stderr.write(\n> >                         'Warning: unknown variable %r in the following line; line skipped:\\n'\n> >                         '    %s'\n> >                         % (e.args[0], line,)\n> >                         )\n> \n> If DEBUG isn't on, you risk leaving the value string interpolated\n> without even telling the user.  What does it mean to the end user?\n> \n> >             else:\n> >                 try:\n> >                     h = Header(value, header_name=name)\n> >                 except UnicodeDecodeError:\n> >                     h = Header(value, header_name=name, charset=CHARSET, errors='replace')\n> >                 for splitline in ('%s: %s\\n' % (name, h.encode(),)).splitlines(True):\n> >                     yield splitline\n> \n> Not elated by this exception cascading, but I suppose it's cheaper\n> than actually checking everything.\n\nIt's generally considered more idiomatic in Python to use exceptions\nthan check explicitly.  In CPython the cost is roughly equivalent.\n\n> >     def generate_email_header(self):\n> >         \"\"\"Generate the RFC 2822 email headers for this Change, a line at a time.\n> > \n> >         The output should not include the trailing blank line.\"\"\"\n> > \n> >         raise NotImplementedError()\n> > \n> >     def generate_email_intro(self):\n> >         \"\"\"Generate the email intro for this Change, a line at a time.\n> > \n> >         The output will be used as the standard boilerplate at the top\n> >         of the email body.\"\"\"\n> > \n> >         raise NotImplementedError()\n> > \n> >     def generate_email_body(self):\n> >         \"\"\"Generate the main part of the email body, a line at a time.\n> > \n> >         The text in the body might be truncated after a specified\n> >         number of lines (see multimailhook.emailmaxlines).\"\"\"\n> > \n> >         raise NotImplementedError()\n> > \n> >     def generate_email_footer(self):\n> >         \"\"\"Generate the footer of the email, a line at a time.\n> > \n> >         The footer is always included, irrespective of\n> >         multimailhook.emailmaxlines.\"\"\"\n> > \n> >         raise NotImplementedError()\n> \n> Unsure what these are about.\n> \n> >     def generate_email(self, push, body_filter=None):\n> >         \"\"\"Generate an email describing this change.\n> > \n> >         Iterate over the lines (including the header lines) of an\n> >         email describing this change.  If body_filter is not None,\n> >         then use it to filter the lines that are intended for the\n> >         email body.\"\"\"\n> > \n> >         for line in self.generate_email_header():\n> >             yield line\n> >         yield '\\n'\n> >         for line in self.generate_email_intro():\n> >             yield line\n> > \n> >         body = self.generate_email_body(push)\n> >         if body_filter is not None:\n> \n> Redundant \"is not None\".\n\nWhy redundant?  body_filter could be non-None but evaluate as false and\n\"explicit is better than implicit\".\n\n> >             body = body_filter(body)\n> >         for line in body:\n> >             yield line\n> > \n> >         for line in self.generate_email_footer():\n> >             yield line\n> \n> Nicely done with yield.\n> \n> > class Revision(Change):\n> >     \"\"\"A Change consisting of a single git commit.\"\"\"\n> > \n> >     def __init__(self, reference_change, rev, num, tot):\n> >         Change.__init__(self, reference_change.environment)\n> \n> super?\n> \n> >         self.reference_change = reference_change\n> >         self.rev = rev\n> >         self.change_type = self.reference_change.change_type\n> >         self.refname = self.reference_change.refname\n> >         self.num = num\n> >         self.tot = tot\n> >         self.author = read_git_output(['log', '--no-walk', '--format=%aN <%aE>', self.rev.sha1])\n> \n> Determining author using log --format.  Okay.\n> \n> >         self.recipients = self.environment.get_revision_recipients(self)\n> > \n> >     def _compute_values(self):\n> >         values = Change._compute_values(self)\n> \n> super.\n> \n> >         # First line of commit message:\n> >         try:\n> >             oneline = read_git_output(\n> >                 ['log', '--format=%s', '--no-walk', self.rev.sha1]\n> >                 )\n> >         except CommandError:\n> >             oneline = self.rev.sha1\n> \n> What does this mean?  When will you get a CommandError?  And how do\n> you respond to it?\n> \n> >         values['rev'] = self.rev.sha1\n> >         values['rev_short'] = self.rev.short\n> >         values['change_type'] = self.change_type\n> >         values['refname'] = self.refname\n> >         values['short_refname'] = self.reference_change.short_refname\n> >         values['refname_type'] = self.reference_change.refname_type\n> >         values['reply_to_msgid'] = self.reference_change.msgid\n> >         values['num'] = self.num\n> >         values['tot'] = self.tot\n> >         values['recipients'] = self.recipients\n> >         values['oneline'] = oneline\n> >         values['author'] = self.author\n> \n> Ugh.  Use\n> \n>   { rev: self.rev.sha1,\n>     rev_short: self.rev.short\n>     ...\n>   }\n> \n> and merge it with the existing dictionary.  Unsure why you're building\n> a dictionary in the first place.\n> \n> >     def generate_email_header(self):\n> >         for line in self.expand_header_lines(REVISION_HEADER_TEMPLATE):\n> >             yield line\n> > \n> >     def generate_email_intro(self):\n> >         for line in self.expand_lines(REVISION_INTRO_TEMPLATE):\n> >             yield line\n> >\n> >     def generate_email_body(self, push):\n> >         \"\"\"Show this revision.\"\"\"\n> > \n> >         return read_git_lines(\n> >             [\n> >                 'log', '-C',\n> >                  '--stat', '-p', '--cc',\n> >                 '-1', self.rev.sha1,\n> >                 ],\n> >             keepends=True,\n> >             )\n> >\n> >     def generate_email_footer(self):\n> >         return self.expand_lines(REVISION_FOOTER_TEMPLATE)\n> \n> Okay.\n> \n> > class ReferenceChange(Change):\n> >     \"\"\"A Change to a Git reference.\n> > \n> >     An abstract class representing a create, update, or delete of a\n> >     Git reference.  Derived classes handle specific types of reference\n> >     (e.g., tags vs. branches).  These classes generate the main\n> >     reference change email summarizing the reference change and\n> >     whether it caused any any commits to be added or removed.\n> > \n> >     ReferenceChange objects are usually created using the static\n> >     create() method, which has the logic to decide which derived class\n> >     to instantiate.\"\"\"\n> > \n> >     REF_RE = re.compile(r'^refs\\/(?P<area>[^\\/]+)\\/(?P<shortname>.*)$')\n> \n> Okay.\n> \n> >     @staticmethod\n> \n> Unsure what such a huge static method is doing here, but we'll find\n> out soon enough.\n> \n> >     def create(environment, oldrev, newrev, refname):\n> >         \"\"\"Return a ReferenceChange object representing the change.\n> > \n> >         Return an object that represents the type of change that is being\n> >         made. oldrev and newrev should be SHA1s or ZEROS.\"\"\"\n> \n> Like I said before, use the typesystem effectively: why is using a\n> string with 40 zeros somehow better than None in your program _logic_?\n> I can understand converting None to 40 zeros for display purposes.\n> \n> >         old = GitObject(oldrev)\n> >         new = GitObject(newrev)\n> >         rev = new or old\n> > \n> >         # The revision type tells us what type the commit is, combined with\n> >         # the location of the ref we can decide between\n> >         #  - working branch\n> >         #  - tracking branch\n> >         #  - unannotated tag\n> >         #  - annotated tag\n> \n> Could be simpler.\n> \n> >         return klass(\n> >             environment,\n> >             refname=refname, short_refname=short_refname,\n> >             old=old, new=new, rev=rev,\n> >             )\n> \n> Everything inherits from ReferenceChange anyway, so it should be safe.\n> \n> >     def __init__(self, environment, refname, short_refname, old, new, rev):\n> >         Change.__init__(self, environment)\n> >         self.change_type = {\n> >             (False, True) : 'create',\n> >             (True, True) : 'update',\n> >             (True, False) : 'delete',\n> >             }[bool(old), bool(new)]\n> \n> As a general principle, avoid casting: if new is a dictionary, what\n> does bool(new) even mean?\n\nnonempty.\n\n>                            You just have to trust types, and let go of\n> that much safety.\n> \n> >     def get_subject(self):\n> >         template = {\n> >             'create' : REF_CREATED_SUBJECT_TEMPLATE,\n> >             'update' : REF_UPDATED_SUBJECT_TEMPLATE,\n> >             'delete' : REF_DELETED_SUBJECT_TEMPLATE,\n> >             }[self.change_type]\n> >         return self.expand(template)\n> > \n> >     def generate_email_header(self):\n> >         for line in self.expand_header_lines(\n> >             REFCHANGE_HEADER_TEMPLATE, subject=self.get_subject(),\n> >             ):\n> >             yield line\n> > \n> >     def generate_email_intro(self):\n> >         for line in self.expand_lines(REFCHANGE_INTRO_TEMPLATE):\n> >             yield line\n> > \n> >     def generate_email_body(self, push):\n> >         \"\"\"Call the appropriate body-generation routine.\n> > \n> >         Call one of generate_create_summary() /\n> >         generate_update_summary() / generate_delete_summary().\"\"\"\n> > \n> >         change_summary = {\n> >             'create' : self.generate_create_summary,\n> >             'delete' : self.generate_delete_summary,\n> >             'update' : self.generate_update_summary,\n> >             }[self.change_type](push)\n> >         for line in change_summary:\n> >             yield line\n> > \n> >         for line in self.generate_revision_change_summary(push):\n> >             yield line\n> > \n> >     def generate_email_footer(self):\n> >         return self.expand_lines(FOOTER_TEMPLATE)\n> \n> Mostly boring string interpolation.  Okay.\n> \n> >     def generate_revision_change_log(self, new_commits_list):\n> >         if self.showlog:\n> >             yield '\\n'\n> >             yield 'Detailed log of new commits:\\n\\n'\n> >             for line in read_git_lines(\n> >                     ['log', '--no-walk']\n> >                     + self.logopts\n> >                     + new_commits_list\n> >                     + ['--'],\n> >                     keepends=True,\n> >                 ):\n> >                 yield line\n> \n> Okay.\n> \n> Got bored.  Skipping to the next class.\n> \n> > class BranchChange(ReferenceChange):\n> >     refname_type = 'branch'\n> \n> Unsure what new information this conveys over the type.\n\nPresumably the \"refname_type\" member is used to provide more friendly\ntext in the message.\n\n> > class AnnotatedTagChange(ReferenceChange):\n> >     refname_type = 'annotated tag'\n> > \n> >     def __init__(self, environment, refname, short_refname, old, new, rev):\n> >         ReferenceChange.__init__(\n> >             self, environment,\n> >             refname=refname, short_refname=short_refname,\n> >             old=old, new=new, rev=rev,\n> >             )\n> >         self.recipients = environment.get_announce_recipients(self)\n> >         self.show_shortlog = environment.announce_show_shortlog\n> > \n> >     ANNOTATED_TAG_FORMAT = (\n> >         '%(*objectname)\\n'\n> >         '%(*objecttype)\\n'\n> >         '%(taggername)\\n'\n> >         '%(taggerdate)'\n> >         )\n> \n> Now I'm curious what you get by differentiating between annotated and\n> unannotated tags.\n> \n> >     def describe_tag(self, push):\n> >         \"\"\"Describe the new value of an annotated tag.\"\"\"\n> > \n> >         # Use git for-each-ref to pull out the individual fields from\n> >         # the tag\n> >         [tagobject, tagtype, tagger, tagged] = read_git_lines(\n> >             ['for-each-ref', '--format=%s' % (self.ANNOTATED_TAG_FORMAT,), self.refname],\n> >             )\n> \n> You could've saved yourself a lot of trouble by running one f-e-r on\n> refs/tags and filtering that.  I don't know what you're gaining from\n> this overzealous object-orientation.\n> \n> >         yield self.expand(\n> >             BRIEF_SUMMARY_TEMPLATE, action='tagging',\n> >             rev_short=tagobject, text='(%s)' % (tagtype,),\n> >             )\n> >         if tagtype == 'commit':\n> >             # If the tagged object is a commit, then we assume this is a\n> >             # release, and so we calculate which tag this tag is\n> >             # replacing\n> >             try:\n> >                 prevtag = read_git_output(['describe', '--abbrev=0', '%s^' % (self.new,)])\n> >             except CommandError:\n> >                 prevtag = None\n> >             if prevtag:\n> >                 yield '  replaces  %s\\n' % (prevtag,)\n> >         else:\n> >             prevtag = None\n> >             yield '    length  %s bytes\\n' % (read_git_output(['cat-file', '-s', tagobject]),)\n> > \n> >         yield ' tagged by  %s\\n' % (tagger,)\n> >         yield '        on  %s\\n' % (tagged,)\n> >         yield '\\n'\n> \n> Okay, this information isn't present in an unannotated tag.  So you\n> differentiate to exploit the additional information you can get.\n> \n> >         # Show the content of the tag message; this might contain a\n> >         # change log or release notes so is worth displaying.\n> >         yield LOGBEGIN\n> >         contents = list(read_git_lines(['cat-file', 'tag', self.new.sha1], keepends=True))\n> \n> You could've easily batched this.\n> \n> >         contents = contents[contents.index('\\n') + 1:]\n> >         if contents and contents[-1][-1:] != '\\n':\n> >             contents.append('\\n')\n> >         for line in contents:\n> >             yield line\n> > \n> >         if self.show_shortlog and tagtype == 'commit':\n> >             # Only commit tags make sense to have rev-list operations\n> >             # performed on them\n> >             yield '\\n'\n> >             if prevtag:\n> >                 # Show changes since the previous release\n> >                 revlist = read_git_output(\n> >                     ['rev-list', '--pretty=short', '%s..%s' % (prevtag, self.new,)],\n> >                     keepends=True,\n> >                     )\n> >             else:\n> >                 # No previous tag, show all the changes since time\n> >                 # began\n> >                 revlist = read_git_output(\n> >                     ['rev-list', '--pretty=short', '%s' % (self.new,)],\n> >                     keepends=True,\n> >                     )\n> >             for line in read_git_lines(['shortlog'], input=revlist, keepends=True):\n> >                 yield line\n> > \n> >         yield LOGEND\n> >         yield '\\n'\n> \n> Way too many git invocations, I think.\n> \n> > class OtherReferenceChange(ReferenceChange):\n> >     refname_type = 'reference'\n> > \n> >     def __init__(self, environment, refname, short_refname, old, new, rev):\n> >         # We use the full refname as short_refname, because otherwise\n> >         # the full name of the reference would not be obvious from the\n> >         # text of the email.\n> >         ReferenceChange.__init__(\n> >             self, environment,\n> >             refname=refname, short_refname=refname,\n> >             old=old, new=new, rev=rev,\n> >             )\n> >         self.recipients = environment.get_refchange_recipients(self)\n> \n> What is the point of this?  Why not just use ReferenceChange directly?\n> \n> > class Mailer(object):\n> >     \"\"\"An object that can send emails.\"\"\"\n> > \n> >     def send(self, lines, to_addrs):\n> >         \"\"\"Send an email consisting of lines.\n> > \n> >         lines must be an iterable over the lines constituting the\n> >         header and body of the email.  to_addrs is a list of recipient\n> >         addresses (can be needed even if lines already contains a\n> >         \"To:\" field).  It can be either a string (comma-separated list\n> >         of email addresses) or a Python list of individual email\n> >         addresses.\n> > \n> >         \"\"\"\n> > \n> >         raise NotImplementedError()\n> \n> Abstract base class (abc)?  Or do you want to support Python <2.6?\n> \n> > class SendMailer(Mailer):\n> >     \"\"\"Send emails using '/usr/sbin/sendmail -t'.\"\"\"\n> > \n> >     def __init__(self, command=None, envelopesender=None):\n> >         \"\"\"Construct a SendMailer instance.\n> > \n> >         command should be the command and arguments used to invoke\n> >         sendmail, as a list of strings.  If an envelopesender is\n> >         provided, it will also be passed to the command, via '-f\n> >         envelopesender'.\"\"\"\n> > \n> >         if command:\n> >             self.command = command[:]\n> >         else:\n> >             self.command = ['/usr/sbin/sendmail', '-t']\n> \n> If you want to DWIM when the configuration variable is missing, do it\n> fully using a list of good candidates like /usr/lib/sendmail,\n> /usr/sbin/sendmail, /usr/ucblib/sendmail, /usr/bin/msmtp.  Also, what\n> happened to our faithful 'git send-email' Perl script?  Isn't that\n> most likely to be installed?\n> \n> >         if envelopesender:\n> >             self.command.extend(['-f', envelopesender])\n> > \n> >     def send(self, lines, to_addrs):\n> >         try:\n> >             p = subprocess.Popen(self.command, stdin=subprocess.PIPE)\n> >         except OSError, e:\n> >             sys.stderr.write(\n> >                 '*** Cannot execute command: %s\\n' % ' '.join(self.command)\n> >                 + '*** %s\\n' % str(e)\n> >                 + '*** Try setting multimailhook.mailer to \"smtp\"\\n'\n> >                 '*** to send emails without using the sendmail command.\\n'\n> >                 )\n> >             sys.exit(1)\n> \n> Why do you need to concatenate strings using +?  This can take a list of strings, no?\n> \n> > class SMTPMailer(Mailer):\n> >     \"\"\"Send emails using Python's smtplib.\"\"\"\n> > \n> >     def __init__(self, envelopesender, smtpserver):\n> >         if not envelopesender:\n> >             sys.stderr.write(\n> >                 'fatal: git_multimail: cannot use SMTPMailer without a sender address.\\n'\n> >                 'please set either multimailhook.envelopeSender or user.email\\n'\n> >                 )\n> >             sys.exit(1)\n> >         self.envelopesender = envelopesender\n> >         self.smtpserver = smtpserver\n> >         try:\n> >             self.smtp = smtplib.SMTP(self.smtpserver)\n> >         except Exception, e:\n> >             sys.stderr.write('*** Error establishing SMTP connection to %s***\\n' % self.smtpserver)\n> >             sys.stderr.write('*** %s\\n' % str(e))\n> >             sys.exit(1)\n> \n> Let's hope Python's smtplib is robust.\n> \n> >     def __del__(self):\n> >         self.smtp.quit()\n> \n> So you close the connection when the object is destroyed by the GC.\n> \n> >     def send(self, lines, to_addrs):\n> >         try:\n> >             msg = ''.join(lines)\n> >             # turn comma-separated list into Python list if needed.\n> >             if isinstance(to_addrs, basestring):\n> >                 to_addrs = [email for (name, email) in getaddresses([to_addrs])]\n> >             self.smtp.sendmail(self.envelopesender, to_addrs, msg)\n> >         except Exception, e:\n> >             sys.stderr.write('*** Error sending email***\\n')\n> >             sys.stderr.write('*** %s\\n' % str(e))\n> >             self.smtp.quit()\n> >             sys.exit(1)\n> \n> Okay.\n> \n> > class OutputMailer(Mailer):\n> >     \"\"\"Write emails to an output stream, bracketed by lines of '=' characters.\n> > \n> >     This is intended for debugging purposes.\"\"\"\n> > \n> >     SEPARATOR = '=' * 75 + '\\n'\n> > \n> >     def __init__(self, f):\n> >         self.f = f\n> > \n> >     def send(self, lines, to_addrs):\n> >         self.f.write(self.SEPARATOR)\n> >         self.f.writelines(lines)\n> >         self.f.write(self.SEPARATOR)\n> \n> Unsure what this is.\n> \n> > def get_git_dir():\n> >     \"\"\"Determine GIT_DIR.\n> > \n> >     Determine GIT_DIR either from the GIT_DIR environment variable or\n> >     from the working directory, using Git's usual rules.\"\"\"\n> > \n> >     try:\n> >         return read_git_output(['rev-parse', '--git-dir'])\n> >     except CommandError:\n> >         sys.stderr.write('fatal: git_multimail: not in a git working copy\\n')\n> >         sys.exit(1)\n> \n> Why do you need a working copy?  Will a bare repository not suffice?\n> \n> > class Environment(object):\n> \n> New-style class.  I wonder why you suddenly switched.\n> \n> >     REPO_NAME_RE = re.compile(r'^(?P<name>.+?)(?:\\.git)$')\n> > \n> >     def __init__(self, osenv=None):\n> >         self.osenv = osenv or os.environ\n> >         self.announce_show_shortlog = False\n> >         self.maxcommitemails = 500\n> >         self.diffopts = ['--stat', '--summary', '--find-copies-harder']\n> >         self.logopts = []\n> >         self.refchange_showlog = False\n> > \n> >         self.COMPUTED_KEYS = [\n> >             'administrator',\n> >             'charset',\n> >             'emailprefix',\n> >             'fromaddr',\n> >             'pusher',\n> >             'pusher_email',\n> >             'repo_path',\n> >             'repo_shortname',\n> >             'sender',\n> >             ]\n> > \n> >         self._values = None\n> \n> Okay.\n> \n> > [...]\n> \n> Seems to be some boilerplate thing.  I'll skip to the next class.\n> \n> > class ConfigEnvironmentMixin(Environment):\n> >     \"\"\"A mixin that sets self.config to its constructor's config argument.\n> > \n> >     This class's constructor consumes the \"config\" argument.\n> > \n> >     Mixins that need to inspect the config should inherit from this\n> >     class (1) to make sure that \"config\" is still in the constructor\n> >     arguments with its own constructor runs and/or (2) to be sure that\n> >     self.config is set after construction.\"\"\"\n> > \n> >     def __init__(self, config, **kw):\n> >         super(ConfigEnvironmentMixin, self).__init__(**kw)\n> >         self.config = config\n> \n> Overdoing the OO factories, much?\n> \n> I'll skip a few boring factory classes.\n> \n> > class GenericEnvironment(\n> >     ProjectdescEnvironmentMixin,\n> >     ConfigMaxlinesEnvironmentMixin,\n> >     ConfigFilterLinesEnvironmentMixin,\n> >     ConfigRecipientsEnvironmentMixin,\n> >     PusherDomainEnvironmentMixin,\n> >     ConfigOptionsEnvironmentMixin,\n> >     GenericEnvironmentMixin,\n> >     Environment,\n> >     ):\n> >     pass\n> \n> Sigh.  I might as well be reading some Java now :/\n> \n> Sorry, I'm exhausted.\n> \n> Let's take a step back and look at what this gigantic script is doing.\n> It uses the information from a push to string-interpolate a template\n> and generate emails, right?  The rest of the script is about churning\n> on the updated refs to prettify the emails.\n> \n> From my quick reading, it seems to be unnecessarily complicated and\n> inefficient.  Why are there so many factories, and why do you call out\n> to git at every opportunity, instead of cleanly separating computation\n> from rendering?\n\nI thought the point of this was to produce a much more flexible way to\ngenerate emails when commits happen.  AFAICT all of the \"complexity\" is\nflexibility to make it easy to use this for new use cases.\n\nI have to say that I don't think this is a particularly useful review,\nyou seem to have skipped huge portions of the code and spent a lot of\ntime making us read your thought process rather than providing\nconstructive feedback.  What feedback there is mostly seems to be\nexpressions of disgust rather than actionable points.\n"},{"id":"222406","messageId":"CALkWK0=Hke+a9ebKYHZzo13gPTxcMSjVKFHtZa03WNgssEzvUA@mail.gmail.com","threadId":"34326","inReplyTo":"20130702205143.GC9161@serenity.lan","subject":"Re: Review of git multimail","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-07-02T21:34:19Z","receivedAt":"2013-07-02T21:34:19Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"John Keeping wrote:\n> I have to say that I don't think this is a particularly useful review,\n> you seem to have skipped huge portions of the code and spent a lot of\n> time making us read your thought process rather than providing\n> constructive feedback.  What feedback there is mostly seems to be\n> expressions of disgust rather than actionable points.\n\nWell, ignore it then.  I'm sorry for having wasted your time.\n\nI did what I could in the time I was willing to spend reading it.\n"},{"id":"222408","messageId":"7vsizwiowt.fsf@alter.siamese.dyndns.org","threadId":"34326","inReplyTo":"1372793019-12162-1-git-send-email-artagnon@gmail.com","subject":"Re: Review of git multimail","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-02T22:21:22Z","receivedAt":"2013-07-02T22:21:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n>>     def get(self, name, default=''):\n>>         try:\n>>             values = self._split(read_git_output(\n>>                     ['config', '--get', '--null', '%s.%s' % (self.section, name)],\n>>                     env=self.env, keepends=True,\n>>                     ))\n>\n> Wait, what is the point of using --null and then splitting by hand\n> using a poorly-defined static method?  Why not drop the --null and\n> splitlines() as usual?\n\nYou may actually have spotted a bug or misuse of \"--get\" here.\n\nWith this sample configuration:\n\n        $ cat >sample <<\\EOF\n        [a]\n                one = value\n                one = another\n\n        [b]\n                one = \"value\\nanother\"\n        EOF\n\nA script cannot differentiate between them without using '--null'.\n\n\t$ git config -f sample --get-all a.one\n        $ git config -f sample --get-all b.one\n\nBut that matters only when you use \"--get-all\", not \"--get\".  If\nthis method wants to make sure that the user did not misuse a.one\nas a multi-valued configuration variable, use of \"--null --get-all\"\nfollowed by checking how many items the command gives you back would\nbe a way to do so.\n"},{"id":"222424","messageId":"51D36BD8.1060909@alum.mit.edu","threadId":"34326","inReplyTo":"1372793019-12162-1-git-send-email-artagnon@gmail.com","subject":"Re: Review of git multimail","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-07-03T00:10:00Z","receivedAt":"2013-07-03T00:10:00Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 07/02/2013 09:23 PM, Ramkumar Ramachandra wrote:\n> I figured that we should quickly read through git-multimail and give\n> it an on-list review.  Hopefully, it'll educate the list about what\n> this is, and help improve the script itself.\n\nWonderful, thanks!\n\n> Sources: https://github.com/mhagger/git-multimail\n> \n> git_multimail.py wrote:\n>> #! /usr/bin/env python2\n> \n> Do all distributions ship it as python2 now?\n\nNo, but nor is \"python\" always Python version 2.x (I believe that Arch\nLinux now installs Python 3 as \"python\").  This topic was discussed here\n[1].  Basically, my reasoning is that (a) PEP 394 [2] says that\n\"python2\" is the correct name for a Python 2.x interpreter, (b) I\nbelieve that other distros are moving in that direction, and (c) if the\nscript says \"python2\" but no python2 is installed, the error is pretty\nobvious, whereas if the script says \"python\" and that is actually Python\n3.x, the error would be more cryptic.\n\n>> class CommandError(Exception):\n>>     def __init__(self, cmd, retcode):\n>>         self.cmd = cmd\n>>         self.retcode = retcode\n>>         Exception.__init__(\n>>             self,\n>>             'Command \"%s\" failed with retcode %s' % (' '.join(cmd), retcode,)\n> \n> So cmd is a list.\n\nYes, commands are always lists in my code because it is less error-prone\nthan trying to quote arguments correctly for the shell.  Do you think I\nshould document that here, or elsewhere, or everywhere, or ...?\n\n>> class ConfigurationException(Exception):\n>>     pass\n> \n> Dead code?\n\nNo, this defines an exception class that inherits all of its methods\n(including its constructors) from Exception.  This is useful because an\nexception of type ConfigurationException is distinguishable from other\ntypes of Exceptions, and can be caught using \"except\nConfigurationException, e\".\n\n>> def read_git_output(args, input=None, keepends=False, **kw):\n>>     \"\"\"Read the output of a Git command.\"\"\"\n>>\n>>     return read_output(\n>>         ['git', '-c', 'i18n.logoutputencoding=%s' % (ENCODING,)] + args,\n>>         input=input, keepends=keepends, **kw\n>>         )\n> \n> Okay, although I'm wondering what i18n.logoutputencoding has to do with anything.\n\nUltimately, a lot of the output of these commands is going to be\ninserted into an email that claims to be UTF-8, so this is here to\nhopefully avoid at least one source of non-UTF-8 text.\n\n> [...]\n>> class Config(object):\n>>     def __init__(self, section, git_config=None):\n>>         \"\"\"Represent a section of the git configuration.\n>>\n>>         If git_config is specified, it is passed to \"git config\" in\n>>         the GIT_CONFIG environment variable, meaning that \"git config\"\n>>         will read the specified path rather than the Git default\n>>         config paths.\"\"\"\n>>\n>>         self.section = section\n>>         if git_config:\n>>             self.env = os.environ.copy()\n>>             self.env['GIT_CONFIG'] = git_config\n>>         else:\n>>             self.env = None\n> \n> Okay.\n> \n>>     @staticmethod\n>>     def _split(s):\n>>         \"\"\"Split NUL-terminated values.\"\"\"\n>>\n>>         words = s.split('\\0')\n>>         assert words[-1] == ''\n>>         return words[:-1]\n> \n> Ugh.  Two callers of this poorly-defined static method: I wonder if\n> we'd be better off inlining it.\n> \n>>     def get(self, name, default=''):\n>>         try:\n>>             values = self._split(read_git_output(\n>>                     ['config', '--get', '--null', '%s.%s' % (self.section, name)],\n>>                     env=self.env, keepends=True,\n>>                     ))\n> \n> Wait, what is the point of using --null and then splitting by hand\n> using a poorly-defined static method?  Why not drop the --null and\n> splitlines() as usual?\n\nTo avoid confusion if a single config value contains end-of-line\ncharacters.  In this case we are using --get, so only a single value is\nallowed anyway, and presumably we could take the output and strip a\nsingle last '\\n' from it.  But null-terminated output is simply easier\nto handle in general and I don't see an advantage to avoiding its usage.\n\n>>             assert len(values) == 1\n> \n> When does this assert fail?\n\nIt shouldn't; treat this as a verification that everything is sane.  For\nexample if I had mistakenly used splitlines(), then the assertion could\nhave failed; without the assertion the next line would have masked the\nerror ;-)\n\n>>             return values[0]\n>>         except CommandError:\n>>             return default\n> \n> If you're emulating the dictionary get method, default=None.  This is\n> not C, where all codepaths of the function must return the same type.\n\nYou are right.  When I designed this class, I though that the empty\nstring would often be convenient.  But I just reviewed callers, and many\n(most?) of them explicitly set the default to None anyway.  I will\nchange this.\n\n> [...]\n>>     def get_all(self, name, default=None):\n>>         \"\"\"Read a (possibly multivalued) setting from the configuration.\n>>\n>>         Return the result as a list of values, or default if the name\n>>         is unset.\"\"\"\n>>\n>>         try:\n>>             return self._split(read_git_output(\n>>                 ['config', '--get-all', '--null', '%s.%s' % (self.section, name)],\n>>                 env=self.env, keepends=True,\n>>                 ))\n>>         except CommandError, e:\n> \n> CommandError as e?\n\nThe new syntax is not available before Python 2.6.  It will have to be\nchanged when we try to support Python 3.x, but until then the change\nwouldn't bring any benefits and would definitely prevent the script from\nrunning under Python 2.4 or 2.5 (which are currently supported).\n\n>>             if e.retcode == 1:\n> \n> What does this cryptic retcode mean?\n\nAccording to git-config(1), it means \"the section or key is invalid\" and\nempirically this is the error code you get when you try to read a key\nthat is not defined.  I will add a comment.\n\n>>                 return default\n>>             else:\n>>                 raise\n> \n> raise what?\n\nThis is the Python construct to re-throw the exception that was caught\nin the catch block containing it; i.e., the CommandError from a few\nlines earlier.\n\n> You've instantiated the Config class in two places: user and\n> multimailhook sections.  Considering that you're going to read all the\n> keys in that section, why not --get-regexp, pre-load the configuration\n> into a dictionary and refer to that instead of spawning 'git config'\n> every time you need a configuration value?\n\nYes, it's on my todo list.\n\n>>     def get_recipients(self, name, default=None):\n>>         \"\"\"Read a recipients list from the configuration.\n>>\n>>         Return the result as a comma-separated list of email\n>>         addresses, or default if the option is unset.  If the setting\n>>         has multiple values, concatenate them with comma separators.\"\"\"\n>>\n>>         lines = self.get_all(name, default=None)\n>>         if lines is None:\n>>             return default\n>>         return ', '.join(line.strip() for line in lines)\n> \n> Ugh.\n\n?\n\n>>     def set(self, name, value):\n>>         read_git_output(\n>>             ['config', '%s.%s' % (self.section, name), value],\n>>             env=self.env,\n>>             )\n>>\n>>     def add(self, name, value):\n>>         read_git_output(\n>>             ['config', '--add', '%s.%s' % (self.section, name), value],\n>>             env=self.env,\n>>             )\n>>\n>>     def has_key(self, name):\n>>         return self.get_all(name, default=None) is not None\n>>\n>>     def unset_all(self, name):\n>>         try:\n>>             read_git_output(\n>>                 ['config', '--unset-all', '%s.%s' % (self.section, name)],\n>>                 env=self.env,\n>>                 )\n>>         except CommandError, e:\n>>             if e.retcode == 5:\n>>                 # The name doesn't exist, which is what we wanted anyway...\n>>                 pass\n>>             else:\n>>                 raise\n>>\n>>     def set_recipients(self, name, value):\n>>         self.unset_all(name)\n>>         for pair in getaddresses([value]):\n>>             self.add(name, formataddr(pair))\n> \n> Dead code?\n\ngit_multimail is used as a library by migrate-mailhook-config, and that\nscript uses these methods.\n\n>> def generate_summaries(*log_args):\n>>     \"\"\"Generate a brief summary for each revision requested.\n>>\n>>     log_args are strings that will be passed directly to \"git log\" as\n>>     revision selectors.  Iterate over (sha1_short, subject) for each\n>>     commit specified by log_args (subject is the first line of the\n>>     commit message as a string without EOLs).\"\"\"\n>>\n>>     cmd = [\n>>         'log', '--abbrev', '--format=%h %s',\n>>         ] + list(log_args) + ['--']\n> \n> What is log_args if not a list?\n\nIt is a tuple and therefore needs to be converted to a list here.\n\n> But yeah, log is the best way to generate summaries.\n> \n> [...]\n>> class GitObject(object):\n>>     def __init__(self, sha1, type=None):\n>>         if sha1 == ZEROS:\n>>             self.sha1 = self.type = self.commit = None\n>>         else:\n>>             self.sha1 = sha1\n>>             self.type = type or read_git_output(['cat-file', '-t', self.sha1])\n>>\n>>             if self.type == 'commit':\n>>                 self.commit = self\n>>             elif self.type == 'tag':\n>>                 try:\n>>                     self.commit = GitObject(\n>>                         read_git_output(['rev-parse', '--verify', '%s^0' % (self.sha1,)]),\n>>                         type='commit',\n>>                         )\n>>                 except CommandError:\n>>                     self.commit = None\n>>             else:\n>>                 self.commit = None\n>>\n>>         self.short = read_git_output(['rev-parse', '--short', sha1])\n> \n> Just rev-parse --verify --short $SHA1^0: if it resolves, set\n> self.short; one liner?\n\nI don't follow.  We need both the long and the short SHA-1s to fill in\nthe templates.  What code exactly do you propose to replace with your\none-liner?\n\n>>     def get_summary(self):\n>>         \"\"\"Return (sha1_short, subject) for this commit.\"\"\"\n>>\n>>         if not self.sha1:\n>>             raise ValueError('Empty commit has no summary')\n> \n> What is the point of letting the user instantiate a GitObject without\n> a valid .sha1 in the first place?\n\n'0'*40 is passed to the post-receive script to indicate \"no object\"; for\nexample, a branch deletion is represented as\n\n0123456789abcdef0123456789abcdef01234567\n0000000000000000000000000000000000000000 refs/heads/branch\n\nIt is convenient to treat this as if it were a GitObject.\nGitObject.__nonzero__() (which is called if a GitObject is evaluated in\na boolean context) returns False for these non-objects.\n\n>>         return iter(generate_summaries('--no-walk', self.sha1)).next()\n> \n> Not exactly fond of this, but I don't have a concrete replacement at\n> the moment.\n> \n> [...]\n> Okay.\n> \n>>     def __str__(self):\n>>         return self.sha1 or ZEROS\n> \n> I wonder what value this adds when .short is around.\n\nThe full object name is used in the X-Git-{Oldrev,Newrev} email headers\nand probably in some error messages and stuff.\n\n>> class Change(object):\n>>     \"\"\"A Change that has been made to the Git repository.\n>>\n>>     Abstract class from which both Revisions and ReferenceChanges are\n>>     derived.  A Change knows how to generate a notification email\n>>     describing itself.\"\"\"\n>>\n>>     def __init__(self, environment):\n>>         self.environment = environment\n>>         self._values = None\n>>\n>>     def _compute_values(self):\n>>         \"\"\"Return a dictionary {keyword : expansion} for this Change.\n>>\n>>         Derived classes overload this method to add more entries to\n>>         the return value.  This method is used internally by\n>>         get_values().  The return value should always be a new\n>>         dictionary.\"\"\"\n>>\n>>         return self.environment.get_values()\n> \n> Why is this an \"internal function\"?  What is your criterion for\n> internal versus non-internal?\n\nThis method is meant to be overridden by derived classes to affect the\nmap returned by get_values().  But elsewhere get_values() should be\ncalled, not this method (because get_values() memoizes its return value).\n\n>>     def get_values(self, **extra_values):\n>>         \"\"\"Return a dictionary {keyword : expansion} for this Change.\n>>\n>>         Return a dictionary mapping keywords to the values that they\n>>         should be expanded to for this Change (used when interpolating\n>>         template strings).  If any keyword arguments are supplied, add\n>>         those to the return value as well.  The return value is always\n>>         a new dictionary.\"\"\"\n>>\n>>         if self._values is None:\n>>             self._values = self._compute_values()\n>>\n>>         values = self._values.copy()\n>>         if extra_values:\n>>             values.update(extra_values)\n>>         return values\n> \n> Unsure what this is about.\n\nThe dictionary is mainly used to provide values that can be interpolated\ninto the email templates.  It also has the advantage that it is only\ncalled once, and then its value is used multiple times, which limits the\namount of boilerplate needed for derived classes to override the getter\nmethods without forcing those methods to be called many times.\n\n> [...]\n>>     def expand_header_lines(self, template, **extra_values):\n>>         \"\"\"Break template into lines and expand each line as an RFC 2822 header.\n>>\n>>         Encode values and split up lines that are too long.  Silently\n>>         skip lines that contain references to unknown variables.\"\"\"\n>>\n>>         values = self.get_values(**extra_values)\n>>         for line in template.splitlines(True):\n>>             (name, value) = line.split(':', 1)\n>>             value = value.rstrip('\\n\\r')\n> \n> Doesn't splitlines() make the rstrip() redundant?\n\nAs written it doesn't because I pass keepends=True to splitlines().  But\nif I remove the keepends argument then I can indeed drop the rstrip().\nWill change.\n\n>>             try:\n>>                 value = value % values\n>>             except KeyError, e:\n>>                 if DEBUG:\n>>                     sys.stderr.write(\n>>                         'Warning: unknown variable %r in the following line; line skipped:\\n'\n>>                         '    %s'\n>>                         % (e.args[0], line,)\n>>                         )\n> \n> If DEBUG isn't on, you risk leaving the value string interpolated\n> without even telling the user.  What does it mean to the end user?\n\nThere are some \"nice-to-have\" values in the templates that are not\nnecessary and might be missing if the user hasn't gone to the trouble to\nconfigure every last setting.  For example, if no emaildomain is defined\nthen the pusher_email cannot be determined, resulting in the Reply-To\nheader being omitted.\n\nMy assumption is that a sysadmin would turn on DEBUG when testing the\nscript, check that any missing headers are uninteresting, and then turn\noff DEBUG for production use so that users don't have to see the\nwarnings every time they push.\n\nIf you have another suggestion, let me know.\n\n>>             else:\n>>                 try:\n>>                     h = Header(value, header_name=name)\n>>                 except UnicodeDecodeError:\n>>                     h = Header(value, header_name=name, charset=CHARSET, errors='replace')\n>>                 for splitline in ('%s: %s\\n' % (name, h.encode(),)).splitlines(True):\n>>                     yield splitline\n> \n> Not elated by this exception cascading, but I suppose it's cheaper\n> than actually checking everything.\n> \n>>     def generate_email_header(self):\n>>         \"\"\"Generate the RFC 2822 email headers for this Change, a line at a time.\n>>\n>>         The output should not include the trailing blank line.\"\"\"\n>>\n>>         raise NotImplementedError()\n>>\n>>     def generate_email_intro(self):\n>>         \"\"\"Generate the email intro for this Change, a line at a time.\n>>\n>>         The output will be used as the standard boilerplate at the top\n>>         of the email body.\"\"\"\n>>\n>>         raise NotImplementedError()\n>>\n>>     def generate_email_body(self):\n>>         \"\"\"Generate the main part of the email body, a line at a time.\n>>\n>>         The text in the body might be truncated after a specified\n>>         number of lines (see multimailhook.emailmaxlines).\"\"\"\n>>\n>>         raise NotImplementedError()\n>>\n>>     def generate_email_footer(self):\n>>         \"\"\"Generate the footer of the email, a line at a time.\n>>\n>>         The footer is always included, irrespective of\n>>         multimailhook.emailmaxlines.\"\"\"\n>>\n>>         raise NotImplementedError()\n> \n> Unsure what these are about.\n\nThese are basically just to allow code sharing across the various Change\nclasses.\n\n>>     def generate_email(self, push, body_filter=None):\n>>         \"\"\"Generate an email describing this change.\n>>\n>>         Iterate over the lines (including the header lines) of an\n>>         email describing this change.  If body_filter is not None,\n>>         then use it to filter the lines that are intended for the\n>>         email body.\"\"\"\n>>\n>>         for line in self.generate_email_header():\n>>             yield line\n>>         yield '\\n'\n>>         for line in self.generate_email_intro():\n>>             yield line\n>>\n>>         body = self.generate_email_body(push)\n>>         if body_filter is not None:\n> \n> Redundant \"is not None\".\n\nThis way of writing the test is robust against objects for which\nbool(body_filter) might return False.\n\n>>             body = body_filter(body)\n>>         for line in body:\n>>             yield line\n>>\n>>         for line in self.generate_email_footer():\n>>             yield line\n> \n> Nicely done with yield.\n> \n>> class Revision(Change):\n>>     \"\"\"A Change consisting of a single git commit.\"\"\"\n>>\n>>     def __init__(self, reference_change, rev, num, tot):\n>>         Change.__init__(self, reference_change.environment)\n> \n> super?\n\nIMO, in Python 2.x, super() is really only useful in a class hierarchy\nwhere multiple inheritance is going to be supported, like in the\nEnvironment classes.  The problem is that even if you use super(), you\nhave to type the name of the containing class explicitly; e.g.,\n\n    super(Revision, self).__init__(reference_change.environment)\n\nIt is even longer than the explicit reference to the parent class, and\nthough it doesn't break if another class is inserted into the\ninheritance chain, it *does* break if the class itself is renamed.  So I\nusually don't bother with super() unless I'm using multiple inheritance.\n\nIn Python 3, where super() doesn't require redundant arguments, it is\nmuch less cumbersome to use.\n\n> [...]\n>>         # First line of commit message:\n>>         try:\n>>             oneline = read_git_output(\n>>                 ['log', '--format=%s', '--no-walk', self.rev.sha1]\n>>                 )\n>>         except CommandError:\n>>             oneline = self.rev.sha1\n> \n> What does this mean?  When will you get a CommandError?\n\nI can't think of a plausible reason that this command would fail.\n\n>                                                          And how do\n> you respond to it?\n\nBy using the commit's SHA-1 in place of its subject line.\n\n>>         values['rev'] = self.rev.sha1\n>>         values['rev_short'] = self.rev.short\n>>         values['change_type'] = self.change_type\n>>         values['refname'] = self.refname\n>>         values['short_refname'] = self.reference_change.short_refname\n>>         values['refname_type'] = self.reference_change.refname_type\n>>         values['reply_to_msgid'] = self.reference_change.msgid\n>>         values['num'] = self.num\n>>         values['tot'] = self.tot\n>>         values['recipients'] = self.recipients\n>>         values['oneline'] = oneline\n>>         values['author'] = self.author\n> \n> Ugh.  Use\n> \n>   { rev: self.rev.sha1,\n>     rev_short: self.rev.short\n>     ...\n>   }\n> \n> and merge it with the existing dictionary.\n\nYes, I could do that (though it needs quotes around the key strings).\nOr the even more attractive\n\n    values.update(\n        rev=self.rev.sha1,\n        rev_short=self.rev.short,\n        ...\n        )\n\nI had the latter in an earlier version of the script, but I thought it\nmight be too unfamiliar for non-Python-experts.  I guess I'm using\npretty highfalutin Python anyway so this change wouldn't hurt.  What do\nyou think?\n\n>                                             Unsure why you're building\n> a dictionary in the first place.\n\nTo use in template interpolation and also the other reasons mentioned above.\n\n> [...]\n>>     @staticmethod\n> \n> Unsure what such a huge static method is doing here, but we'll find\n> out soon enough.\n> \n>>     def create(environment, oldrev, newrev, refname):\n>>         \"\"\"Return a ReferenceChange object representing the change.\n>>\n>>         Return an object that represents the type of change that is being\n>>         made. oldrev and newrev should be SHA1s or ZEROS.\"\"\"\n> \n> Like I said before, use the typesystem effectively: why is using a\n> string with 40 zeros somehow better than None in your program _logic_?\n> I can understand converting None to 40 zeros for display purposes.\n\nThe ZEROS come straight from the post-receive script input, and as soon\nas they are wrapped in a GitObject they are turned into None.\n\n>>         old = GitObject(oldrev)\n>>         new = GitObject(newrev)\n>>         rev = new or old\n>>\n>>         # The revision type tells us what type the commit is, combined with\n>>         # the location of the ref we can decide between\n>>         #  - working branch\n>>         #  - tracking branch\n>>         #  - unannotated tag\n>>         #  - annotated tag\n> \n> Could be simpler.\n\nIf you mean the distinction between four types of ref is\novercomplicated, this is something taken over from the old\npost-receive-email script.  If you just mean that the code could be\nsimplified, then please make a suggestion.\n\n> [...]\n>>     def __init__(self, environment, refname, short_refname, old, new, rev):\n>>         Change.__init__(self, environment)\n>>         self.change_type = {\n>>             (False, True) : 'create',\n>>             (True, True) : 'update',\n>>             (True, False) : 'delete',\n>>             }[bool(old), bool(new)]\n> \n> As a general principle, avoid casting: if new is a dictionary, what\n> does bool(new) even mean?  You just have to trust types, and let go of\n> that much safety.\n\nold and new are not dictionaries, they are GitObject instances.  And\nthis is not casting, it is calling old.__nonzero__() and\nnew.__nonzero__() to see whether they are real objects vs. ZEROS and to\ncanonicalize their values so that they can be used as indexes for the\nliteral dictionary that decides what type of change is being described.\n\n> [...]\n>> class BranchChange(ReferenceChange):\n>>     refname_type = 'branch'\n> \n> Unsure what new information this conveys over the type.\n\nIt is made available for template interpolation.\n\n> [...]\n>>     def describe_tag(self, push):\n>>         \"\"\"Describe the new value of an annotated tag.\"\"\"\n>>\n>>         # Use git for-each-ref to pull out the individual fields from\n>>         # the tag\n>>         [tagobject, tagtype, tagger, tagged] = read_git_lines(\n>>             ['for-each-ref', '--format=%s' % (self.ANNOTATED_TAG_FORMAT,), self.refname],\n>>             )\n> \n> You could've saved yourself a lot of trouble by running one f-e-r on\n> refs/tags and filtering that.  I don't know what you're gaining from\n> this overzealous object-orientation.\n\nIt's only needed for the tags that have changed (which is probably zero\nin most cases).\n\n> [...]\n>>         # Show the content of the tag message; this might contain a\n>>         # change log or release notes so is worth displaying.\n>>         yield LOGBEGIN\n>>         contents = list(read_git_lines(['cat-file', 'tag', self.new.sha1], keepends=True))\n> \n> You could've easily batched this.\n\nI don't understand what you mean.\n\n>>         contents = contents[contents.index('\\n') + 1:]\n>>         if contents and contents[-1][-1:] != '\\n':\n>>             contents.append('\\n')\n>>         for line in contents:\n>>             yield line\n>>\n>>         if self.show_shortlog and tagtype == 'commit':\n>>             # Only commit tags make sense to have rev-list operations\n>>             # performed on them\n>>             yield '\\n'\n>>             if prevtag:\n>>                 # Show changes since the previous release\n>>                 revlist = read_git_output(\n>>                     ['rev-list', '--pretty=short', '%s..%s' % (prevtag, self.new,)],\n>>                     keepends=True,\n>>                     )\n>>             else:\n>>                 # No previous tag, show all the changes since time\n>>                 # began\n>>                 revlist = read_git_output(\n>>                     ['rev-list', '--pretty=short', '%s' % (self.new,)],\n>>                     keepends=True,\n>>                     )\n>>             for line in read_git_lines(['shortlog'], input=revlist, keepends=True):\n>>                 yield line\n>>\n>>         yield LOGEND\n>>         yield '\\n'\n> \n> Way too many git invocations, I think.\n\nLuckily git is very fast :-)\n\nI'm not to worried about performance here.  The script will typically\nonly be run on pushes, and most pushes affect only one or a few\nreferences.  I don't think these few git invocations will be prohibitive.\n\n>> class OtherReferenceChange(ReferenceChange):\n>>     refname_type = 'reference'\n>>\n>>     def __init__(self, environment, refname, short_refname, old, new, rev):\n>>         # We use the full refname as short_refname, because otherwise\n>>         # the full name of the reference would not be obvious from the\n>>         # text of the email.\n>>         ReferenceChange.__init__(\n>>             self, environment,\n>>             refname=refname, short_refname=refname,\n>>             old=old, new=new, rev=rev,\n>>             )\n>>         self.recipients = environment.get_refchange_recipients(self)\n> \n> What is the point of this?  Why not just use ReferenceChange directly?\n\nMaybe you missed \"short_refname=refname\" (one of the arguments is not\nbeing passed through 1:1).  The reason is explained in the comment.\n\n>> class Mailer(object):\n>>     \"\"\"An object that can send emails.\"\"\"\n>>\n>>     def send(self, lines, to_addrs):\n>>         \"\"\"Send an email consisting of lines.\n>>\n>>         lines must be an iterable over the lines constituting the\n>>         header and body of the email.  to_addrs is a list of recipient\n>>         addresses (can be needed even if lines already contains a\n>>         \"To:\" field).  It can be either a string (comma-separated list\n>>         of email addresses) or a Python list of individual email\n>>         addresses.\n>>\n>>         \"\"\"\n>>\n>>         raise NotImplementedError()\n> \n> Abstract base class (abc)?  Or do you want to support Python <2.6?\n\nYes, AFAIK the script works with any Python >= 2.4.\n\n>> class SendMailer(Mailer):\n>>     \"\"\"Send emails using '/usr/sbin/sendmail -t'.\"\"\"\n>>\n>>     def __init__(self, command=None, envelopesender=None):\n>>         \"\"\"Construct a SendMailer instance.\n>>\n>>         command should be the command and arguments used to invoke\n>>         sendmail, as a list of strings.  If an envelopesender is\n>>         provided, it will also be passed to the command, via '-f\n>>         envelopesender'.\"\"\"\n>>\n>>         if command:\n>>             self.command = command[:]\n>>         else:\n>>             self.command = ['/usr/sbin/sendmail', '-t']\n> \n> If you want to DWIM when the configuration variable is missing, do it\n> fully using a list of good candidates like /usr/lib/sendmail,\n> /usr/sbin/sendmail, /usr/ucblib/sendmail, /usr/bin/msmtp.\n\nOK, I just added /usr/sbin/sendmail and /usr/lib/sendmail, which are the\npaths checked by \"git send-mail\".\n\n>                                                            Also, what\n> happened to our faithful 'git send-email' Perl script?  Isn't that\n> most likely to be installed?\n\nWe could use \"git send-email\" to generate and send the revision emails,\nbut then we would lose most control over the contents of the emails.\n\n>>         if envelopesender:\n>>             self.command.extend(['-f', envelopesender])\n>>\n>>     def send(self, lines, to_addrs):\n>>         try:\n>>             p = subprocess.Popen(self.command, stdin=subprocess.PIPE)\n>>         except OSError, e:\n>>             sys.stderr.write(\n>>                 '*** Cannot execute command: %s\\n' % ' '.join(self.command)\n>>                 + '*** %s\\n' % str(e)\n>>                 + '*** Try setting multimailhook.mailer to \"smtp\"\\n'\n>>                 '*** to send emails without using the sendmail command.\\n'\n>>                 )\n>>             sys.exit(1)\n> \n> Why do you need to concatenate strings using +?  This can take a list of strings, no?\n\nsys.stderr.write() can only take a single string argument.  You might\nhave seen it called like this:\n\n    sys.stderr.write(\n        'foo\\n'\n        'bar\\n'\n        )\n\nThis is using the Python compiler's feature that literal strings can be\nappended to each other by juxtaposition (notice there are no commas).\nBut this only works for literal strings, not for string expressions.\n\n> [...]\n>>     def __del__(self):\n>>         self.smtp.quit()\n> \n> So you close the connection when the object is destroyed by the GC.\n\nYes, where here (since we are talking about CPython) reference counting\nis used and objects are deleted as soon as the reference count goes to\nzero.  The point is to send all of the emails through one connection to\nthe SMTP server, which I think saves a lot of time.\n\n> [...]\n>> class OutputMailer(Mailer):\n>>     \"\"\"Write emails to an output stream, bracketed by lines of '=' characters.\n>>\n>>     This is intended for debugging purposes.\"\"\"\n>>\n>>     SEPARATOR = '=' * 75 + '\\n'\n>>\n>>     def __init__(self, f):\n>>         self.f = f\n>>\n>>     def send(self, lines, to_addrs):\n>>         self.f.write(self.SEPARATOR)\n>>         self.f.writelines(lines)\n>>         self.f.write(self.SEPARATOR)\n> \n> Unsure what this is.\n\nFor testing and debugging (e.g., via the --stdout command-line option).\n\n>> def get_git_dir():\n>>     \"\"\"Determine GIT_DIR.\n>>\n>>     Determine GIT_DIR either from the GIT_DIR environment variable or\n>>     from the working directory, using Git's usual rules.\"\"\"\n>>\n>>     try:\n>>         return read_git_output(['rev-parse', '--git-dir'])\n>>     except CommandError:\n>>         sys.stderr.write('fatal: git_multimail: not in a git working copy\\n')\n>>         sys.exit(1)\n> \n> Why do you need a working copy?  Will a bare repository not suffice?\n\nYes, a bare repo definitely suffices.  I think the error message is just\nmisleading, correct?  Will fix.\n\n>> class Environment(object):\n> \n> New-style class.  I wonder why you suddenly switched.\n\n?  All of the classes are new-style classes.\n\n> [...]\n>> class ConfigEnvironmentMixin(Environment):\n>>     \"\"\"A mixin that sets self.config to its constructor's config argument.\n>>\n>>     This class's constructor consumes the \"config\" argument.\n>>\n>>     Mixins that need to inspect the config should inherit from this\n>>     class (1) to make sure that \"config\" is still in the constructor\n>>     arguments with its own constructor runs and/or (2) to be sure that\n>>     self.config is set after construction.\"\"\"\n>>\n>>     def __init__(self, config, **kw):\n>>         super(ConfigEnvironmentMixin, self).__init__(**kw)\n>>         self.config = config\n> \n> Overdoing the OO factories, much?\n\nI went to a lot of trouble to make the Environment mixin classes\ncomposable, because what I've learned from the feedback in the last\nmonths is that everybody wants to do something different with this\nscript.  I tried out a few designs before I settled on this one.\n\n> I'll skip a few boring factory classes.\n> \n>> class GenericEnvironment(\n>>     ProjectdescEnvironmentMixin,\n>>     ConfigMaxlinesEnvironmentMixin,\n>>     ConfigFilterLinesEnvironmentMixin,\n>>     ConfigRecipientsEnvironmentMixin,\n>>     PusherDomainEnvironmentMixin,\n>>     ConfigOptionsEnvironmentMixin,\n>>     GenericEnvironmentMixin,\n>>     Environment,\n>>     ):\n>>     pass\n> \n> Sigh.  I might as well be reading some Java now :/\n\nNo, Java doesn't allow multiple inheritance :-)\n\n> Sorry, I'm exhausted.\n> \n> Let's take a step back and look at what this gigantic script is doing.\n> It uses the information from a push to string-interpolate a template\n> and generate emails, right?  The rest of the script is about churning\n> on the updated refs to prettify the emails.\n> \n> From my quick reading, it seems to be unnecessarily complicated and\n> inefficient.  Why are there so many factories, and why do you call out\n> to git at every opportunity, instead of cleanly separating computation\n> from rendering?\n\nRegarding size: post-receive-email is 748 lines of shell script,\nincluding comments and string literals.  The extent of its\nconfigurability is approximately this block of code:\n\n> projectdesc=$(sed -ne '1p' \"$GIT_DIR/description\" 2>/dev/null)\n> # Check if the description is unchanged from it's default, and shorten it to\n> # a more manageable length if it is\n> if expr \"$projectdesc\" : \"Unnamed repository.*$\" >/dev/null\n> then\n> \tprojectdesc=\"UNNAMED PROJECT\"\n> fi\n> \n> recipients=$(git config hooks.mailinglist)\n> announcerecipients=$(git config hooks.announcelist)\n> envelopesender=$(git config hooks.envelopesender)\n> emailprefix=$(git config hooks.emailprefix || echo '[SCM] ')\n> custom_showrev=$(git config hooks.showrev)\n> maxlines=$(git config hooks.emailmaxlines)\n> diffopts=$(git config hooks.diffopts)\n> : ${diffopts:=\"--stat --summary --find-copies-harder\"}\n\nThe script has to be edited to make any non-trivial configuration change.\n\ngit_multimail.py is 2398 lines of Python script, including comments and\nstring literals.  The fraction of that code that is dedicated to\nconfigurability is approximately 1000 lines.  Relative to\npost-receive-email, it adds\n\n* much more configurability, without the need to edit the script.\n\n* optional separate emails for each commit\n\n* non-buggy determination of which commits have been added by a\nreference change, and distinction between commits that have been added\nto a branch vs. commits that have been added altogether and between\ncommits that have been deleted from a branch vs. commits that have been\ndeleted altogether.\n\n* migration code to migrate a post-receive-email configuration into a\ngit-multimail configuration (mostly via a supplemental script)\n\n* support for gitolite environments\n\n* support for sendmail vs. smtplib\n\n* improved utf-8 correctness.\n\nRegarding efficiency, I don't think it is a problem.  But patches or\nconcrete suggestions are certainly welcome.\n\nRegarding separation of computation and rendering, yes, they could be\nseparated better.  (BTW, it would make the script even longer.)  The\nrendering is already largely done via templates that can be changed from\noutside of the script.  But I might work on separating them more\nstrictly so that some of the code could be reused, for example, to send\nnotifications via IRC or XMPP.\n\nThanks for all of your comments!  I hope I have addressed most of them\nin this email and in the commits that I just pushed to GitHub.\n\nMichael\n\n[1] https://github.com/mhagger/git-multimail/pull/2\n[2] http://www.python.org/dev/peps/pep-0394/\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"222435","messageId":"51D3DA9A.9090604@alum.mit.edu","threadId":"34326","inReplyTo":"7vsizwiowt.fsf@alter.siamese.dyndns.org","subject":"Re: Review of git multimail","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-07-03T08:02:34Z","receivedAt":"2013-07-03T08:02:34Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 07/03/2013 12:21 AM, Junio C Hamano wrote:\n> Ramkumar Ramachandra <artagnon@gmail.com> writes:\n> \n>>>     def get(self, name, default=''):\n>>>         try:\n>>>             values = self._split(read_git_output(\n>>>                     ['config', '--get', '--null', '%s.%s' % (self.section, name)],\n>>>                     env=self.env, keepends=True,\n>>>                     ))\n>>\n>> Wait, what is the point of using --null and then splitting by hand\n>> using a poorly-defined static method?  Why not drop the --null and\n>> splitlines() as usual?\n> \n> You may actually have spotted a bug or misuse of \"--get\" here.\n> \n> With this sample configuration:\n> \n>         $ cat >sample <<\\EOF\n>         [a]\n>                 one = value\n>                 one = another\n> \n>         [b]\n>                 one = \"value\\nanother\"\n>         EOF\n> \n> A script cannot differentiate between them without using '--null'.\n> \n> \t$ git config -f sample --get-all a.one\n>         $ git config -f sample --get-all b.one\n> \n> But that matters only when you use \"--get-all\", not \"--get\".  If\n> this method wants to make sure that the user did not misuse a.one\n> as a multi-valued configuration variable, use of \"--null --get-all\"\n> followed by checking how many items the command gives you back would\n> be a way to do so.\n\nNo, the code in question was a simple sanity check (i.e., mostly a check\nof my own sanity and understanding of \"git config\" behavior) preceding\nthe information-losing next line \"return values[0]\".  If it had been\nmeant as a check that the user hadn't misconfigured the system, then I\nwouldn't have used assert but rather raised a ConfigurationException\nwith an explanatory message.\n\nI would be happy to add the checking that you described, but I didn't\nhave the impression that it is the usual convention.  Does code that\nwants a single value from the config usually verify that there is\none-and-only-one value, or does it typically just do the equivalent of\n\"git config --get\" and use the returned (effectively the last) value?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"222437","messageId":"7vzju4giss.fsf@alter.siamese.dyndns.org","threadId":"34326","inReplyTo":"51D3DA9A.9090604@alum.mit.edu","subject":"Re: Review of git multimail","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-03T08:16:19Z","receivedAt":"2013-07-03T08:16:19Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> I would be happy to add the checking that you described, but I didn't\n> have the impression that it is the usual convention.  Does code that\n> wants a single value from the config usually verify that there is\n> one-and-only-one value, or does it typically just do the equivalent of\n> \"git config --get\" and use the returned (effectively the last) value?\n\nIn most cases, variables are \"one value per key\" and follow \"the\nlast one wins\" rule, which is the reason why we read from the most\ngeneric to the most specific (i.e. $GIT_DIR/config is read last).\nFor such uses, reading from \"--get\", and not from \"--get-all\", is\nabsolutely the right thing to do.\n\nBut then as Ram said, there probably is not a need for --null; you\ncan just read from textual \"--get\" to the end without any splitting\n(using splitlines is of course wrong if you do so).\n\nThanks.\n"},{"id":"222438","messageId":"20130703082902.GE9161@serenity.lan","threadId":"34326","inReplyTo":"51D3DA9A.9090604@alum.mit.edu","subject":"Re: Review of git multimail","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-07-03T08:29:02Z","receivedAt":"2013-07-03T08:29:02Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Jul 03, 2013 at 10:02:34AM +0200, Michael Haggerty wrote:\n> On 07/03/2013 12:21 AM, Junio C Hamano wrote:\n> > Ramkumar Ramachandra <artagnon@gmail.com> writes:\n> > \n> >>>     def get(self, name, default=''):\n> >>>         try:\n> >>>             values = self._split(read_git_output(\n> >>>                     ['config', '--get', '--null', '%s.%s' % (self.section, name)],\n> >>>                     env=self.env, keepends=True,\n> >>>                     ))\n> >>\n> >> Wait, what is the point of using --null and then splitting by hand\n> >> using a poorly-defined static method?  Why not drop the --null and\n> >> splitlines() as usual?\n> > \n> > You may actually have spotted a bug or misuse of \"--get\" here.\n> > \n> > With this sample configuration:\n> > \n> >         $ cat >sample <<\\EOF\n> >         [a]\n> >                 one = value\n> >                 one = another\n> > \n> >         [b]\n> >                 one = \"value\\nanother\"\n> >         EOF\n> > \n> > A script cannot differentiate between them without using '--null'.\n> > \n> > \t$ git config -f sample --get-all a.one\n> >         $ git config -f sample --get-all b.one\n> > \n> > But that matters only when you use \"--get-all\", not \"--get\".  If\n> > this method wants to make sure that the user did not misuse a.one\n> > as a multi-valued configuration variable, use of \"--null --get-all\"\n> > followed by checking how many items the command gives you back would\n> > be a way to do so.\n> \n> No, the code in question was a simple sanity check (i.e., mostly a check\n> of my own sanity and understanding of \"git config\" behavior) preceding\n> the information-losing next line \"return values[0]\".  If it had been\n> meant as a check that the user hadn't misconfigured the system, then I\n> wouldn't have used assert but rather raised a ConfigurationException\n> with an explanatory message.\n> \n> I would be happy to add the checking that you described, but I didn't\n> have the impression that it is the usual convention.  Does code that\n> wants a single value from the config usually verify that there is\n> one-and-only-one value, or does it typically just do the equivalent of\n> \"git config --get\" and use the returned (effectively the last) value?\n\nDoesn't \"git config --get\" return an error if there are multiple values?\nThe answer is apparently \"no\" - I wrote the text below from\ngit-config(1) and then checked the behaviour.  This seems to be a\nregression in git-config (bisect running now).\n\nI think the \"correct\" answer is what's below, but it doesn't work like\nthis in current Git:\n\n    If you want a single value then I think it's normal to just read the\n    output of \"git config\" and let it handle the error cases, without\n    needing to split the result at all.\n\n    I think there is a different issue in the \"except\" block following\n    the code quoted at the top though - you will return \"default\" if a\n    key happens to be multi-valued.  The script should check the return\n    code and raise a ConfigurationException if it is 2.\n"},{"id":"222439","messageId":"20130703083356.GF9161@serenity.lan","threadId":"34326","inReplyTo":"20130703082902.GE9161@serenity.lan","subject":"Re: Review of git multimail","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-07-03T08:33:56Z","receivedAt":"2013-07-03T08:33:56Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Jul 03, 2013 at 09:29:02AM +0100, John Keeping wrote:\n> Doesn't \"git config --get\" return an error if there are multiple values?\n> The answer is apparently \"no\" - I wrote the text below from\n> git-config(1) and then checked the behaviour.  This seems to be a\n> regression in git-config (bisect running now).\n\nAh, that was an intentional change in commit 00b347d (git-config: do not\ncomplain about duplicate entries, 2012-10-23).  So the issue is that the\ndocumentation was not updated when the behaviour was changed.\n"},{"id":"222457","messageId":"CALkWK0=taYiV3UTaj9r-FLdaCeZRzVBTp_MH4sQt8-v+YYqbaA@mail.gmail.com","threadId":"34326","inReplyTo":"51D36BD8.1060909@alum.mit.edu","subject":"Re: Review of git multimail","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-07-03T10:23:03Z","receivedAt":"2013-07-03T10:23:03Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Michael Haggerty wrote:\n> On 07/02/2013 09:23 PM, Ramkumar Ramachandra wrote:\n>> git_multimail.py wrote:\n>>> #! /usr/bin/env python2\n>>\n>> Do all distributions ship it as python2 now?\n>\n> No, but nor is \"python\" always Python version 2.x (I believe that Arch\n> Linux now installs Python 3 as \"python\").  This topic was discussed here\n> [1].  Basically, my reasoning is that (a) PEP 394 [2] says that\n> \"python2\" is the correct name for a Python 2.x interpreter, (b) I\n> believe that other distros are moving in that direction, and (c) if the\n> script says \"python2\" but no python2 is installed, the error is pretty\n> obvious, whereas if the script says \"python\" and that is actually Python\n> 3.x, the error would be more cryptic.\n\nYeah, this is good reasoning.  And yes, I'm on Arch: python points to\npython3, and python2 points to python2.  A couple of thoughts while\nwe're on the subject:\n\n1. We should probably convert git-remote-{hg,bzr} to use this style\ntoo: they give cryptic errors now, and I have a ~/bin/python pointing\nto python2 which is higher up in $PATH to work around it.  Debian uses\nan alternatives mechanism to have multiple versions of the same\npackage, but I personally think the system is ugly.\n\n2. Is there a way to determine the python version in-script and\nerror-out quickly?  Is it worth the ugliness?\n\n>>>             'Command \"%s\" failed with retcode %s' % (' '.join(cmd), retcode,)\n>>\n>> So cmd is a list.\n>\n> Yes, commands are always lists in my code because it is less error-prone\n> than trying to quote arguments correctly for the shell.  Do you think I\n> should document that here, or elsewhere, or everywhere, or ...?\n\nIf you look at the prototype of execvpe(), the calling semantics are\nimmediately clear, but we don't have that luxury in Python: probably\nrename the variable cmd_argv?\n\n>>> class ConfigurationException(Exception):\n>>>     pass\n>>\n>> Dead code?\n>\n> No, this defines an exception class that inherits all of its methods\n> (including its constructors) from Exception.  This is useful because an\n> exception of type ConfigurationException is distinguishable from other\n> types of Exceptions, and can be caught using \"except\n> ConfigurationException, e\".\n\nOkay.  I was under the impression that you had some future extension plans.\n\n>>>     def get(self, name, default=''):\n>>>         try:\n>>>             values = self._split(read_git_output(\n>>>                     ['config', '--get', '--null', '%s.%s' % (self.section, name)],\n>>>                     env=self.env, keepends=True,\n>>>                     ))\n>>\n>> Wait, what is the point of using --null and then splitting by hand\n>> using a poorly-defined static method?  Why not drop the --null and\n>> splitlines() as usual?\n>\n> To avoid confusion if a single config value contains end-of-line\n> characters.  In this case we are using --get, so only a single value is\n> allowed anyway, and presumably we could take the output and strip a\n> single last '\\n' from it.  But null-terminated output is simply easier\n> to handle in general and I don't see an advantage to avoiding its usage.\n\nMy rationale for avoiding it comes from Python's lack of inbuilt\nfunctions to handle it; but yeah, Junio pointed out that quirk in the\nprevious email.\n\n>>>                 return default\n>>>             else:\n>>>                 raise\n>>\n>> raise what?\n>\n> This is the Python construct to re-throw the exception that was caught\n> in the catch block containing it; i.e., the CommandError from a few\n> lines earlier.\n\nAh, thanks.\n\n>>>     def get_recipients(self, name, default=None):\n>>>         lines = self.get_all(name, default=None)\n>>>         if lines is None:\n>>>             return default\n>>>         return ', '.join(line.strip() for line in lines)\n>>\n>> Ugh.\n>\n> ?\n\nI would expect it to return a list that can be churned on further, not\na string that's ready for rendering.  Doesn't it resemble the\ndictionary get(), and even your own get_all() in name and signature?\n\n>> Dead code?\n>\n> git_multimail is used as a library by migrate-mailhook-config, and that\n> script uses these methods.\n\nI see.  Perhaps clean separation to avoid confusing readers?\n\n>>> def generate_summaries(*log_args):\n>>>     cmd = [\n>>>         'log', '--abbrev', '--format=%h %s',\n>>>         ] + list(log_args) + ['--']\n>>\n>> What is log_args if not a list?\n>\n> It is a tuple and therefore needs to be converted to a list here.\n\nOh, I always thought *log_args meant list; to my surprise, it's a tuple.\n\n>> Just rev-parse --verify --short $SHA1^0: if it resolves, set\n>> self.short; one liner?\n>\n> I don't follow.  We need both the long and the short SHA-1s to fill in\n> the templates.  What code exactly do you propose to replace with your\n> one-liner?\n\nOh, you need both.  I was hoping to cut the cat-file -t too, but you\nneed another call resolving $SHA1^{object} to distinguish between\ncommits and tags; so never mind.\n\n>> What is the point of letting the user instantiate a GitObject without\n>> a valid .sha1 in the first place?\n>\n> '0'*40 is passed to the post-receive script to indicate \"no object\"; for\n> example, a branch deletion is represented as\n>\n> 0123456789abcdef0123456789abcdef01234567\n> 0000000000000000000000000000000000000000 refs/heads/branch\n>\n> It is convenient to treat this as if it were a GitObject.\n> GitObject.__nonzero__() (which is called if a GitObject is evaluated in\n> a boolean context) returns False for these non-objects.\n\nFair enough, although my objection mainly has to do with not\nseparating logic and rendering cleanly.\n\n>>>     def __str__(self):\n>>>         return self.sha1 or ZEROS\n>>\n>> I wonder what value this adds when .short is around.\n>\n> The full object name is used in the X-Git-{Oldrev,Newrev} email headers\n> and probably in some error messages and stuff.\n\nIf I manage to separate out the logic from the rendering, I'll send\nPRs directly.\n\n>>>     def _compute_values(self):\n>>>         return self.environment.get_values()\n>>\n>> Why is this an \"internal function\"?  What is your criterion for\n>> internal versus non-internal?\n>\n> This method is meant to be overridden by derived classes to affect the\n> map returned by get_values().  But elsewhere get_values() should be\n> called, not this method (because get_values() memoizes its return value).\n\nFair rationale.\n\n>>>     def get_values(self, **extra_values):\n>>>         if self._values is None:\n>>>             self._values = self._compute_values()\n>>>\n>>>         values = self._values.copy()\n>>>         if extra_values:\n>>>             values.update(extra_values)\n>>>         return values\n>>\n>> Unsure what this is about.\n>\n> The dictionary is mainly used to provide values that can be interpolated\n> into the email templates.  It also has the advantage that it is only\n> called once, and then its value is used multiple times, which limits the\n> amount of boilerplate needed for derived classes to override the getter\n> methods without forcing those methods to be called many times.\n\nMakes sense.\n\n>>>             try:\n>>>                 value = value % values\n>>>             except KeyError, e:\n>>>                 if DEBUG:\n>>>                     sys.stderr.write(\n>>>                         'Warning: unknown variable %r in the following line; line skipped:\\n'\n>>>                         '    %s'\n>>>                         % (e.args[0], line,)\n>>>                         )\n>>\n>> If DEBUG isn't on, you risk leaving the value string interpolated\n>> without even telling the user.  What does it mean to the end user?\n>\n> There are some \"nice-to-have\" values in the templates that are not\n> necessary and might be missing if the user hasn't gone to the trouble to\n> configure every last setting.  For example, if no emaildomain is defined\n> then the pusher_email cannot be determined, resulting in the Reply-To\n> header being omitted.\n>\n> My assumption is that a sysadmin would turn on DEBUG when testing the\n> script, check that any missing headers are uninteresting, and then turn\n> off DEBUG for production use so that users don't have to see the\n> warnings every time they push.\n\nAh, so that is the intended usage.  If the impact of omitting certain\nheaders (due to lack of information) doesn't result in unusable emails\nbeing generated, I think we're good.  Are you sure that doesn't\nhappen?\n\n>>>         raise NotImplementedError()\n>>\n>> Unsure what these are about.\n>\n> These are basically just to allow code sharing across the various Change\n> classes.\n\nI'm not sure it's worth supporting Python < 2.6, especially if it\ncosts more to port it to Python 3+ (no abstract base classes; ugliness\nlike this).  Which brings me to: considering that the first commit is\nin late 2012, why didn't you choose to code it in python3 directly?\nUnless I'm mistaken, the only reason git-remote-{bzr,hg} aren't in\npython3 is because the dependencies bzrlib and hglib are in python2.\n\n>>>         body = self.generate_email_body(push)\n>>>         if body_filter is not None:\n>>\n>> Redundant \"is not None\".\n>\n> This way of writing the test is robust against objects for which\n> bool(body_filter) might return False.\n\nIf body_filter isn't a function, we have much larger problems, don't we? ;)\n\nNevertheless, I'm not going to argue with idioms (I've not written any\nPython in years now, so I don't know them).\n\n>>> class Revision(Change):\n>>>     \"\"\"A Change consisting of a single git commit.\"\"\"\n>>>\n>>>     def __init__(self, reference_change, rev, num, tot):\n>>>         Change.__init__(self, reference_change.environment)\n>>\n>> super?\n>\n> IMO, in Python 2.x, super() is really only useful in a class hierarchy\n> where multiple inheritance is going to be supported, like in the\n> Environment classes.  The problem is that even if you use super(), you\n> have to type the name of the containing class explicitly; e.g.,\n>\n>     super(Revision, self).__init__(reference_change.environment)\n>\n> It is even longer than the explicit reference to the parent class, and\n> though it doesn't break if another class is inserted into the\n> inheritance chain, it *does* break if the class itself is renamed.  So I\n> usually don't bother with super() unless I'm using multiple inheritance.\n>\n> In Python 3, where super() doesn't require redundant arguments, it is\n> much less cumbersome to use.\n\nI see.  Thanks for the explanation.\n\n>> [...]\n>>>         # First line of commit message:\n>>>         try:\n>>>             oneline = read_git_output(\n>>>                 ['log', '--format=%s', '--no-walk', self.rev.sha1]\n>>>                 )\n>>>         except CommandError:\n>>>             oneline = self.rev.sha1\n>>\n>> What does this mean?  When will you get a CommandError?\n>\n> I can't think of a plausible reason that this command would fail.\n>\n>>                                                          And how do\n>> you respond to it?\n>\n> By using the commit's SHA-1 in place of its subject line.\n\nWhat you have written translates to: \"If there is a valid commit whose\nsubject cannot be determined (empty subject is still determinate), I\nwill use the commit's SHA-1 hex in its place\", which implies that you\ndo not trust git to be sane ;)\n\nIsn't the entire premise of your script that you have a sane git?\n\n> Yes, I could do that (though it needs quotes around the key strings).\n> Or the even more attractive\n>\n>     values.update(\n>         rev=self.rev.sha1,\n>         rev_short=self.rev.short,\n>         ...\n>         )\n>\n> I had the latter in an earlier version of the script, but I thought it\n> might be too unfamiliar for non-Python-experts.  I guess I'm using\n> pretty highfalutin Python anyway so this change wouldn't hurt.  What do\n> you think?\n\nI like pretty, maintainable, modern code with low redundancy.  As a\ngeneral principle, I always tilt towards educating users/ developers\nabout new (or \"advanced\") features, not abstaining from them because\nthey are too unfamiliar (or \"complicated\").\n\nOn this specifically, a beginner can look up help(values.update) to\nunderstand what the code is doing.  So, yes: values.update() is\ndefinitely better.\n\n>>>         # The revision type tells us what type the commit is, combined with\n>>>         # the location of the ref we can decide between\n>>>         #  - working branch\n>>>         #  - tracking branch\n>>>         #  - unannotated tag\n>>>         #  - annotated tag\n>>\n>> Could be simpler.\n>\n> If you mean the distinction between four types of ref is\n> overcomplicated, this is something taken over from the old\n> post-receive-email script.  If you just mean that the code could be\n> simplified, then please make a suggestion.\n\nI'll send a PR directly if I manage to simplify it.\n\n>>>     def __init__(self, environment, refname, short_refname, old, new, rev):\n>>>         Change.__init__(self, environment)\n>>>         self.change_type = {\n>>>             (False, True) : 'create',\n>>>             (True, True) : 'update',\n>>>             (True, False) : 'delete',\n>>>             }[bool(old), bool(new)]\n>>\n>> As a general principle, avoid casting: if new is a dictionary, what\n>> does bool(new) even mean?  You just have to trust types, and let go of\n>> that much safety.\n>\n> old and new are not dictionaries, they are GitObject instances.  And\n> this is not casting, it is calling old.__nonzero__() and\n> new.__nonzero__() to see whether they are real objects vs. ZEROS and to\n> canonicalize their values so that they can be used as indexes for the\n> literal dictionary that decides what type of change is being described.\n\nAh, I missed that.\n\n>>>     def describe_tag(self, push):\n>>>         \"\"\"Describe the new value of an annotated tag.\"\"\"\n>>>\n>>>         # Use git for-each-ref to pull out the individual fields from\n>>>         # the tag\n>>>         [tagobject, tagtype, tagger, tagged] = read_git_lines(\n>>>             ['for-each-ref', '--format=%s' % (self.ANNOTATED_TAG_FORMAT,), self.refname],\n>>>             )\n>>\n>> You could've saved yourself a lot of trouble by running one f-e-r on\n>> refs/tags and filtering that.  I don't know what you're gaining from\n>> this overzealous object-orientation.\n>\n> It's only needed for the tags that have changed (which is probably zero\n> in most cases).\n\nHm, we'll have to discuss performance in the \"typical case\" soon.\n\n>>>         contents = list(read_git_lines(['cat-file', 'tag', self.new.sha1], keepends=True))\n>>\n>> You could've easily batched this.\n>\n> I don't understand what you mean.\n\nWhenever I call cat-file from a script, I always find myself using the\n--batch variant.\n\n>> Way too many git invocations, I think.\n>\n> Luckily git is very fast :-)\n>\n> I'm not to worried about performance here.  The script will typically\n> only be run on pushes, and most pushes affect only one or a few\n> references.  I don't think these few git invocations will be prohibitive.\n\nI personally push very often, so it's not a problem.  I'm thinking of\na mirroring batched push, where the maintainer pushes out history to a\n\"release\" server every major release (every few months): is the script\nintended to be used in such a scenario, when multiple refs and tags\nare updated non-trivially?\n\n>>>         ReferenceChange.__init__(\n>>>             self, environment,\n>>>             refname=refname, short_refname=refname,\n>>>             old=old, new=new, rev=rev,\n>>>             )\n>>>         self.recipients = environment.get_refchange_recipients(self)\n>>\n>> What is the point of this?  Why not just use ReferenceChange directly?\n>\n> Maybe you missed \"short_refname=refname\" (one of the arguments is not\n> being passed through 1:1).  The reason is explained in the comment.\n\nGah, my bad habit of not reading comments.\n\n>> If you want to DWIM when the configuration variable is missing, do it\n>> fully using a list of good candidates like /usr/lib/sendmail,\n>> /usr/sbin/sendmail, /usr/ucblib/sendmail, /usr/bin/msmtp.\n>\n> OK, I just added /usr/sbin/sendmail and /usr/lib/sendmail, which are the\n> paths checked by \"git send-mail\".\n\nI'm on Arch, and sendmail is dead (only available in AUR now).  I\nthink we should support modern sendmail-compatible alternatives like\nmsmtp (which I have and use).\n\n>>                                                            Also, what\n>> happened to our faithful 'git send-email' Perl script?  Isn't that\n>> most likely to be installed?\n>\n> We could use \"git send-email\" to generate and send the revision emails,\n> but then we would lose most control over the contents of the emails.\n\nI'm talking about the <file>... form.  Does it necessarily mangle the\nheaders of an mbox that is fed to it, or am I missing something?\n\n> sys.stderr.write() can only take a single string argument.  You might\n> have seen it called like this:\n>\n>     sys.stderr.write(\n>         'foo\\n'\n>         'bar\\n'\n>         )\n>\n> This is using the Python compiler's feature that literal strings can be\n> appended to each other by juxtaposition (notice there are no commas).\n> But this only works for literal strings, not for string expressions.\n\nOuch, that's ugly.\n\n>>> class Environment(object):\n>>\n>> New-style class.  I wonder why you suddenly switched.\n>\n> ?  All of the classes are new-style classes.\n\nWhen you say class Foo:, aren't you declaring an old-style class by\ndefault in python2?  New-style classes are those that explicitly\ninherit from object (implicit in python3).\n\n>> Overdoing the OO factories, much?\n>\n> I went to a lot of trouble to make the Environment mixin classes\n> composable, because what I've learned from the feedback in the last\n> months is that everybody wants to do something different with this\n> script.  I tried out a few designs before I settled on this one.\n\nI see.  I'm not a user, so I can't comment.\n\n>>> class GenericEnvironment(\n>>>     ProjectdescEnvironmentMixin,\n>>>     ConfigMaxlinesEnvironmentMixin,\n>>>     ConfigFilterLinesEnvironmentMixin,\n>>>     ConfigRecipientsEnvironmentMixin,\n>>>     PusherDomainEnvironmentMixin,\n>>>     ConfigOptionsEnvironmentMixin,\n>>>     GenericEnvironmentMixin,\n>>>     Environment,\n>>>     ):\n>>>     pass\n>>\n>> Sigh.  I might as well be reading some Java now :/\n>\n> No, Java doesn't allow multiple inheritance :-)\n\nHa, yes: I meant in the way you factory'ified everything.  I don't\nrecall seeing this kind of code in a real-world application though: I\nskimmed through Django's code many years ago, and haven't found such a\npattern.  I have seen instances where multiple inheritance may be\nclassified as borderline useful, but nothing as extensive as this.\nAdditionally, Ruby does not have multiple inheritance.  So, I tilt\ntowards the conclusion that multiple inheritance is Bad and Wrong.\nThen again, I'm not an OO person in general, so I can't have a mature\nopinion on the subject.  Can you present a case for this kind of\nextensive usage quoting real-world examples?\n\n> git_multimail.py is 2398 lines of Python script, including comments and\n> string literals.  The fraction of that code that is dedicated to\n> configurability is approximately 1000 lines.  Relative to\n> post-receive-email, it adds\n>\n> * much more configurability, without the need to edit the script.\n\nSo this is the main selling point.\n\n> * non-buggy determination of which commits have been added by a\n> reference change, and distinction between commits that have been added\n> to a branch vs. commits that have been added altogether and between\n> commits that have been deleted from a branch vs. commits that have been\n> deleted altogether.\n\nOkay, so the post-receive-email script has corner-case bugs.\n\n> [...]\n\nThe others seem to be small'ish features.\n\n> Regarding efficiency, I don't think it is a problem.  But patches or\n> concrete suggestions are certainly welcome.\n\nPre-optimization is the root of all evil :)  Can you give us some\nnumbers from real-world usecases, so we know whether or not it _needs_\nto be optimized?  I ran your test script (test-email, I think) and it\ngenerated 35 emails in ~10 seconds; but the repository was\nsuper-trivial.\n\n> Regarding separation of computation and rendering, yes, they could be\n> separated better.  (BTW, it would make the script even longer.)  The\n> rendering is already largely done via templates that can be changed from\n> outside of the script.  But I might work on separating them more\n> strictly so that some of the code could be reused, for example, to send\n> notifications via IRC or XMPP.\n\nMy reasons for wanting to separate out the computation from rendering\nare different: they centre around clarity, speed, ease of\nmaintainability and composability.  It's possible that I'm\nirrationally biased towards such a separation because of my experience\nwith MVC-like frameworks.  Then again, I don't have code to show; talk\nis cheap.\n\n> Thanks for all of your comments!  I hope I have addressed most of them\n> in this email and in the commits that I just pushed to GitHub.\n\nGlad to be of help.  Good turnaround time.\n"},{"id":"222463","messageId":"CALkWK0mCChJtLqDTiaw8Ji6==7Qh9AoBDpkde-siGVUMArpVpA@mail.gmail.com","threadId":"34326","inReplyTo":"CALkWK0=taYiV3UTaj9r-FLdaCeZRzVBTp_MH4sQt8-v+YYqbaA@mail.gmail.com","subject":"Re: Review of git multimail","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-07-03T11:02:14Z","receivedAt":"2013-07-03T11:02:14Z","isPatch":false,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Ramkumar Ramachandra wrote:\n>>> New-style class.  I wonder why you suddenly switched.\n>>\n>> ?  All of the classes are new-style classes.\n>\n> When you say class Foo:, aren't you declaring an old-style class by\n> default in python2?  New-style classes are those that explicitly\n> inherit from object (implicit in python3).\n\nI just noticed that all your classes are new-style.\n"},{"id":"222464","messageId":"51D40DD1.8010809@alum.mit.edu","threadId":"34326","inReplyTo":"CALkWK0=taYiV3UTaj9r-FLdaCeZRzVBTp_MH4sQt8-v+YYqbaA@mail.gmail.com","subject":"Re: Review of git multimail","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-07-03T11:41:05Z","receivedAt":"2013-07-03T11:41:05Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 07/03/2013 12:23 PM, Ramkumar Ramachandra wrote:\n> Michael Haggerty wrote:\n>> On 07/02/2013 09:23 PM, Ramkumar Ramachandra wrote:\n>>> git_multimail.py wrote:\n>>>> #! /usr/bin/env python2\n>>>\n>>> Do all distributions ship it as python2 now?\n>>\n>> No, but nor is \"python\" always Python version 2.x (I believe that Arch\n>> Linux now installs Python 3 as \"python\").  This topic was discussed here\n>> [1].  Basically, my reasoning is that (a) PEP 394 [2] says that\n>> \"python2\" is the correct name for a Python 2.x interpreter, (b) I\n>> believe that other distros are moving in that direction, and (c) if the\n>> script says \"python2\" but no python2 is installed, the error is pretty\n>> obvious, whereas if the script says \"python\" and that is actually Python\n>> 3.x, the error would be more cryptic.\n> \n> Yeah, this is good reasoning.  And yes, I'm on Arch: python points to\n> python3, and python2 points to python2.  A couple of thoughts while\n> we're on the subject:\n> \n> 1. We should probably convert git-remote-{hg,bzr} to use this style\n> too: they give cryptic errors now, and I have a ~/bin/python pointing\n> to python2 which is higher up in $PATH to work around it.  Debian uses\n> an alternatives mechanism to have multiple versions of the same\n> package, but I personally think the system is ugly.\n> \n> 2. Is there a way to determine the python version in-script and\n> error-out quickly?  Is it worth the ugliness?\n\nThe problems is that the whole script is parsed before execution starts,\nso using the wrong interpreter likely leads to a SyntaxError before the\nscript even gains control.\n\nThe correct solution, I'm afraid, is to have a build step that\ndetermines the correct Python shebang contents at build times and\nrewrites the script like the filename.perl -> filename transformations\nthat are already done for Perl.\n\n>>>>             'Command \"%s\" failed with retcode %s' % (' '.join(cmd), retcode,)\n>>>\n>>> So cmd is a list.\n>>\n>> Yes, commands are always lists in my code because it is less error-prone\n>> than trying to quote arguments correctly for the shell.  Do you think I\n>> should document that here, or elsewhere, or everywhere, or ...?\n> \n> If you look at the prototype of execvpe(), the calling semantics are\n> immediately clear, but we don't have that luxury in Python: probably\n> rename the variable cmd_argv?\n\nAdded to to-do list.\n\n> [...]\n>>>>     def get_recipients(self, name, default=None):\n>>>>         lines = self.get_all(name, default=None)\n>>>>         if lines is None:\n>>>>             return default\n>>>>         return ', '.join(line.strip() for line in lines)\n>>>\n>>> Ugh.\n>>\n>> ?\n> \n> I would expect it to return a list that can be churned on further, not\n> a string that's ready for rendering.  Doesn't it resemble the\n> dictionary get(), and even your own get_all() in name and signature?\n\nWe support both single and multiple-valued options, and each value can\ncontain multiple comma-separated RFC 2822 email addresses:\n\n    git config multimailhook.mailinglist \"larry@example.com\"\n    git config --add multimailhook.mailinglist \"moe@example.com,\ncurley@example.com\"\n    git config --add multimailhook.mailinglist '\"Shemp, the other one\"\n<shemp@example.com>'\n\n(I think that last one is valid.)\n\nSo we could turn the arguments into a list, but to be useful it would\nrequire the individual values to be parsed into possibly multiple\naddresses.  That seemed overkill, given that we only need the result as\na single string.\n\n> [...]\n>>> Dead code?\n>>\n>> git_multimail is used as a library by migrate-mailhook-config, and that\n>> script uses these methods.\n> \n> I see.  Perhaps clean separation to avoid confusing readers?\n\nThink of git_multimail.py as a library that can be included, e.g. by a\npost-receive script, but just happens to be executable as well.\n\nSplitting it up more would prevent a one-file install, which I think\nwould be unfortunate.\n\n> [...]\n>>>>             try:\n>>>>                 value = value % values\n>>>>             except KeyError, e:\n>>>>                 if DEBUG:\n>>>>                     sys.stderr.write(\n>>>>                         'Warning: unknown variable %r in the following line; line skipped:\\n'\n>>>>                         '    %s'\n>>>>                         % (e.args[0], line,)\n>>>>                         )\n>>>\n>>> If DEBUG isn't on, you risk leaving the value string interpolated\n>>> without even telling the user.  What does it mean to the end user?\n>>\n>> There are some \"nice-to-have\" values in the templates that are not\n>> necessary and might be missing if the user hasn't gone to the trouble to\n>> configure every last setting.  For example, if no emaildomain is defined\n>> then the pusher_email cannot be determined, resulting in the Reply-To\n>> header being omitted.\n>>\n>> My assumption is that a sysadmin would turn on DEBUG when testing the\n>> script, check that any missing headers are uninteresting, and then turn\n>> off DEBUG for production use so that users don't have to see the\n>> warnings every time they push.\n> \n> Ah, so that is the intended usage.  If the impact of omitting certain\n> headers (due to lack of information) doesn't result in unusable emails\n> being generated, I think we're good.  Are you sure that doesn't\n> happen?\n\nI believe that the Environment classes themselves will scream if a\nrequired value is missing, though of course all bets are off if the user\noverrides the templates.\n\n>>>>         raise NotImplementedError()\n>>>\n>>> Unsure what these are about.\n>>\n>> These are basically just to allow code sharing across the various Change\n>> classes.\n> \n> I'm not sure it's worth supporting Python < 2.6, especially if it\n> costs more to port it to Python 3+ (no abstract base classes; ugliness\n> like this).  Which brings me to: considering that the first commit is\n> in late 2012, why didn't you choose to code it in python3 directly?\n> Unless I'm mistaken, the only reason git-remote-{bzr,hg} aren't in\n> python3 is because the dependencies bzrlib and hglib are in python2.\n\nYes, someday.  But I think there are still widely-used server\ndistributions without a solid Python 3.x, and this script will mostly be\nused on servers.  Whereas I doubt that there will be any significant\ndistribution without a Python 2.x for the foreseeable future.\n\n> [...]\n>>>>         body = self.generate_email_body(push)\n>>>>         if body_filter is not None:\n>>>\n>>> Redundant \"is not None\".\n>>\n>> This way of writing the test is robust against objects for which\n>> bool(body_filter) might return False.\n> \n> If body_filter isn't a function, we have much larger problems, don't we? ;)\n> Nevertheless, I'm not going to argue with idioms (I've not written any\n> Python in years now, so I don't know them).\n\nbody_filter could be an instance of a class with a __call__() method and\na __nonzero__() method.  Since it is user-provided we should be as\nrobust as possible.\n\n>>> [...]\n>>>>         # First line of commit message:\n>>>>         try:\n>>>>             oneline = read_git_output(\n>>>>                 ['log', '--format=%s', '--no-walk', self.rev.sha1]\n>>>>                 )\n>>>>         except CommandError:\n>>>>             oneline = self.rev.sha1\n>>>\n>>> What does this mean?  When will you get a CommandError?\n>>\n>> I can't think of a plausible reason that this command would fail.\n>>\n>>>                                                          And how do\n>>> you respond to it?\n>>\n>> By using the commit's SHA-1 in place of its subject line.\n> \n> What you have written translates to: \"If there is a valid commit whose\n> subject cannot be determined (empty subject is still determinate), I\n> will use the commit's SHA-1 hex in its place\", which implies that you\n> do not trust git to be sane ;)\n> \n> Isn't the entire premise of your script that you have a sane git?\n\nIt is rather my own sanity that I doubt more than git's.  But I will\nremove this redundant error-handling.\n\n>> Yes, I could do that (though it needs quotes around the key strings).\n>> Or the even more attractive\n>>\n>>     values.update(\n>>         rev=self.rev.sha1,\n>>         rev_short=self.rev.short,\n>>         ...\n>>         )\n>>\n>> I had the latter in an earlier version of the script, but I thought it\n>> might be too unfamiliar for non-Python-experts.  I guess I'm using\n>> pretty highfalutin Python anyway so this change wouldn't hurt.  What do\n>> you think?\n> \n> I like pretty, maintainable, modern code with low redundancy.  As a\n> general principle, I always tilt towards educating users/ developers\n> about new (or \"advanced\") features, not abstaining from them because\n> they are too unfamiliar (or \"complicated\").\n> \n> On this specifically, a beginner can look up help(values.update) to\n> understand what the code is doing.  So, yes: values.update() is\n> definitely better.\n\nWill change.\n\n>>>>         contents = list(read_git_lines(['cat-file', 'tag', self.new.sha1], keepends=True))\n>>>\n>>> You could've easily batched this.\n>>\n>> I don't understand what you mean.\n> \n> Whenever I call cat-file from a script, I always find myself using the\n> --batch variant.\n\nOK, now I understand what you mean.  Yes, if people complain about\nperformance, this is one of the possibilities that I have in mind.\n\n>>> Way too many git invocations, I think.\n>>\n>> Luckily git is very fast :-)\n>>\n>> I'm not to worried about performance here.  The script will typically\n>> only be run on pushes, and most pushes affect only one or a few\n>> references.  I don't think these few git invocations will be prohibitive.\n> \n> I personally push very often, so it's not a problem.  I'm thinking of\n> a mirroring batched push, where the maintainer pushes out history to a\n> \"release\" server every major release (every few months): is the script\n> intended to be used in such a scenario, when multiple refs and tags\n> are updated non-trivially?\n\nThe script certainly supports such scenarios (at least modulo bugs).\nPerformance in a script like this is something that I try not to put\nextra work into unless there are obvious concerns or until somebody\ncomplains.\n\n>>> If you want to DWIM when the configuration variable is missing, do it\n>>> fully using a list of good candidates like /usr/lib/sendmail,\n>>> /usr/sbin/sendmail, /usr/ucblib/sendmail, /usr/bin/msmtp.\n>>\n>> OK, I just added /usr/sbin/sendmail and /usr/lib/sendmail, which are the\n>> paths checked by \"git send-mail\".\n> \n> I'm on Arch, and sendmail is dead (only available in AUR now).  I\n> think we should support modern sendmail-compatible alternatives like\n> msmtp (which I have and use).\n\nIt's configurable.  This is the kind of thing that I would expect the\nsysadmin (or the packager) to be familiar with, and I'd rather leave it\nto them than make lots of wild guesses about what might be installed.\n\nAFAIK there is a rather well-established convention that *whatever* mail\nprogram is installed, it should put some kind of sendmail-compatible\nbinary at /usr/sbin/sendmail.  Perhaps you should suggest this to the\nmsmtp packager?\n\n>>>                                                            Also, what\n>>> happened to our faithful 'git send-email' Perl script?  Isn't that\n>>> most likely to be installed?\n>>\n>> We could use \"git send-email\" to generate and send the revision emails,\n>> but then we would lose most control over the contents of the emails.\n> \n> I'm talking about the <file>... form.  Does it necessarily mangle the\n> headers of an mbox that is fed to it, or am I missing something?\n\nI only glanced at the documentation but it didn't look like an obvious\nimprovement over invoking sendmail directly or using smtplib.  But it\nwould be easy to add another Mailer class that delegates to \"git\nsend-email\" if somebody is motivated to do so.\n\n>>>> class Environment(object):\n>>>\n>>> New-style class.  I wonder why you suddenly switched.\n>>\n>> ?  All of the classes are new-style classes.\n> \n> When you say class Foo:, aren't you declaring an old-style class by\n> default in python2?  New-style classes are those that explicitly\n> inherit from object (implicit in python3).\n\nThat is correct.  But I don't think I use \"class Foo:\" anywhere in\ngit-multimail; in other words, I think I only use new-style classes in\nthis project.\n\n>>>> class GenericEnvironment(\n>>>>     ProjectdescEnvironmentMixin,\n>>>>     ConfigMaxlinesEnvironmentMixin,\n>>>>     ConfigFilterLinesEnvironmentMixin,\n>>>>     ConfigRecipientsEnvironmentMixin,\n>>>>     PusherDomainEnvironmentMixin,\n>>>>     ConfigOptionsEnvironmentMixin,\n>>>>     GenericEnvironmentMixin,\n>>>>     Environment,\n>>>>     ):\n>>>>     pass\n>>>\n>>> Sigh.  I might as well be reading some Java now :/\n>>\n>> No, Java doesn't allow multiple inheritance :-)\n> \n> Ha, yes: I meant in the way you factory'ified everything.  I don't\n> recall seeing this kind of code in a real-world application though: I\n> skimmed through Django's code many years ago, and haven't found such a\n> pattern.  I have seen instances where multiple inheritance may be\n> classified as borderline useful, but nothing as extensive as this.\n> Additionally, Ruby does not have multiple inheritance.  So, I tilt\n> towards the conclusion that multiple inheritance is Bad and Wrong.\n> Then again, I'm not an OO person in general, so I can't have a mature\n> opinion on the subject.  Can you present a case for this kind of\n> extensive usage quoting real-world examples?\n\nMultiple inheritance has to be used with great care.  I've never used it\nthis extensively before, so it was kindof an experiment for me.  But I\nam pleased with the result, especially that it makes it possible to\nseparate concerns quite well.\n\nI believe that Zope uses mixins quite extensively (and did so back in\nthe very old days), though that is not necessarily a strong endorsement :-)\n\n> [...]\n>> git_multimail.py is 2398 lines of Python script, including comments and\n>> string literals.  The fraction of that code that is dedicated to\n>> configurability is approximately 1000 lines.  Relative to\n>> post-receive-email, it adds\n>>\n>> * much more configurability, without the need to edit the script.\n> \n> So this is the main selling point.\n\nYes, that plus the one-email-per-commit feature and (hopefully!) better\nmaintainability due to the use of a more powerful language are the main\nselling points.\n\n> [...]\n>> Regarding efficiency, I don't think it is a problem.  But patches or\n>> concrete suggestions are certainly welcome.\n> \n> Pre-optimization is the root of all evil :)  Can you give us some\n> numbers from real-world usecases, so we know whether or not it _needs_\n> to be optimized?  I ran your test script (test-email, I think) and it\n> generated 35 emails in ~10 seconds; but the repository was\n> super-trivial.\n\nWe use the script at work for our main Git repo, which is quite large.\nOn the same repo we use a rather extensive pre-receive hook as well, so\nit is not obvious what part of the push time comes from each script.\nTogether they create a noticeable delay if many commits are being pushed\nat once, but (except when I once pushed thousands of commits at a time)\nthe total delay is max a few seconds.\n\nThanks again for your feedback!\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"222506","messageId":"87ppuzz6xr.fsf@mcs.anl.gov","threadId":"34326","inReplyTo":"CALkWK0=taYiV3UTaj9r-FLdaCeZRzVBTp_MH4sQt8-v+YYqbaA@mail.gmail.com","subject":"Re: Review of git multimail","fromName":"Jed Brown","fromEmail":"jed@59a2.org","sentAt":"2013-07-03T21:09:52Z","receivedAt":"2013-07-03T21:09:52Z","isPatch":false,"sender":{"key":"jed@59a2.org","avatar":"https://gravatar.com/avatar/1391d04d82555f9058a9fdf5eead233e909a48e40480db31fc554e7afeb301da?d=mp&s=160"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> Yeah, this is good reasoning.  And yes, I'm on Arch: python points to\n> python3, and python2 points to python2.  \n\nI'm also on Arch and it has been this way since October 2010 [1].\nUbuntu plans to remove python2 from the desktop CD images in 14.04 [2],\nso having code that does not work with python3 will become more painful\npretty soon.\n\nNote that RHEL5 has only python2.4 and will be supported through March,\n2017.  Since it is not feasible to have code that works in both python3\nand any versions prior to python2.6, any chosen dialect will be broken\nby default on some major distributions that still have full vendor\nsupport.\n\n> A couple of thoughts while we're on the subject:\n>\n> 1. We should probably convert git-remote-{hg,bzr} to use this style\n> too: \n\nPython-2.6.8 from python.org installs only python2.6 and python, but not\npython2, so this will break on a lot of older systems.  Some\ndistributions have been nice enough to provide python2 symlinks anyway.\n\nMichael's rationale that at least the error message is obvious still\nstands.\n\n> Debian uses an alternatives mechanism to have multiple versions of the\n> same package\n\nAlternatives is global configuration that doesn't really solve this\nproblem anyway.\n\n\n[1] https://www.archlinux.org/news/python-is-now-python-3/\n[2] https://wiki.ubuntu.com/Python/3\n"},{"id":"222532","messageId":"vpqvc4q68xm.fsf@anie.imag.fr","threadId":"34326","inReplyTo":"87ppuzz6xr.fsf@mcs.anl.gov","subject":"Re: Review of git multimail","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-07-04T08:11:49Z","receivedAt":"2013-07-04T08:11:49Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jed Brown <jed@59A2.org> writes:\n\n> Note that RHEL5 has only python2.4 and will be supported through March,\n> 2017.  Since it is not feasible to have code that works in both python3\n> and any versions prior to python2.6, any chosen dialect will be broken\n> by default on some major distributions that still have full vendor\n> support.\n\nAt worse, if git-multimail is ported to Python 3, people using old\ndistros will still be able to use today's version which works in Python\n2 and already does a good job.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"222533","messageId":"51D531D9.10203@alum.mit.edu","threadId":"34326","inReplyTo":"87ppuzz6xr.fsf@mcs.anl.gov","subject":"Re: Review of git multimail","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2013-07-04T08:27:05Z","receivedAt":"2013-07-04T08:27:05Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Thanks for all of the information.\n\nOn 07/03/2013 11:09 PM, Jed Brown wrote:\n> Ramkumar Ramachandra <artagnon@gmail.com> writes:\n> \n>> Yeah, this is good reasoning.  And yes, I'm on Arch: python points to\n>> python3, and python2 points to python2.  \n> \n> I'm also on Arch and it has been this way since October 2010 [1].\n> Ubuntu plans to remove python2 from the desktop CD images in 14.04 [2],\n> so having code that does not work with python3 will become more painful\n> pretty soon.\n\nIt may not be on the CD image, but python2 will undoubtedly continue to\nbe supported in the Ubuntu repositories; i.e., it is just an \"apt-get\ninstall\" away.  (For that matter, I don't think Git itself is on the\nUbuntu CD image.)\n\n> Note that RHEL5 has only python2.4 and will be supported through March,\n> 2017.  Since it is not feasible to have code that works in both python3\n> and any versions prior to python2.6, any chosen dialect will be broken\n> by default on some major distributions that still have full vendor\n> support.\n\nI think for a server-oriented program like git-multimail it is more\nimportant to support old versions of Python than to support the\nbleeding-edge versions.  For user-oriented programs a different\nconclusion might be reached.\n\nMy vague, long-term plan is roughly:\n\n* Continue to support Python 2.4 or at least 2.5 for the next year or\ntwo.  This excludes any reasonable hope of simultaneously being Python\n3.x compatible, so don't worry about 3.x for now (though\nbackwards-compatible and non-hideous changes that move in the direction\nof Python 3.x compatibility are of course welcome).\n\n* At some point, abandon support for the older Python 2.x releases and\nstart using 3.x-compatibility features that were added in Python 2.6 and\n2.7.\n\n* Make string handling Unicode-correct.\n\n* Then evaluate the situation and decide between two courses of action:\n\n  * Evolve the script to work with both Python 2.7 (and maybe 2.6) and\nPython 3.3 (and maybe 3.2) simultaneously.\n\n  * Fork development of a Python 3.x version while retaining a 2.x\nversion in maintenance mode.\n\n>> A couple of thoughts while we're on the subject:\n>>\n>> 1. We should probably convert git-remote-{hg,bzr} to use this style\n>> too: \n> \n> Python-2.6.8 from python.org installs only python2.6 and python, but not\n> python2, so this will break on a lot of older systems.  Some\n> distributions have been nice enough to provide python2 symlinks anyway.\n> \n> Michael's rationale that at least the error message is obvious still\n> stands.\n\nThe approach I've taken in git-multimail isn't necessarily applicable to\ngit-remote-*.  The main difference is that git-multimail *has to* be\ninstalled by a repository administrator to have an effect (either by\nbeing copied or linked to $GIT_DIR/hooks or by adding a script there\nthat refers to git_multimail.py).  So the admin, at that time, can\ndecide what python is best to use on his system and adjust the shebang\nline or create a \"python2\" symlink or whatever.\n\nThe git-remote-* scripts are meant for users, who simply want to execute\nthem without thinking.  So in that case, the scripts should be installed\nto the default $PATH with the correct shebang line already in place for\nthe local environment.  Thus their shebang lines will tend to be decided\nby packagers, not end users, and this difference changes the situation.\n I think they should be managed via build step that rewrites the scripts\nto use a build-time-configured Python interpreter.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"}]}