[RFC] Store patch authors in the new Patch.author field
guilherme.salgado at linaro.org
Sat Apr 30 00:04:21 EST 2011
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:
> 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)
> > + if first_line.startswith('From:'):
> > + emails = extract_email_addresses(first_line)
> > + if len(emails) == 1:
> > + email = emails
> > + try:
> > + person = Person.objects.get(email__iexact=email)
> > + except Person.DoesNotExist:
> > + name = email.split('@', 1)
> 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?
Guilherme Salgado <https://launchpad.net/~salgado>
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 198 bytes
Desc: This is a digitally signed message part
More information about the Patchwork