processing / p5.js

p5.js is a client-side JS platform that empowers artists, designers, students, and anyone to learn to code and express themselves creatively on the web. It is based on the core principles of Processing. http://twitter.com/p5xjs —
http://p5js.org/
GNU Lesser General Public License v2.1
21.2k stars 3.23k forks source link

Force Git Squash and Git Rebase before every PR being merged #6802

Open Ayush23Dash opened 5 months ago

Ayush23Dash commented 5 months ago

Improve Git commit History

Issue

As of now, the entire p5.js repository does not follow squashing down all of the commits of a PR to one commit and then rebasing the branch with main branch before merging a PR.

This creates a messed up commit history. Squashing down commits improves readability throughout the git log.

Steps to squash Down Commits to one commit

Note - pick or p will only use those commits, but squash or s will use them and combine them all together.

Steps to rebase the PR branch with main branch

limzykenneth commented 5 months ago

I'm not sure what the benefit of doing this would be and I feel like squashing commits erases history and context to a merge, not to mention the PR author's authorship. If a particular issue stems from the middle of a PR, we have no easy way of tracking it down.

In addition, having to go through the repo locally for every PR to do the squash and rebase is a bit too much and isn't practical in my opinion.

davepagurek commented 5 months ago

In addition, having to go through the repo locally for every PR to do the squash and rebase is a bit too much and isn't practical in my opinion.

I think there's a squash and merge option in GitHub's UI that we could enable if we want: https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/about-merge-methods-on-github#squashing-your-merge-commits

I'm not sure what the benefit of doing this would be and I feel like squashing commits erases history and context to a merge, not to mention the PR author's authorship. If a particular issue stems from the middle of a PR, we have no easy way of tracking it down.

For me the benefit would be easier browsing of the history of a file: individual commits within a PR are a lot less likely to have useful context for me personally rather than info about the PR in general. If I want to check the context of a line of code, I usually go to the commit, and then click through to the PR it came from. This is still possible without squash & merge, just more cumbersome. I don't have a strong opinion on this one, I'd be ok either way.

limzykenneth commented 5 months ago

If I want to check the context of a line of code, I usually go to the commit, and then click through to the PR it came from.

Yeah I get that and I do that as well but to give a bit of context in terms of what the full commit history can be useful for, what I often use when there is a regression or a bug introduced some time in the past but I'm not sure where and when, I will do a git bisect from the last known good release. With git bisect I can very quickly narrow down onto the exact commit where incorrect behavior starts (and thus introduced).

From that commit I got from git bisect I can see exactly what changed and quickly understand what the possible cause of a problem is. However this only works if commits are relatively small. If we do squash commit, almost any commit I get from git bisect will contain possibly many changes as if all changes are done in one massive commit, then my only option will be to go to the original PR then inspect line by line or commit by commit. So that's kinda what I need the full history and context for.

Ayush23Dash commented 5 months ago

Got it as why you are considering not squashing down commits. But my another topic of concern would be atleast rebasing the commits on top of the already existing commit history from the incoming changes branch, because this would help keeping the commit history in a linear fashion and easy to read. This won't even hamper when git bisect would be used to move to the exact commit as and when required. Uploading just a small pictorial representation of how rebasing might be helpful :

Screenshot 2024-02-25 at 9 49 13 AM
limzykenneth commented 5 months ago

I am more open to considering a rebase merge but there are some downsides to rebase merge as well (at least GitHub's version of it, we won't be using local rebase and merge as mentioned before).

GitHub rebase and merge takes individual commits from the head branch and apply them onto the base branch as new commits, this means that the commit hash in the original branch and in the PR itself will be different from what is merged into the base branch. Once we identify a problem commit in the merged base branch, its hash does not existing on the original branch.

The lack of a merge commit also makes it a bit more difficult to identify the original PR from the commit, ie. we no longer have a commit that tells us "Merge pull request #__ from ___", I'm not sure if GitHub keep track of this information separately itself but it won't be part of the git history of the repo.

Ayush23Dash commented 5 months ago

Hmm, I am getting your point of hesitation for implementation of github's rebase merge. I am attaching a screenshot of my recent rebase merge(this one is a local rebase merge not github's version of it)

Here, you can see that the commit itself and the commit of merging the PR have seperate commit hashes. So, let's say if we encounter a specific problem in the future after meging a PR, we actually do have the whole record of all of the PRs and their respective commits with seperate commit hashes in the commit history.

So in my opinion tracking down of some buggy commit in the commit history won't be difficult, however I would respect your decision of not implementing it if you consider that this might create an issue in the future for bisecting commits.

Screenshot 2024-02-29 at 1 21 02 PM