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

Restore functionality from sync-pr-commit-title removed in #2738 #2768

Closed
anton-bot opened this issue Feb 10, 2020 · 16 comments
Closed

Restore functionality from sync-pr-commit-title removed in #2738 #2768

anton-bot opened this issue Feb 10, 2020 · 16 comments

Comments

@anton-bot
Copy link

My merge commits are no longer renamed automatically to sync with the PR title. Only squash commits are being renamed.

Perhaps can this functionality be restored as a separate feature, to rename merge commits, not only the squash commits?

This was always there, but I believe it was removed via #2738.

@fregante
Copy link
Member

I removed it because merge commits are indistinguishable, so in a list it looks like that commit added a feature and then the following commits did the same. If the merge commit title is the default (Merge X into Y) then it’s more clear

I think it you want to keep each commit in history it’s best to rebase them.

Can you show me an example of a merge commit you made?

@anton-bot
Copy link
Author

anton-bot commented Feb 10, 2020

Well my commits previously looked like this:

fix: typo on main page (#9123) <-- this is the merge commit
fix: typo on main page

I feel that the whole Merge pull request #9123 into develop is too verbose and distracting, so if the merge commit message is the same as commit message, then it makes it easier to skim through, and the rebases are not allowed in my repo.

But perhaps you are right it may not make much sense to other people.

@fregante
Copy link
Member

fregante commented Feb 10, 2020

I personally think merge commits are just noise in most cases, so I may not be the right person to talk about them. In that example I just don’t see it being an advantage over the current situation.

I’d perhaps suggest we restore an old feature that made merge commits distinguishable (and if I remember correctly also dimmed them). We dropped it because there’s no “merge commit class” in the DOM anymore, so we could instead detect them if their title starts with Merge and contains into

@refined-github refined-github deleted a comment from keelerak14 Feb 10, 2020
@bastienboutonnet
Copy link

bastienboutonnet commented Feb 15, 2020

I personally really liked the merge renaming when it was done like that: fix: typo on main page (#9123). And my entire team did too. Would be ok with is being Merge fix: typo on main page (#9123) to distinguish them from squashed merges too.

I do agree with @fregante that merge commits are kinda noisy, but not every org or team is able to have squash merge as a default.

Or maybe users should be given the option to choose if they care about distinguishing? Although I guess that defies the point of an opinionated git extension. I just thing this opinion may be dismissed too quickly.

@anton-bot
Copy link
Author

@bastienboutonnet if you name it as Merge fix: typo..., this breaks the semantic commit naming rules

https://www.conventionalcommits.org/en/v1.0.0/

@bastienboutonnet
Copy link

bastienboutonnet commented Feb 19, 2020

I was not aware you were sticking to those rules. Just so that I can understand. What point does it break actually? I'm trying to see how the default merge commit message from github does not violate those conventions.

@fregante what was the old feature like:

I’d perhaps suggest we restore an old feature that made merge commits distinguishable (and if I remember correctly also dimmed them). We dropped it because there’s no “merge commit class” in the DOM anymore, so we could instead detect them if their title starts with Merge and contains into

Because that could be good also :)

@yakov116
Copy link
Member

#1121

@fregante

This comment has been minimized.

@fregante
Copy link
Member

fregante commented Feb 23, 2020

I moved the “dimmed merge commits” suggestion to #2827

cc @bastienboutonnet

As a solution to this issue, a new feature could be added: clean-merge-commit-title to create default merge commits as Merge of #133 for example, so it’s less noisy than the default

@bastienboutonnet
Copy link

@fregante I totally buy that!

@anton-bot
Copy link
Author

@fregante

Nice idea but I suggest one thing to consider:

It seems visually easier to skip a merge commit if it has almost exactly the same title as the normal commit. For example, this:

fix: bug in home page (#9345) 
fix: bug in home page

seems easier to skim than this:

Merge PR #9345
fix: bug in home page

@fregante
Copy link
Member

It's easier to skim it if you just avoid merge commits altogether, what's the point? Change the politics if it doesn't help your work. Squash commits and the problem disappears.

#2827 will help reduce the noise at least on GitHub.com

@anton-bot
Copy link
Author

@fregante fair enough, agreed

@fregante
Copy link
Member

I opened a more specific issue ⬆️

@GMNGeoffrey
Copy link

Just to add my perspective, we use both merge commits and "squash and merge" in our project, using merge commits where it's necessary or desirable to preserve history, like for a feature branch. I like linear history for seeing what's actually happened on the default branch and even if you use merge commits you can get this with git log --first-parent, but that only works if the merge commit message isn't utter garbage, which is what GitHub gives you by default. A merge commit message that matches the PR title and body gives you exactly what I'm looking for. Anyway, I think this is getting implemented in https://github.com/zachwhaley/squashed-merge-message, so it's not a super pressing issue for me, but something to consider.

Digging a bit more into the details, in our project we actually have two independent long-lived branches which we merge back and forth between. Doing that without merge commits would be infeasible (I experimented with it).

@ssgosh
Copy link

ssgosh commented Oct 10, 2021

Anyway, I think this is getting implemented in zachwhaley/squashed-merge-message, so it's not a super pressing issue for me, but something to consider.

Thanks, this should be the official workaround for those who want this feature in default merge commits :-)

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

No branches or pull requests

6 participants