cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sheng Yang <sh...@yasker.org>
Subject Re: [DISCUSS]Patch format/applying policy for contributors and committers
Date Wed, 12 Sep 2012 23:10:38 GMT
On Wed, Sep 12, 2012 at 4:07 PM, Edison Su <Edison.su@citrix.com> wrote:
>
>
>> -----Original Message-----
>> From: Sheng Yang [mailto:sheng@yasker.org]
>> Sent: Wednesday, September 12, 2012 4:04 PM
>> To: cloudstack-dev@incubator.apache.org
>> Subject: Re: [DISCUSS]Patch format/applying policy for contributors and
>> committers
>>
>> On Wed, Sep 12, 2012 at 3:36 PM, Edison Su <Edison.su@citrix.com> wrote:
>> >
>> >
>> >> -----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".
>>
>> Yes, this is nice!
>>
>> Could we make it mandatory?
>
> You can't make it mandatory, as people always can upload patch from review board UI,
instead of command line.

An recommendation on "How to submit patches for CloudStack" would be helpful.
>
>>
>> Or at least, follow the policy that:
>> 1. Contributor is author
>> 2. Committer need to provide Signed-off.

And this policy should able to be enforced by committers.

--Sheng

>>
>> --Sheng
>> >>
>> >> --Sheng
>> >> >
>> >> > -chip

Mime
View raw message