incubator-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:04:17 GMT
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?

Or at least, follow the policy that:
1. Contributor is author
2. Committer need to provide Signed-off.

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

Mime
View raw message