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.ro...@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 >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >
