Git commits history conventions in PR

[AlonZivony]

I want to create a consensus about the conventions of how should the commit log history look like when opening a PR in the project.

We have a consensus that all PR commits should be squashed to 1-3 commits (according to PR size) before merging the commit to main.
However, during the CR cycle it is a different issue.

There are two major options:

1. Pushing all commits as is to the remote
2. Squashing the commits before pushing them

I am opening this discussion to continue the discussion from #1103

[AlonZivony]

I will write here my opinion again:
I think that all the commits used locally should be pushed to the remote until the PR is approved.

From git best practices, one should commit often and regularly to be able to maintain his code better.
Commits help to revert specific changes more easily and locating bugs more easily. They are also a documentations of all the changes occurred during the branch life cycle.
If we squash the commits before pushing them, this vessel will not be available to use when receiving CR comments (for both commiter and reviewer).

One thing that squashed commits will help in is during the CR process to have better understanding of the code written and the connections between files and packages, but I think that using good division to packages and files it will be clear as well.

A suggestion was to create a local branch with all the commits and squash them to the remote branch. This way you can preserve your commit messages but pushing a clean history.
However, this option is very hard to maintain because each branch would have to be maintained separately, rebased separately, etc.

So the solutions in my opinion are the following:

1. Pushing all the commits as-is
2. squashing them before merge in one of the following ways:
   • Use squash-merge button when merging (Github in my opinion can be configured to only allow merge this way).
   • Requiring the developer to squash the commits to gain the final approval for the PR.

Tell me what you think :)

[rafaeldtinoco]

Currently 'draft' means that the PR is not ready for merge and still need to be edited before check of maintainer.

That is the case, no ? I'm afraid of them being committed. But I thought about your point as well. How to be draft and, still, call for review. That is why I suggested calling for reviewers.

If we use it to only say that the commits are not ready for merge, won't it be hard for the maintainers to understand what they need to check?

Could be, maybe we can have a tag to tell reviewers it is ready for review but needs squashing.

With this idea I'm just trying to "bridge" the 2 ideas (of not doing it, or doing it in an acceptable way for all others). I already bought the idea, but I'm not the only one reviewing (thus the draft + review suggestion).

I'll let others say what they prefer/think now.

[yanivagman]

I will write here my opinion again: I think that all the commits used locally should be pushed to the remote until the PR is approved.

From git best practices, one should commit often and regularly to be able to maintain his code better. Commits help to revert specific changes more easily and locating bugs more easily. They are also a documentations of all the changes occurred during the branch life cycle. If we squash the commits before pushing them, this vessel will not be available to use when receiving CR comments (for both commiter and reviewer).

I agree that it is a best practice to commit often, but not every piece of code should become a commit. Commits should be done only when a logical component is completed. In particular, it makes the life of a reviewer harder to review one commit that adds new code that is then removed in a different commit of the same PR (doubles the effort). In addition, splitting one logical component into several commits should also be forbidden, as you shouldn't commit half baked work. As long as this concept is kept (one commit per logical component), I agree with what you wrote.

So the solutions in my opinion are the following:

1. Pushing all the commits as-is

2. squashing them before merge in one of the following ways:

   • Use squash-merge button when merging (Github in my opinion can be configured to only allow merge this way).
   • Requiring the developer to squash the commits to gain the final approval for the PR.

Tell me what you think :)

Push all the commits as long as the concept of one commit per logical component is kept, and as long as you don't add code in one commit, which is then removed in another commit. If this concept is kept, there will be no need to squash the PR anyway.

[yanivagman]

2. squashing them before merge in one of the following ways:

   • Use squash-merge button when merging (Github in my opinion can be configured to only allow merge this way).
   • Requiring the developer to squash the commits to gain the final approval for the PR.

Please, let's not use the squash-merge button. It does not guarantee that that git log message will have correct wraps or format. In the past we've had git commit logs with very very long unwrapped sentences.

Using the squash button also allows you to edit the commit message and remove the comments that are not relevant.

IMO, if others would like to follow this approach, which is okay for me after you explained your needs, the +1 should be verbal, together with a request "now that code is good, let's rebase this with final commits.

The developer would squash (and create even more than 1 commit if developer wants) and prepare for the final merge.

My opinion is also that the PR should be kept as draft if its not squashed in the way it should be merged. To avoid accidental merges (that already happened).

So, IMO, follow this, the final flow could be:

1. developer opens a draft PR and selects maintainers for reviews.
2. maintainer assigns PR to himself and reviews PR.
3. after a final +1, maintainer asks developer to squash and remove draft.
4. final commits are checked, rebase-merged is done.

