Ah, I see, you’re talking about PRs like https://github.com/apache/calcite/pull/3375 and https://github.com/apache/calcite/pull/3393, which have 65 and 84 commits respectively but should only have one or two.
Wow, I had no idea that force-pushes made such a mess. Yes, I agree, it makes sense for you, as reviewer, to ask the PR author to rebase in that situation. Julian > On Sep 6, 2023, at 6:53 PM, LakeShen <[email protected]> wrote: > > Thanks to Julian and Alessandro for your reply.I agree with your point,maybe > I don't have a clear description of my thought. > > Because we have a forced update of the main branch before, now almost every > PR on github has hundreds of File changes and dozens of commits, so it is a > bit difficult to review. > > What I'm trying to say is that the author of the PR could rebase the latest > code in the main branch, recommit, and the records will be back to normal. > > > > Best, > LakeShen > > Alessandro Solimando <[email protected] > <mailto:[email protected]>> 于2023年9月7日周四 00:41写道： >> Hello everyone, >> I share Julian's opinion about this, and I have seen other reviewers making >> the same request (hold off from squashing while the review is in progress), >> but never the opposite (please squash as there are many commits), but my >> experience might just be anecdotal. >> >> When I author a PR (not only here), I try to separate my contribution into >> small and purposeful commits, so that people can clearly skip eventual >> refactoring commits, clearly see code modifications and tests >> changes/additions. >> >> I also sometimes find the commit messages coming from the code changes to >> become the extended commit message for my squashed commit, for some >> non-trivial changes. >> >> IMO it's a way to guide the reviewer through your PR, more informative than >> a single commit with all changes at the same time. >> >> As a reviewer, I like when a PR is crafted this way, as it allows easily to >> split the review process into different time frames, following the commits. >> Of course sometimes I still need to go back and forth between them (like >> checking the code change when looking at the tests), but it's better than >> just going file by file and marking them as "reviewed" in the GitHub UI. >> >> As Julian says, there are different styles and it's always a trade-off >> between multiple factors. >> >> Best regards, >> Alessandro >> >> >> On Wed, 6 Sept 2023 at 17:50, Julian Hyde <[email protected] >> <mailto:[email protected]>> wrote: >> >> > I should have said that the current policy isn’t absolute. I agree that a >> > squash/rebase can be beneficial if there are many commits. It’s a trade >> > off, and the reviewer should make the call, not the author of the PR. >> > >> > Julian >> > >> > > On Sep 6, 2023, at 8:45 AM, Julian Hyde <[email protected] >> > > <mailto:[email protected]>> wrote: >> > > >> > > Personally, I don’t have a problem reviewing PRs that have many >> > commits. The GitHub web UI and command-like tools like *git diff main” make >> > it easy to see the whole change. >> > > >> > > Conversely, if I have reviewed a PR and requested changes, I would much >> > rather that the author makes those changes in an additional commit or >> > commits. I can easily see what they did to address my concerns, and I don’t >> > need to re-read the changes I already reviewed. >> > > >> > > Furthermore, if an earlier review had comments against specific lines, >> > GitHub has trouble keeping those comments in place when there is a >> > force-push. >> > > >> > > So, I ask authors to not force-push or rebase unless a reviewer >> > explicitly asks them to. I have heard other committers make similar >> > requests, so I would say this is the current consensus policy in Calcite. >> > (Of course, reconsidering that policy, as we are doing in this discussion, >> > is just fine.) >> > > >> > > Julian >> > > >> > >> On Sep 6, 2023, at 8:05 AM, LakeShen <[email protected] >> > >> <mailto:[email protected]>> wrote: >> > >> >> > >> Hi devs, >> > >> >> > >> I found that each PR has a lot of commits and file changed, which could >> > >> make it difficult to review. One approach is for the author of the PR to >> > >> rebase the latest code in the main branch and force push again. Best, >> > >> LakeShen >> >
