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