We could have used a "ready for review" tag in addition to marking the PR as a draft, but this will make things more complicated. This will require to set every PR as a draft first, even when not necessary, and creates a "protocol" between a contributor and a reviewer that is composed of several steps (while there are PRs that are ready since the time they are opened, and can be merged without having this contributor-reviewer ping pong).

My suggestion is the following:

1. Open a PR when you want to publish your code, and have a feedback from other people. You might want to mark it as a draft if it is not ready to be merged,
2. You might want to add specific reviewers if you see it right, but otherwise, a contributor doesn't have to know which reviewers he should choose. If no reviewers are selected - the maintainers can choose to review it on their own, or assign a person that they see fit.
3. A PR which has many small commits where not every commit is for one logical component should be marked as a draft.
4. A developer can ask a reviewer to review his code while still a draft. The reviewer might then give a "verbal +1" and ask the developer to make modifications to the commits in his PR before removing the draft mark

[itaysk]

• open a PR when the code is ready to be shared
• if the code isn't ready for review, make it draft. otherwise (and by default), an open PR means - please review and merge my code
• if the reviewer (or submitter) thinks the commit log is not merge-worthy, then say so - I think a label is good, I've seen needs-rebase used around.
• if you want to block the merge, then generally don't approve it, but I suppose we could do a custom solution that enforce if a certain label is present the merge will be blocked even if approved.

[AlonZivony]

if you want to block the merge, then generally don't approve it, but I suppose we could do a custom solution that enforce if a certain label is present the merge will be blocked even if approved.

This sounds like a very good solution to me.

Commits should be done only when a logical component is completed

@yanivagman I agree with you on that (and I myself need to get better at it), but a logical component can be a function or a UT. This is important because when searching for a bug I want to find the exact code it occurred in, and if I want to revert I probably want to revert only the relevant part. This means that normal feature which implements a new idea will still have more than 5 commits, and large PR about 30. A finished file shouldn't be committed at once because it doesn't add one single logical component.
From the standard of this project, I think this is more commits than what we want to have in main for a single feature.

and as long as you don't add code in one commit, which is then removed in another commit.

I think this is a weird request, because I might regret deleting a part of my code during the CR cycle (I had 2 implementations to some function and I chose 1, but later regretted that choice) and want to undo the deletion. When merging there is no reason to save the deletion, but during the development it is crucial in my opinion. That is why I think there are commits that should be removed only when all the code passed a review in my opinion.

In particular, it makes the life of a reviewer harder to review one commit that adds new code that is then removed in a different commit of the same PR

So I am not sure if reviewing by commits is the best practice. Commits are the way for the solution, and it doesn't have to be a straight road. You have done more CRs than me, but don't you think it would be better to just check the final code instead of each commit?

[yanivagman]

So I am not sure if reviewing by commits is the best practice. Commits are the way for the solution, and it doesn't have to be a straight road. You have done more CRs than me, but don't you think it would be better to just check the final code instead of each commit?

If the commits are crafted well and according to the concept I mentioned before, it can be much easier to review by commits (especially in big PRs)

[AlonZivony]

I agree that by crafting commits by logic it will make the CR easier, but I also think (again, you can argue with it) that commits are not tools for CR but for developers.
I think a reviewer can use the commits for the CR using git-blame, which is a nice method to understand code, but I am not sure that CR is what the developer need to have in mind when commiting his work.

[yanivagman]

but I am not sure that CR is what the developer need to have in mind when commiting his work

Commit whatever you want to make your development process easy, I won't tell you how to develop your own local branch.
But when you push your code and open a PR, the above guidelines should be followed, as you want to make the review process easy, and have your PR merged as soon as possible.

This is why I use the following method, as previously described in your PR as a comment:

1. Developing my own branch with many small commits
2. When I'm satisfied with the code, I checkout a new branch from the original development branch
3. I then craft commits that can be easily digestable by a reviewer and which make sense. Each logical part that is self contained gets its own commit.
4. When I get review comments, I'm adding new commits on top of the new branch.
5. I then use "rebase -i" to insert the changes to the relevant commits. Of course, before doing "rebase -i" you can open a new branch, and repeat the process described above.

[rafaeldtinoco]

That is usually how upstream open-source projects work =), and what I do as well. We're deciding here if we will accept a deviation from this pattern because of personal preferences or not.

If we chose to accept this deviation, then my suggestion is that pr IS A DRAFT and marked as "ready to review" with a tag. After a draft review and +1, it would have to be squashed and prepared as a real PR.

I will continue to do PRs ready to be merged, but if others would like to work like Alon is suggested, let's, at least, keep the PRs as drafts (not to encourage others to do the same, as it is not the way to go in open-source projects).