[RFC] Store patch authors in the new Patch.author field

[Guilherme Salgado]

On Fri, 2011-04-29 at 10:10 +0800, Jeremy Kerr wrote:
> Hi Guilherme,
> 
> > When the first line of the email message that contains a patch starts with
> > 'From:' and contain an email address, we use that address as the author;
> > otherwise we use the same as the submitter.
> > 
> > Currently this information is not shown anywhere but it could be easily
> > exposed in the UI and or over XML-RPC. I'd be happy to do that if there are no
> > objections to this patch.
> 
> I'd be happy to apply this; the tricky bit will be how to expose this in
> the UI, but we can work that out separately.

Cool, thanks for the quick feedback!

> 
> One question though: should we allow From: to be on any line of a patch,
> rather than the first? Consider something like:
> 
>  http://patchwork.ozlabs.org/patch/92959/
> 
> For ubuntu-kernel, when proposing an update patch, we forward the entire
> commit (possibly with comments before it, like why we're proposing the
> patch for that ubuntu kernel version). In this case, it'd be correct to
> parse the *last* From: line before the patch itself.

The reason why I decided to only override the author when the first line
starts with the 'From:' string is that others expressed concerns with
the chance of false-positives if we accepted a 'From:' anywhere in the
message.  Also, as Peter Maydell pointed out, it's probably a good idea
to be compatible with git-am, which only accept the author to be in the
first line (or the second when there's a new Subject as well).

> 
> A couple of comments:
> 
> > +def find_author(comment):
> > +    """Return a Person entry for the author specified in the given comment.
> > +
> > +    The author of a patch is sometimes not the person who's submitting it, and
> > +    in these cases the first line of the email will specify the author, using
> > +    the following format:
> > +
> > +        From: Somebody <somebody at somewhere.tld>
> > +
> > +    When the first line starts with 'From:' and contain an email address, we
> > +    return a Person entry representing that email address, creating a new one
> > +    if necessary.  If the first line doesn't start with 'From:' or it doesn't
> > +    contain an email address, we return None.
> > +    """
> > +    if not comment:
> > +        return None
> > +    first_line = comment.content.split('\n', 1)[0]
> > +    if first_line.startswith('From:'):
> > +        emails = extract_email_addresses(first_line)
> > +        if len(emails) == 1:
> > +            email = emails[0]
> > +            try:
> > +                person = Person.objects.get(email__iexact=email)
> > +            except Person.DoesNotExist:
> > +                name = email.split('@', 1)[0]
> 
> hm, we shouldn't be using the username part of the email address here,
> instead look for something in the format:
> 
>  "Firstname Lastname <me at example.com>"
> 
> and rather than re-implementing this, It'd be best to abstract
> find_author (which already handles different email formats), and use it
> in both author and submitter parsing.

Sure, I'm happy to do that.

> 
> > +                person = Person(name=name, email=email)
> > +                person.save()
> > +            return person
> > +        elif len(emails) == 0:
> > +            return None
> > +        else:
> > +            raise AssertionError(
> > +                'This patch has more than one author: %s.' % first_line)
> 
> I'd prefer we just pick one here, rather than aborting the parse

I'm not fond of raising an AssertionError here, but I'm not sure
arbitrarily picking one of the emails is a reasonable option.  Maybe a
highly visible warning of some sort?

Cheers,

-- 
Guilherme Salgado <https://launchpad.net/~salgado>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/patchwork/attachments/20110429/e3444a46/attachment.pgp>