Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Ayush23Dash opened this issue Feb 11, 2024 · 6 comments
Open

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

Ayush23Dash opened this issue Feb 11, 2024 · 6 comments

Comments

@Ayush23Dash
Copy link
Contributor

Ayush23Dash commented Feb 11, 2024

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

  • To squash down 'n' number of commits:

    git rebase -i HEAD~n

  • This will open up your editor of choice for Git. The default is Vim.

  • Now, you need to replace all those pick with squash (or simply s) apart from the first one.

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

  • The first commit is the one you will combine them into without losing your changes.
  • After doing that, save the file and close it. Git will open up another editor where you can see the new commit message it generates for you.
  • You can get rid of all of them and add your custom message.
  • After doing that, you should see a success message in the terminal.

Steps to rebase the PR branch with main branch

  • Checkout to main and git pull upstream main
  • Checkout to PR branch and git rebase upstream/main
  • There are chances git conflicts will come in your changed files, remove them manually and carefully.
  • Add the unstaged changes to git staging area : git add .
  • git commit -m “Original commit message that you used earlier”
  • git rebase --continue
  • To push the changes to the online PR : git push -f origin [branch name]
@limzykenneth
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants