incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Edison Su <Edison...@citrix.com>
Subject RE: [DISCUSS]Patch format/applying policy for contributors and committers
Date Wed, 12 Sep 2012 22:36:17 GMT


> -----Original Message-----
> From: Sheng Yang [mailto:sheng@yasker.org]
> Sent: Wednesday, September 12, 2012 2:36 PM
> To: cloudstack-dev@incubator.apache.org
> Subject: Re: [DISCUSS]Patch format/applying policy for contributors and
> committers
> 
> On Wed, Sep 12, 2012 at 1:42 PM, Chip Childers
> <chip.childers@sungard.com> wrote:
> > On Wed, Sep 12, 2012 at 4:35 PM, Sheng Yang <sheng@yasker.org> wrote:
> >> Hi,
> >>
> >> I think we would need some way to make the commit more clear.
> >>
> >> Currently committers are committed the patch in any format he/she
> >> want. E.g. using reviews.apache.org 's ticket number as subject, add
> >> some "Contributed-by:" or "Sent-by:" or "Wrote-by:" in the context
> to
> >> identify the contributor(but committer themselves remained "Author"),
> >> which is inconsistent across the committers, and hard for statistics.
> >>
> >> Also many commits lacks of proper explanation and subjects to be
> >> easily identified.
> >>
> >> So the best way to me, is let contributor listed as Author in the
> >> commit, and committer can "Signed-off" on it.
> >>
> >> I suggest we can follow the Linux Kernel community case on this
> issue.
> >>
> >> 1. the contributor should provide git-format-patch generated patch,
> >> which is able to indicate who is the author and what's to fix.
> >> 2. Committer should use git-am to apply the patch, add Signed-off-by:
> >> xxxx(xxxx@citrix.com) at end of it, to indicate who agreed and
> >> committed this patch(in fact there are more potential rules here,
> but
> >> let's stick with the simplest one first).
> >>
> >> Then author need to write the subject and more detail explaining of
> >> the patch if necessary, and it would be easier for others to track
> >> which patch is from who on what issue, and what's the solution.
> >>
> >> Another advantage/disadvantage of  git-am is, it's very strict. You
> >> have provided up-to-date patch, otherwise it would fail. No fuzz is
> >> allowed.
> >>
> >> One problem currently is reviews.apache.org doesn't accept the git
> >> formatted patch, it would only accept diff. So if we want to get
> patch
> >> from reviews.apache.org, it would result in committer have to find
> >> subject and detail explanation for the patch, which may varies from
> >> author's, and also increase committer's workload.
> >>
> >> I think probably we can let contributor send out two mail for each
> >> patch? One attached latest patch, another one which is sent by
> >> reviews.apache.org, provided a convenient way for reviewing? The
> >> alternative way of doing is let contributor send out
> >> git-format-patch-ed patch(which would be based on the latest code)
> >> after committer reviewed patch and think it's ready to be applied.
> >>
> >> --Sheng
> >>
> >
> > Hey Sheng,
> >
> > Given the current limits of reviews.a.o, I've recently turned to
> using
> > this process:
> >
> > git apply foo.patch
> > git commit -a -s --author='Person Name <email@test.com>'
> >
> > Then, I've been putting in the reviewboard summary and description
> > into the commit comment (sometimes edited to help with clarity),
> which
> > should be easily available since the patch was just downloaded from
> > reviewboard anyway.
> >
> > This gives us the correct author, correct committer and a decent
> commit message.
> 
> Hi Chip,
> 
> Yes, this works, though still committer need to do some tweak on the
> patches, and more you do, the more chance you got to mess up
> something. To me, what's the advantage of git-am is, it would
> guarantee what's original author wanted information is there, as well
> as code is intact.
> 
> Currently I think your approach is good, but we need to make it as a
> policy and require every committer to do the same as well.
> >
> > Personally, I'd like to avoid double emails.  We also know that the
> > dev list both truncates (and sometimes mangles) patches sent in the
> > text of emails, as well as stripping attachments out.  Would the
> > method I'm describing above work for you?
> 
> I think it's fine for now, but when volume of patches and workload
> increase, I suppose we still need to figure out some way more
> directly.

http://wiki.cloudstack.org/display/gen/Review%20Board
http://markmail.org/message/zuppmf7un7pt6q65
Rohit, had a tool to send original patch to review board, committer can easily download the
patch from a link in the review board, the "git apply".
> 
> --Sheng
> >
> > -chip

Mime
View raw message