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

Consider putting more emphasis on readable git history; use squash strategy when merging single-unit PRs. #151

Closed
dmitshur opened this Issue Oct 2, 2017 · 4 comments

Comments

Projects
None yet
2 participants
@dmitshur
Member

dmitshur commented Oct 2, 2017

This is simply a suggestion from a user and contributor to this (excellent) Go project about a way I think it can be made better.

I was recently updating to the latest version and looking over the history to see what changed. On master branch, I saw commit d593fc4, a revert commit, which did not describe the motivation for reverting a change (which is a bad practice, because it makes it really hard to understand why something was reverted for all other people). Upon further investigation, I saw that commit was actually a part of PR #147, so nothing was actually reverted on master, it was just an internal detail of that PR.

In general, the git history of master branch has a low signal to noise ratio because of many commits with unhelpful subjects and commit message bodies. E.g.:

untitled

It seems that most PRs here are made of commits that are just addressing internal PR feedback, rather than broken up into clean, logical and well documented separate commits to make the overall diff easier to understand. Yet they're merged via the "merge commit" strategy, which preserves individual PR commits with their original commit messages, resulting in the problem described above.

I suggest merging such PRs via the "squash merge" strategy, with a descriptive commit message that actually describes what changed on master and why. GitHub makes it easy ever since they've added this feature:

image

image

For examples of what it can look like to have more readable git history, see:

Thanks for consideration!

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 2, 2017

Member

For reference, I was using GPS to see the updates, which exacerbates the situation. Compare:

image

To something much more readable, like:

image

Member

dmitshur commented Oct 2, 2017

For reference, I was using GPS to see the updates, which exacerbates the situation. Compare:

image

To something much more readable, like:

image

@slimsag

This comment has been minimized.

Show comment
Hide comment
@slimsag

slimsag Oct 8, 2017

Member

Thanks for creating this @shurcooL -- I appreciate your attention to detail (especially since this more relates to how someone less involved in Vecty development might see / perceive Vecty).

Deciding the right strategy here is hard because sometimes changes / PRs do need to actually make two distinct changes from the perspective of the master branch. For example, if adding a new public function requires modifying an existing one. For these situations, what I will do from now on is write this detail into the squash merge commit message. I don't want to require contributors necessarily to create two distinct PRs in this case, since I wish to place more emphasis on code quality rather than git history.

But, to hopefully improve this issue as a whole, I've enabled GitHub's restrictions on merge/rebase commits, so that only squash merge is possible for me now:

image

We'll see if that helps improve our commit logicality overall. If it doesn't, we can try a different strategy :)

Closing for now.

Member

slimsag commented Oct 8, 2017

Thanks for creating this @shurcooL -- I appreciate your attention to detail (especially since this more relates to how someone less involved in Vecty development might see / perceive Vecty).

Deciding the right strategy here is hard because sometimes changes / PRs do need to actually make two distinct changes from the perspective of the master branch. For example, if adding a new public function requires modifying an existing one. For these situations, what I will do from now on is write this detail into the squash merge commit message. I don't want to require contributors necessarily to create two distinct PRs in this case, since I wish to place more emphasis on code quality rather than git history.

But, to hopefully improve this issue as a whole, I've enabled GitHub's restrictions on merge/rebase commits, so that only squash merge is possible for me now:

image

We'll see if that helps improve our commit logicality overall. If it doesn't, we can try a different strategy :)

Closing for now.

@slimsag slimsag closed this Oct 8, 2017

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 10, 2017

Member

Great, thanks a lot! I'm sure this will help significantly. Just make sure to review and edit the commit message title and body when merging PRs, so they contain only the information relevant to the final overall diff. You get a chance to easily modify them during the process of merging (this is one of the biggest benefits of this feature).

Deciding the right strategy here is hard because sometimes changes / PRs do need to actually make two distinct changes from the perspective of the master branch.

In those cases, which as you mention are more rare, you could use rebase merge strategy, but it requires each commit to have a clean commit message. It won't give you a chance to edit them when merging.

Member

dmitshur commented Oct 10, 2017

Great, thanks a lot! I'm sure this will help significantly. Just make sure to review and edit the commit message title and body when merging PRs, so they contain only the information relevant to the final overall diff. You get a chance to easily modify them during the process of merging (this is one of the biggest benefits of this feature).

Deciding the right strategy here is hard because sometimes changes / PRs do need to actually make two distinct changes from the perspective of the master branch.

In those cases, which as you mention are more rare, you could use rebase merge strategy, but it requires each commit to have a clean commit message. It won't give you a chance to edit them when merging.

@slimsag

This comment has been minimized.

Show comment
Hide comment
@slimsag

slimsag Oct 10, 2017

Member

Will do! Good points all around :)

Member

slimsag commented Oct 10, 2017

Will do! Good points all around :)

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