+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 > > > > > >
