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

[Michael Haggerty <mhagger@alum.mit.edu>]

On 02/14/2013 01:55 PM, Matthieu Moy wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> 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.
> 
>>> @@ -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.

I guess it depends a lot on what logopts are used.  If the user
configures logopts to emit full patches, then the repeated reporting of
the same commits would cause a big increase in the bulk of notification
emails.  But if the logopts are set to just emit a brief summary (e.g.,
author and log message), then a bit of repetition might be acceptable.
But since I wouldn't use this feature, I don't personally have a preference.

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

I will check it out.

Thanks!

Michael

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