Re: [RFC v2] git-multimail: a replacement for post-receive-email

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Feb 13, 2013, 21:42 UTC
Message-ID: <511C08AF.7090502@alum.mit.edu>
In-Reply-To: <vpqtxpgb6ue.fsf@grenoble-inp.fr>

On 02/13/2013 03:56 PM, Matthieu Moy wrote:

Show 16 quoted lines

> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> A while ago, I submitted an RFC for adding a new email notification
>> script to "contrib" [1].  The reaction seemed favorable and it was
>> suggested that the new script should replace post-receive-email rather
>> than be added separately, ideally with some kind of migration support.
> 
> I think replacing the old post-receive-email is a sane goal in the long
> run, but a good first step would be to have git-multimail merged in
> contrib, and start considering the old script as deprecated (keeping the
> old script doesn't harm IMHO, it's a one-file, 3 commits/year script,
> not really a maintainance burden).
> 
> I started playing with git-multimail. In short, I do like it but had to
> fight a bit with python to get it to work, and couldn't get it to do
> exactly what I expect. Pull request attached :-).

Thanks very much for your feedback and patches.

Show 9 quoted lines

> Installation troubles:
> 
> I had an old python installation (Red Hat package, and I'm not root),
> that did not include the email.utils package, so I couldn't use my
> system's python. I found no indication about python version in README,
> so I installed the latest python by hand, just to find out that
> git-multimail wasn't compatible with Python 3.x. 2to3 can fix
> automatically a number of 3.x compatibility issues, but not all of them
> so I gave up and installed Python 2.7.

What version of Python was it that caused problems? I just discovered that the script wouldn't have worked with Python 2.4, where "email.utils" used to be called "email.Utils". But I pushed a fix to GitHub:

    ddb1796660 Accommodate older versions of Python's email module.

With this change, I think that git-multimail will work with any version of Python 2.4 <= x < 3.0. So if your original problem was with Python 2.4, maybe you could try the new version and see if it works with that interpreter.

Show 5 quoted lines

> I think adding a short "dependencies" section in the README (or in an
> INSTALL file) saying which Python version works could save new users the
> trouble (I see the sheebang inside the scripts says python2 but since I
> couldn't use my system's python and called
> "path/to/python git_multimail.py", this didn't help).

Yes, I'm working on a "Requirements" section with that information and
more.  I'd like to list a minimum git version too, but it would be quite
a bit of work to figure out when each command and each option was added.
 It would be helpful if anybody who has used the script with an old
version of git lets me know whether they were successful or not.

> Making the script
> portable with python 2 and 3 would be awesome ;-).

Agreed, but I doubt I will be able to get to it very soon.

Show 37 quoted lines

> Missing feature:
> 
> git-multimail can send a summary for each push, with the "git log --oneline"
> of the new revisions, and then 1 mail per patch with the complete log
> and the patch.
> 
> I'd like to have the intermediate: allow the summary email to include
> the complete log (not just --oneline). My colleagues already think they
> receive too many emails so I don't think they'd like the "one email per
> commit" way, but the 1 line summary is really short OTOH.
> 
> I wrote a quick and hopefully not-too-dirty implementation of it,
> there's a pull request here:
> 
> https://github.com/mhagger/git-multimail/pull/6
> 
> essentially, it boils down to:
> 
> @@ -835,6 +837,17 @@ class ReferenceChange(Change):
>                  for line in self.expand_lines(NO_NEW_REVISIONS_TEMPLATE):
>                      yield line
>  
> +            if adds and self.showlog:
> +                yield '\n'
> +                yield 'Detailed log of added commits:\n\n'
> +                for line in read_lines(
> +                        ['git', 'log']
> +                        + self.logopts
> +                        + ['%s..%s' % (self.old.commit, self.new.commit,)],
> +                        keepends=True,
> +                        ):
> +                    yield line
> +
>              # The diffstat is shown from the old revision to the new
>              # revision.  This is to show the truth of what happened in
>              # this change.  There's no point showing the stat from the
>

Thanks for the patch. I like the idea, but I think the implementation is incorrect. Your code will not only list new commits but will also list commits that were already in the repository on another branch (e.g., if an existing feature branch is merged into master, all of the commits on the feature branch will be listed). (Or was that your intention?) But even worse, it will fail to list commits that were added at the same time that a branch was created (e.g., if I create a feature branch with a number of commits on it and then push it for the first time).

Probably the Push object has to negotiate with its constituent ReferenceChange objects to figure out which one is responsible for summarizing each of the commits newly added by the push (i.e., the ones returned by push.get_new_commits(None)).

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/