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 <[email protected]> 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 <[email protected]> >> 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 <[email protected]> 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 < >>>>> [email protected]> >>>>> 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 <[email protected]> wrote: >>>>>> >>>>>> commit 9856080ede62a4529d730bcb6724c757f5010990 >>>>>> >>>>>>> Author: Pramod Immaneni & Vlad Rozov <[email protected] >>>>>>> > >>>>>>> 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 < >>>>>>> >>>>>>> [email protected] >>>>>> 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 <[email protected]> >>>>>>>> >>>>>>>> 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 < >>>>>>>>> >>>>>>>>> [email protected] >>>>>>>> >>>>>>> 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 < >>>>>>>>>>> >>>>>>>>>>> [email protected]> >>>>>>>>>> >>>>>>>>> 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: [email protected] | M: (408) 331-5034 | Twitter: @UnknownRam www.datatorrent.com | apex.apache.org
