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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Feb 14, 2013, 12:55 UTC
Message-ID: <vpq7gmbdpi2.fsf@grenoble-inp.fr>
In-Reply-To: <511C08AF.7090502@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

Show 13 quoted lines

> On 02/13/2013 03:56 PM, Matthieu Moy wrote:
>
>> 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?

Python 2.4.3, installed with RHEL 5.9.

> I just discovered that the script wouldn't have worked with Python
> 2.4, where "email.utils" used to be called "email.Utils".

Indeed, "import email.Utils" works with this Python.

> But I pushed a fix to GitHub:
>
>     ddb1796660 Accommodate older versions of Python's email module.

Not sufficient, but I added a pull request that works for me with 2.4.

Show 26 quoted lines

>> @@ -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?)

I did not think very carefully about this case, but the behavior of my code seems sensible (although not uncontroversial): it's just showing the detailed log for the same commits as the summary at the top of the email. I have no personnal preferences.

> 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).

Right.

> 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)).

I updated the pull request with a version that works for new branches, and takes the list of commits to display from the call to get_new_commits (which were already there for other purpose). Then, it essentially calls "git log --no-walk $list_of_sha1s".

This should be better.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/