My thought was that leaving the bad commit would be a permanent reminder to the committer (and others) that a policy violation occurred and the consequent embarrassment would be an adequate deterrent.
Ram On Thu, Apr 27, 2017 at 9:12 PM, Vlad Rozov <v.ro...@datatorrent.com> wrote: > I also was under impression that everyone agreed to the policy that gives > everyone in the community a chance to raise a concern or to propose an > improvement to a PR. Unfortunately, it is not the case, and we need to > discuss it again. I hope that this discussion will lead to no future > violations so we don't need to forcibly undo such commits, but it will be > good for the community to agree on the policy that deals with violations. > > Ram, committing an improvement on top of a commit should be discouraged, > not encouraged as it eventually leads to the policy violation and lousy PR > reviews. > > Thank you, > > Vlad > > On 4/27/17 20:54, Thomas Weise wrote: > >> I also thought that everybody was in agreement about that after the first >> round of discussion and as you say it would be hard to argue against it. >> And I think we should not have to be back to the same topic a few days >> later. >> >> While you seem to be focussed on the disagreement on policy violation, I'm >> more interested in a style of collaboration that does not require such >> discussion. >> >> Thomas >> >> On Thu, Apr 27, 2017 at 8:45 PM, Munagala Ramanath <r...@datatorrent.com> >> wrote: >> >> Everybody seems agreed on what the committers should do -- that waiting a >>> day or two for >>> others to have a chance to comment seems like an entirely reasonable >>> thing. >>> >>> The disagreement is about what to do when that policy is violated. >>> >>> Ram >>> >>> On Thu, Apr 27, 2017 at 8:37 PM, Thomas Weise <t...@apache.org> wrote: >>> >>> Forced push should not be necessary if committers follow the development >>>> process. >>>> >>>> So why not focus on what the committer should do? Development process >>>> and >>>> guidelines are there for a reason, and the issue was raised before. >>>> >>>> In this specific case, there is a string of commits related to the >>>> plugin >>>> feature that IMO should be part of the original review. There wasn't any >>>> need to rush this, the change wasn't important for the release. >>>> >>>> Thomas >>>> >>>> >>>> On Thu, Apr 27, 2017 at 8:11 PM, Munagala Ramanath <r...@datatorrent.com >>>> > >>>> wrote: >>>> >>>> I agree with Pramod that force pushing should be a rare event done only >>>>> when there is an immediate >>>>> need to undo something serious. Doing it just for a policy violation >>>>> >>>> should >>>> >>>>> itself be codified in our >>>>> policies as a policy violation. >>>>> >>>>> Why not just commit an improvement on top ? >>>>> >>>>> Ram >>>>> >>>>> On Thu, Apr 27, 2017 at 7:55 PM, Vlad Rozov <v.ro...@datatorrent.com> >>>>> wrote: >>>>> >>>>> Violation of the PR merge policy is a sufficient reason to forcibly >>>>>> >>>>> undo >>>> >>>>> the commit and such violations affect everyone in the community. >>>>>> >>>>>> Thank you, >>>>>> >>>>>> Vlad >>>>>> >>>>>> On 4/27/17 19:36, Pramod Immaneni wrote: >>>>>> >>>>>> I agree that PRs should not be merged without a chance for others to >>>>>>> review. I don't agree to force push and altering the commit tree >>>>>>> >>>>>> unless >>>> >>>>> it >>>>> >>>>>> is absolutely needed, as it affects everyone. This case doesn't >>>>>>> >>>>>> warrant >>>> >>>>> this step, one scenario where a force push might be needed is if >>>>>>> >>>>>> somebody >>>>> >>>>>> pushed some copyrighted code. >>>>>>> >>>>>>> Thanks >>>>>>> >>>>>>> On Thu, Apr 27, 2017 at 6:44 PM, Vlad Rozov < >>>>>>> >>>>>> v.ro...@datatorrent.com> >>> >>>> wrote: >>>>>>> >>>>>>> I am open to both approaches - two commits or a join commit. Both >>>>>>> >>>>>> have >>> >>>> pros and cons that we may discuss. What I am strongly against are >>>>>>>> >>>>>>> PRs >>> >>>> that >>>>>>>> are merged without a chance for other contributors/committers to >>>>>>>> >>>>>>> review. >>>>> >>>>>> There should be a way to forcibly undo such commits. >>>>>>>> >>>>>>>> Thank you, >>>>>>>> >>>>>>>> Vlad >>>>>>>> >>>>>>>> >>>>>>>> On 4/27/17 12:41, Pramod Immaneni wrote: >>>>>>>> >>>>>>>> My comments inline.. >>>>>>>> >>>>>>>>> On Thu, Apr 27, 2017 at 12:01 PM, Thomas Weise <t...@apache.org> >>>>>>>>> >>>>>>>> wrote: >>>>> >>>>>> I'm -1 on using the author field like this in Apex for the reason >>>>>>>>> >>>>>>>> stated >>>>> >>>>>> (it is also odd to see something like this showing up without >>>>>>>>>> >>>>>>>>> prior >>> >>>> discussion). >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> I am not set on this for future commits but would like to say, do >>>>>>>>>> >>>>>>>>> we >>>> >>>>> really >>>>>>>>> verify the author field and treat it with importance. For >>>>>>>>> >>>>>>>> example, I >>> >>>> don't >>>>>>>>> think we ever check if the author is the person they say they are, >>>>>>>>> >>>>>>>> check >>>>> >>>>>> name, email etc. If I were to use slightly different variations of >>>>>>>>> >>>>>>>> my >>>> >>>>> name >>>>>>>>> or email (not that I would do that) would reviewers really verify >>>>>>>>> >>>>>>>> that. >>>>> >>>>>> I >>>>>>>>> also have checked that tools don't fail on reading a commit >>>>>>>>> >>>>>>>> because >>> >>>> author >>>>>>>>> needs to be in a certain format. I guess contribution stats are >>>>>>>>> >>>>>>>> the >>> >>>> ones >>>>> >>>>>> that will be affected but if used rarely I dont see that being a >>>>>>>>> >>>>>>>> big >>> >>>> problem. I can understand if we wanted to have strict requirements >>>>>>>>> >>>>>>>> for >>>> >>>>> the >>>>>>>>> committer field. >>>>>>>>> >>>>>>>>> Thanks >>>>>>>>> >>>>>>>>> >>>>>>>>> Consider adding the additional author information to the commit >>>>>>>>> >>>>>>>> message. >>>>> >>>>>> Thomas >>>>>>>>>> >>>>>>>>>> On Thu, Apr 27, 2017 at 11:55 AM, Pramod Immaneni < >>>>>>>>>> pra...@datatorrent.com> >>>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> Agreed it is not regular and should only be used in special >>>>>>>>>> circumstances. >>>>>>>>>> >>>>>>>>>> One example of this is pair programming. It has been done before >>>>>>>>>> >>>>>>>>> in >>> >>>> other >>>>>>>>>>> projects and searching on google or stackoverflow you can see >>>>>>>>>>> >>>>>>>>>> how >>> >>>> other >>>>>>>>>>> people have tried to handle it >>>>>>>>>>> >>>>>>>>>>> https://bugs.eclipse.org/bugs/show_bug.cgi?id=375536 >>>>>>>>>>> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=451880 >>>>>>>>>>> http://stackoverflow.com/questions/7442112/attributing- >>>>>>>>>>> a-single-commit-to-multiple-developers >>>>>>>>>>> >>>>>>>>>>> Thanks >>>>>>>>>>> >>>>>>>>>>> On Thu, Apr 27, 2017 at 9:57 AM, Thomas Weise <t...@apache.org> >>>>>>>>>>> >>>>>>>>>> wrote: >>>>> >>>>>> commit 9856080ede62a4529d730bcb6724c757f5010990 >>>>>>>>>>> >>>>>>>>>>> Author: Pramod Immaneni & Vlad Rozov >>>>>>>>>>>> >>>>>>>>>>> <pramod+v.rozov@datatorrent. >>> >>>> com >>>>> >>>>>> Date: Tue Apr 18 09:37:22 2017 -0700 >>>>>>>>>>>> >>>>>>>>>>>> Please don't use the author field in such a way, it leads to >>>>>>>>>>>> incorrect >>>>>>>>>>>> tracking of contributions. >>>>>>>>>>>> >>>>>>>>>>>> Either have separate commits or have one author. >>>>>>>>>>>> >>>>>>>>>>>> Thanks >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On Thu, Apr 27, 2017 at 9:31 AM, Pramod Immaneni < >>>>>>>>>>>> >>>>>>>>>>>> pra...@datatorrent.com >>>>>>>>>>>> >>>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>> The issue was two different plugin models were developed, one >>>>>>>>>>>> >>>>>>>>>>> for >>> >>>> pre-launch and other for post-launch. I felt that the one >>>>>>>>>>>>> >>>>>>>>>>>> built >>> >>>> latter >>>>>>>>>>>>> >>>>>>>>>>>> was >>>>>>>>>>> >>>>>>>>>>> better and it would be better to have a uniform interface for >>>>>>>>>>>> >>>>>>>>>>> the >>> >>>> users >>>>>>>>>>>>> >>>>>>>>>>>> and >>>>>>>>>>> >>>>>>>>>>> hence asked for the changes. >>>>>>>>>>>> >>>>>>>>>>>>> On Thu, Apr 27, 2017 at 9:05 AM, Thomas Weise <t...@apache.org >>>>>>>>>>>>> wrote: >>>>>>>>>>>>> >>>>>>>>>>>> I think the plugins feature could have benefited from better >>>>>>>>>>> >>>>>>>>>>> original >>>>>>>>>>>> review, which would have eliminated much of the back and forth >>>>>>>>>>>> after >>>>>>>>>>>> the >>>>>>>>>>>> fact. >>>>>>>>>>>> >>>>>>>>>>>>> On Thu, Apr 27, 2017 at 8:20 AM, Vlad Rozov < >>>>>>>>>>>>>> >>>>>>>>>>>>>> v.ro...@datatorrent.com >>>>>>>>>>>>>> >>>>>>>>>>>>> wrote: >>>>>>>>>>>> Pramod, >>>>>>>>>>>> >>>>>>>>>>>>> No, it is not a request to update the apex.apache.org (to >>>>>>>>>>>>>>> >>>>>>>>>>>>>> do >>> >>>> that >>>>>>>>>>>>>>> >>>>>>>>>>>>>> we >>>>>>>>>>>>> >>>>>>>>>>>> need >>>>>>>>>>> >>>>>>>>>>>> to file JIRA). It is a reminder that it is against Apex policy >>>>>>>>>>>>> >>>>>>>>>>>> to >>>> >>>>> merge >>>>>>>>>>>>>>> >>>>>>>>>>>>>> PR >>>>>>>>>>>>> >>>>>>>>>>>>> without giving enough time for others to review it (few hours >>>>>>>>>>>>>> >>>>>>>>>>>>>>> after >>>>>>>>>>>>>>> >>>>>>>>>>>>>> PR >>>>>>>>>>>>> >>>>>>>>>>>> was >>>>>>>>>>>> >>>>>>>>>>>>> open). >>>>>>>>>>>>>> >>>>>>>>>>>>>>> Thank you, >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Vlad >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On 4/27/17 08:05, Pramod Immaneni wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Vlad, are you asking for a consensus on the policy to make >>>>>>>>>>>>>>> >>>>>>>>>>>>>> it >>> >>>> official >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> (publish on website etc). I believe we have one. However, I >>>>>>>>>>>>>> >>>>>>>>>>>>> did >>> >>>> not >>>>>>>>>>>>>> see >>>>>>>>>>>>>> >>>>>>>>>>>>> much difference between what you said on Mar 26th to what I >>>>>>>>>>>>> >>>>>>>>>>>>>> proposed >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> on >>>>>>>>>>>>>> >>>>>>>>>>>>> Mar >>>>>>>>>>>>> >>>>>>>>>>>>>> 24th. Is the main difference any committer can merge (not >>>>>>>>>>>>>>> >>>>>>>>>>>>>> just >>> >>>> the >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> main >>>>>>>>>>>>>> >>>>>>>>>>>>> reviewer) as long as there are no objections from others. In >>>>>>>>>>>> >>>>>>>>>>>>> that >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> case, >>>>>>>>>>>>>> >>>>>>>>>>>>> I >>>>>>>>>>>> >>>>>>>>>>>>> am fine with it. >>>>>>>>>>>>>> >>>>>>>>>>>>>>> On Thu, Apr 27, 2017 at 7:44 AM, Vlad Rozov < >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> v.ro...@datatorrent.com> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> wrote: >>>>>>>>>>>>>> This is a friendly reminder regarding PR merge policy. >>>>>>>>>>>>>> >>>>>>>>>>>>>>> Thank you, >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Vlad >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> On 3/23/17 12:58, Vlad Rozov wrote: >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Lately there were few instances where PR open against >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> apex-core >>>>> >>>>>> and >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> apex-malhar were merged just few hours after it being open >>>>>>>>>>>>>>> >>>>>>>>>>>>>> and >>> >>>> JIRA >>>>>>>>>>>>> >>>>>>>>>>>>>> being >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> raised without giving chance for other contributors to review >>>>>>>>>>>>>> >>>>>>>>>>>>>>> and >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> comment. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> I'd suggest that we stop such practice no matter how trivial >>>>>>>>>>>>> >>>>>>>>>>>>>> those >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> changes >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> are. This equally applies to documentation. In a rear cases >>>>>>>>>>>>> >>>>>>>>>>>>>> where >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> PR >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> is >>>>>>>>>>>> >>>>>>>>>>>>> urgent (for example one that fixes compilation error), I'd >>>>>>>>>>>>>> >>>>>>>>>>>>>>> suggest >>>>>>>>>>>>>>>> that >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> a >>>>>>>>>>>>> >>>>>>>>>>>>>> committer who plans to merge the PR sends an explicit >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> notification >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> to >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> dev@apex and gives others a reasonable time to respond. >>>>>>>>>>>>> >>>>>>>>>>>>>> Thank you, >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Vlad >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>> -- >>>>> >>>>> _______________________________________________________ >>>>> >>>>> Munagala V. Ramanath >>>>> >>>>> Software Engineer >>>>> >>>>> E: r...@datatorrent.com | M: (408) 331-5034 | Twitter: @UnknownRam >>>>> >>>>> www.datatorrent.com | apex.apache.org >>>>> >>>>> >>> >>> -- >>> >>> _______________________________________________________ >>> >>> Munagala V. Ramanath >>> >>> Software Engineer >>> >>> E: r...@datatorrent.com | M: (408) 331-5034 | Twitter: @UnknownRam >>> >>> www.datatorrent.com | apex.apache.org >>> >>> > -- _______________________________________________________ Munagala V. Ramanath Software Engineer E: r...@datatorrent.com | M: (408) 331-5034 | Twitter: @UnknownRam www.datatorrent.com | apex.apache.org
