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

auto-merge-autosquash #64

Closed
wilzbach opened this issue Mar 1, 2017 · 12 comments
Closed

auto-merge-autosquash #64

wilzbach opened this issue Mar 1, 2017 · 12 comments

Comments

@wilzbach
Copy link
Member

wilzbach commented Mar 1, 2017

Idea from @CyberShadow:

Actually, would be cool if [DLang-Bot] could auto-squash --fixup commits.

@CyberShadow
Copy link
Member

As I've feared, today I noticed the first attempted murder of my elaborate commit messages:

dlang/druntime#1784

Perhaps an improved solution would be:

  • When auto-merging, always fixup commits that are made with --squash / --fixup (i.e. those whose commit messages begin with squash! or fixup! , or those whose commit messages begin with [SQUASH] as this seems to be a common practice
  • Remove the auto-merge-squash label
  • Instruct contributors to format fixup commits' messages in the above-described ways when creating them.

@MartinNowak
Copy link
Member

Agreed that PRs merged in commits like
dlang/phobos@d6d6831
aren't ideal and only leave information on GH, but no longer in the git history.
Overall I find all of this squashing business annoying, it hardly hurts anyone if a merged branch contains several commits. It's only a single merge commit after all.
OTOH github has this annoying visualization where they show all the individual commits, https://github.com/dlang/druntime/commits/master, but still what purpose does squashing serve?

@MartinNowak
Copy link
Member

MartinNowak commented Mar 4, 2017

At the moment we're using GH's API to merge https://developer.github.com/v3/pulls/#merge-a-pull-request-merge-button, so it's not a simple option to do our own merge strategy.

@MartinNowak
Copy link
Member

I'll remove the auto-merge-squash labels until the commit message issues are properly resolved.

@wilzbach
Copy link
Member Author

wilzbach commented Mar 5, 2017

I'll remove the auto-merge-squash labels until the commit message issues are properly resolved.

Looking at the current review queue for the DLang-Bot, this might take a while :/

FWIW isn't it easier to explain to @andralex that you don't want sensible commits to be squashed?

@MartinNowak
Copy link
Member

Currently the bot only has a ref to the PR as merge message, thus all commit messages of the squashed PR are lost, until that get's fixed the feature is outright broken.

@wilzbach
Copy link
Member Author

Currently the bot only has a ref to the PR as merge message, thus all commit messages of the squashed PR are lost, until that get's fixed the feature is outright broken.

But that's intended! If you don't trust the contributors to use it properly, that's a big statement about the circle of trust...
In any case we could do sth. like @CyberShadow and require fixup or squash in the commit messages to be part of at least one the commit message.

@MartinNowak MartinNowak changed the title Auto-sqash --fixup commits auto-merge-autosquash Mar 24, 2017
@MartinNowak
Copy link
Member

MartinNowak commented Mar 24, 2017

But that's intended! If you don't trust the contributors to use it properly, that's a big statement about the circle of trust...

It doesn't even preserve the first commit message! We need to at least preserve what github proposes as commit message when you manually squash.
Let's split of that bug ticket from the --autosquash enhancement though.

@MartinNowak
Copy link
Member

see #66

@MartinNowak
Copy link
Member

Only few people will use it, but I'm for teaching dlang-bot to understand --autosquash for auto-merge-squash, but defaulting to a single commit when no such prefixes are found.
Of course preserving the commit message (of any commit and any squash, but ditching fixups) is a requirement for both.

@wilzbach
Copy link
Member Author

FYI I just re-enabled the auto-merge-squash labels as things like e.g. dlang/druntime#2475 (comment) keep happening rather frequently :/

@CyberShadow
Copy link
Member

Actually, would be cool if [DLang-Bot] could auto-squash --fixup commits.

GitHub now shows diffs for force-pushes, so this feature suggestion is no longer useful. People should just force-push instead of pushing --fixup commits.

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