Re: Make the github PR commits and File changed clean

[Tanner Clary]

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  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  于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  于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 
> 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 
> >> 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 
> >> 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,

Re: Make the github PR commits and File changed clean

[Ran Tao]

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

Re: Make the github PR commits and File changed clean

[Julian Hyde]

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

Re: Make the github PR commits and File changed clean

[LakeShen]

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

Re: Make the github PR commits and File changed clean

[Alessandro Solimando]

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

Re: Make the github PR commits and File changed clean

[Julian Hyde]

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

Re: Make the github PR commits and File changed clean

[Julian Hyde]

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

Make the github PR commits and File changed clean

[LakeShen]

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