We have a Git Hook script included in Druid repo ( https://github.com/apache/druid/blob/master/dev/intellij-setup.md#git-checkstyle-verification-hook-optional ). Maybe this is something we can add to the Git hook (to enforce / prevent force push) and encourage more people to install the hook. This way, you only have to take action once (to install the hook) instead of remembering everytime you push your commits.
Best Regards, Maytas On Fri, Jan 15, 2021 at 2:19 PM Jihoon Son <ghoon...@gmail.com> wrote: > @Clint thanks, I didn't know Github supports that now. I think it will make > force pushes a bit better only when Github can restore the commit history > somehow, such as when there are only new commits added without overwriting > or deleting any previous commits. > > @Himanshu, thanks. That makes sense. Regarding forbidding force push for > the master branch, I do agree that is the case we must not do force pushes. > I can create an infra ticket for that, but I'm unsure how much gain we can > get since we never push to the master directly. > I would rather wait until we meet some cases where we have to update the > master directly. > > I opened a PR for promoting the contributing doc and discouraging people > from using force pushes (https://github.com/apache/druid/pull/10769). > > Thanks, > Jihoon > > On Fri, Jan 15, 2021 at 2:06 PM Himanshu <g.himan...@gmail.com> wrote: > > > +1 for discouraging force push in the PRs since there is no way to > enforce > > it. > > > > > clean commit history is not a big gain compared to how much it can make > > the review process worse, especially when the PR is big > > "commit history" of PR is destroyed anyways when we do "Squash and Merge" > > ... commit message in druid master is based on PR "title" ..... so > > maintaining nice/clean looking commit history in PR isn't important. > > > > We MUST certainly not do a force push in apache/druid:master, that > > could/should be enforced I think . Force push to release branches are > > tolerable in extreme circumstances like Clint described. > > > > I hope that makes sense. > > > > -- Himanshu > > > > > > > > > > On Fri, Jan 15, 2021 at 1:47 PM Clint Wylie <cwy...@apache.org> wrote: > > > > > It seems like this will basically only affect release managers. > > > > > > I am maybe -1 since I have personally had to force push to a release > > branch > > > while making an RC, when I optimistically pushed the tags and then > found > > a > > > mistake doing preflight checks before sending the artifacts out to > vote. > > I > > > did this so that I didn't have to do something like jump from RC1 to > RC3 > > > with a dead RC2 before it was even voted on. > > > > > > I find since github added > > > https://github.blog/changelog/2018-11-15-force-push-timeline-event/ > that > > > force-pushes aren't that terrible to deal with during a review even, so > > > would probably personally be in favor of relaxing our soft policy on > > them, > > > but it seems like everyone else is opposed to them, so i think it is > also > > > fine to keep the soft policy as is and adding the link to it to the PR > > > template. > > > > > > On Fri, Jan 15, 2021 at 1:26 PM Gian Merlino <g...@apache.org> wrote: > > > > > > > Will this help for the (common) case where PR branches are in > people's > > > > forks? > > > > > > > > On Fri, Jan 15, 2021 at 1:00 PM Jihoon Son <jihoon...@apache.org> > > wrote: > > > > > > > > > Hi all, > > > > > > > > > > The forced git push is usually used to make the commit history > clean, > > > > which > > > > > I understand its importance. However, one of its downsides is, > > because > > > it > > > > > overwrites the commit history, we cannot tell the exact change > > between > > > > > commits while reviewing a PR. This increases the burden for > reviewers > > > > > because they have to go through the entire PR again after a forced > > > push. > > > > > For the same reason, we are suggesting to not use it in our > > > > documentation ( > > > > > > > > > > > > > > > > > > > > https://github.com/apache/druid/blob/master/CONTRIBUTING.md#if-your-pull-request-shows-conflicts-with-master > > > > > ), > > > > > but I don't believe this documentation is well read by many people > > (It > > > > is a > > > > > good doc, BTW. Maybe we should promote it more effectively). > > > > > > > > > > Since branch sharing doesn't usually happen for us (AFAIK, there > has > > > been > > > > > no branch sharing so far), I think this is the biggest downside of > > > using > > > > > forced push. To me, clean commit history is not a big gain compared > > to > > > > how > > > > > much it can make the review process worse, especially when the PR > is > > > big. > > > > > > > > > > So, I would like to suggest forbidding git forced push for the > Druid > > > > > repository. It seems possible to disable it by creating an infra > > > ticket ( > > > > > > > > > > > > > > > > > > > > https://issues.apache.org/jira/browse/INFRA-13613?jql=text%20~%20%22force%20push%22 > > > > > ). > > > > > I can do it if everyone agrees. > > > > > > > > > > Would like to hear what people think. > > > > > Jihoon > > > > > > > > > > > > > > >
