Hi all, The sudden rise in commits for each PR is due to the mistake I made yesterday when trying to merge a PR. I think I got the main branch as it should be, but the one nasty side effect was that all pre-existing pull requests had many additional commits that had already been merged. In terms of the larger discussion, I also prefer when the contributor leaves separate commits and only rebasing/squashing when requested. Sorry for any headaches this may have caused, I will be more careful in the future.
Thanks, Tanner On Wed, Sep 6, 2023 at 7:17 PM Ran Tao <[email protected]> wrote: > Thanks Shen. > > got your point. Also encountered this problem. > > It seems the all PRs created before yesterday will have a lot of commit > history in GitHub, it's so misleading. > Yes, the author may need to rebase it. So it will be clear like before. > Julian and Alessandro may not understand what you really trying to saying. > > BTW, your picture was broken. I think your screenshot might be useful. > > Best Regards, > Ran Tao > > > LakeShen <[email protected]> 于2023年9月7日周四 09:54写道： > > > 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. > > > > [image: 20230907-095212.jpeg] > > > > Best, > > LakeShen > > > > Alessandro Solimando <[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]> > 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]> > >> 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]> > >> 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 > >> > > >> > > >
