Correct fixing and commit squashing workflow in a pull request

[davereikher]

I'm currently working on a new issue and have an open pull request.
I'm pretty sure (please correct me if I'm wrong here) that before a pull request is merged the commits must be squashed. What I'm not sure is when to squash and in either case I can think of there is a problem.
Let's say someone reviewed my code and I fixed it based on their review. Should I:
1. Commit, squash commits and force push, which would require the reviewer, if they want to review my fix, to go over everything again - they can't make a diff with the previous version before the fix (..or can they?)
2. Fix and push without squashing so that the reviewer could easily review the fix, wait for their ACK and then squash commits. However, if I understand correctly, the person responsible to make the merge will look for several ACKs on the same commit and then they should merge that commit into master.  However, if I'm the one squashing the commits after all the ACKs, wouldn't it change the hash of the last commit which was ACKed by everyone and thus, ideally it needs to be reviewed again so that, for example, a malicious "contributor" wouldn't insert some malicious line of code along with the squash?

So option (1) is a wasteful on developer's time while option (2) might be used as a vector to introduce malicious code changes. What's the solution?

[1715534877]

1715534877

[1715534877]

1715534877

[1715534877]

1715534877

[1715534877]

1715534877

[1715534877]

1715534877

[achow101]

Most people (including myself) do option 1. Github actually now does have an easy way to view what changed between multiple commits that were force pushed. Just look at any current PR. You will see something like

achow101 force-pushed the achow101:descriptor-errors branch from a1b4795 to 787c9ec 17 hours ago

in the PR and you can click on it to show you the diff between the two commits. This means that reviewers don't actually have to do that much more work.

Reviewers can also use the git range-diff command to get a diff between the two force pushes.

Additionally, when you squash and force push, when someone else comes to review your PR for the first time, they won't waste time looking at things that have already been fixed by a fixup commit that will be squashed later.

So overall, it is better to squash and force push as it does actually waste less time and it keeps the commits clean.

[davereikher]

Thanks! That cleared things up   

So, just to make sure I understand correctly - if an ACK was given on a certain commit by a reviewer and I force push, that would basically invalidate that ACK and would require that the reviewer reviews the change again, only this time it would be easier for them because they could use git range-diff or the Github UI to see the diff between the commit they already reviewed and the force pushed one, right?

[achow101]

Thanks! That cleared things up   

So, just to make sure I understand correctly - if an ACK was given on a certain commit by a reviewer and I force push, that would basically invalidate that ACK and would require that the reviewer reviews the change again, only this time it would be easier for them because they could use git range-diff or the Github UI to see the diff between the commit they already reviewed and the force pushed one, right?

Yes.