I recommend the practice. Although the signoff may not be authoritative, it requires a positive action that suggests you purposefully merged the commit, as opposed to commits you might have accidentally merged and pushed.
Thanks, James On Thu, Mar 2, 2017 at 7:49 AM, Joe Witt <joe.w...@gmail.com> wrote: > "If this not the expected process, we should definitely update the > Contributor Guide." > > I think it is fine to encourage it. It is not a requirement though. > > The signoff is not an apache thing. Committer privileges to push code > to a given repo is an apache thing. > > We're an RTC community and the only thing we do require is a +1 from > another committer before the patch/PR is merged. It is perfectly > sufficient for the reviewer to add a +1 on github comments or a JIRA > comment and the author could commit directly. No signoff required. > > Thanks > Joe > > On Thu, Mar 2, 2017 at 10:42 AM, Joe Skora <jsk...@gmail.com> wrote: > > Like Andre, I originally got the requirement for signoff from the > > Contributor Guide[1] when I started working on the project and later from > > this email thread[2]. If this not the expected process, we should > > definitely update the Contributor Guide. > > > > From the Apache perspective the signoff confirms that the contribution > was > > reviewed for Apache compliance, but that's incumbent on anyone committing > > to the repository. Since the committer identity is available from the > > repository the signoff seems redundant. > > > > [1]https://cwiki.apache.org/confluence/display/NIFI/Contributor+Guide# > > ContributorGuide-Stepstomerge/closepullrequestswithtwomainbranches > > [2] > > https://lists.apache.org/thread.html/1ce165a4172f67ce08683d3eb1c825 > 3319a97dadd0ccc3fbc598f639@1446565288@%3Cdev.nifi.apache.org%3E > > > > > > On Thu, Mar 2, 2017 at 9:33 AM, Joe Witt <joe.w...@gmail.com> wrote: > > > >> For what it is worth this is definitely not a requirement and not > >> something I knew anything of so I never do it. > >> > >> I think it is a perfectly fine idea and a good practice to follow so > >> occasional reminders of its utility are fair game. That said, to > >> Bryan's point I rely on the JIRA/issues history if i need to know who > >> did a given review. So we have a couple of options. > >> > >> But we should probably stop short of calling this a requirement. In > >> an apache sense it is not. > >> > >> Thanks > >> Joe > >> > >> On Thu, Mar 2, 2017 at 8:51 AM, Matt Burgess <mattyb...@apache.org> > wrote: > >> > I didn't realize it was required either, I usually only sign off > >> > (using the same thing Bryan Bende does) if the PR author couldn't > >> > merge it on their own (i.e. not a NiFi committer/PMC). Certainly I can > >> > start always signing off commits. > >> > > >> > Regards, > >> > Matt > >> > > >> > On Thu, Mar 2, 2017 at 8:35 AM, Oleg Zhurakousky > >> > <ozhurakou...@hortonworks.com> wrote: > >> >> Thanks Bryan. > >> >> > >> >> If ‘-s’ is only for showcasing the committer I don’t believe anyone > >> would have any issues with it, but my concern at the moment is purely > >> legal, so I am not sure who is the right person to answer that, but > figured > >> raising the concern is the least I can do. > >> >> > >> >> Cheers > >> >> Oleg > >> >> > >> >> > >> >>> On Mar 2, 2017, at 8:16 AM, Bryan Bende <bbe...@gmail.com> wrote: > >> >>> > >> >>> The sign-off is so we can easily see who the reviewer/merger was > from > >> >>> the git history. > >> >>> > >> >>> We can always go back to the JIRA or PR and the reviewer/merger > should > >> >>> have commented there, but its convenient to see it in the git > history > >> >>> in my opinion. > >> >>> > >> >>> Personally, whenever merging someones contribution I use "git am > >> >>> --signoff < patchfile" which I guess is equivalent to doing the > ammend > >> >>> after applying the patch. > >> >>> > >> >>> > >> >>> On Thu, Mar 2, 2017 at 8:05 AM, Oleg Zhurakousky > >> >>> <ozhurakou...@hortonworks.com> wrote: > >> >>>> Andre > >> >>>> > >> >>>> Thanks for the reminder. I admit that I did not know that we > require > >> it in the Contributor Guide, so thanks for pointing it out. > >> >>>> However, your email did prompt me to look at the purpose and origin > >> of the ‘-s’ flag and led me to this thread on Stack Overflow - > >> http://stackoverflow.com/questions/1962094/what-is-the- > >> sign-off-feature-in-git-for. > >> >>>> > >> >>>> And I am now wondering if we should require it or even use it in > the > >> first place, since it’s origin, history and purpose appears to have more > >> “individual” legal implications then showcasing the actual committer. > >> >>>> > >> >>>> Thoughts? > >> >>>> > >> >>>> Cheers > >> >>>> Oleg > >> >>>> > >> >>>> On Mar 2, 2017, at 6:35 AM, Andre <andre-li...@fucs.org<mailto:a > >> ndre-li...@fucs.org>> wrote: > >> >>>> > >> >>>> dev, > >> >>>> > >> >>>> May I remind you to ensure we follow the Contributor Guide and use: > >> >>>> > >> >>>> git commit --amend -s > >> >>>> > >> >>>> when merging commits from your peers? > >> >>>> > >> >>>> While git pretty-format can be used to reveal the committer, I am > >> sure that > >> >>>> all of us will agree that as an inclusive community we value both > the > >> >>>> pretty and ugly formats... > >> >>>> > >> >>>> So can we give the ugly format the support it deserves and ensure > we > >> add > >> >>>> the neat Signed-off-by stamp to the commit message? > >> >>>> > >> >>>> Cheers > >> >>>> > >> >>> > >> >> > >> >
