search

Mergifyio / mergify

Mergify Community Issue Tracker
https://mergify.com
Apache License 2.0
320 stars 92 forks source link

Allow contributor to clean up commit message before merge. #79

Open jguiditta opened 6 years ago

jguiditta commented 6 years ago

Many times, when a patch is submitted as a PR, there will be back and forth to get it in shape to be merged. A lot of people like to make those fixes in incremental patches to make it easier to see what they have changed. At the end of the process, these patches are squashed into the original. I would be nice if we could use something like require_code_owner_reviews to allow the author a chance to squash it themselves, fix the commit message, and then hand it back to mergify. The current behavior of this setting looks like it uses the code_owners file, which is different than what I am trying to suggest, which is the author of this patch.

jd commented 6 years ago

How do you want to do that automatically? I'm missing something here.

jguiditta commented 6 years ago

My thought was that the author would 'approve' the commit, meaning they are ready for it to be merged once it has the proper reviews. This would likely be tracked separately from the standard review approval, since you woudl not want to author to be able to approve their own code without others reviewing it. If this flag were set, and the review had 2 other approvals (assuming you set your project to need 2, just as an example), only once the author had added their approval would it be merged

jd commented 6 years ago

As soon as you squash the commits and repush the branch, the reviews are dismissed because there are no more common commits with the previous reviews. So the review has to start again anyway. It sounds to me that what describes wouldn't be useful here, no?

jguiditta commented 6 years ago

I could be wrong, but it seems that in the case that you are merging as a squash commit anyway, whether the squash is done by mergify or the author should not make any different in the review process. If there is something there that I am not aware of that makes it different, I think I would be ok with saying the reviewers need to approve again once the author squashed it

jd commented 6 years ago

There's probably something I miss to because I really don't understand the feature you need/want. :)

If it's about squashing, then it's either you squash on the author side and then you need committers reviews, or you do it on merge side via GitHub/Mergify, and there's nothing to do.

Or is this about setting a patch to WIP and then "ready to be reviewed"?

fuzzball81 commented 6 years ago

@jd Recent @jguiditta and myself ran across a situation where the author of the pull request made several changes as a result of the review, generating several additional commits. Unfortunately they pull request text no longer matched the final state of the code. The author wanted an opportunity to squash their commits and update the pull request text prior to the merge. If mergify was being used, the commits would of been squashed automatically and the existing (incomplete) pull request text used since reviewers had approved the changes.

The feature we are looking for is something that lets the author of the pull request/commits approve the state of things before the auto-merge were to take place. This would given the author the opportunity to adjust things like the pull request text prior to the squash. If the author chooses to squash the commits themselves, its OK to require approval from reviewers since the code has changed and should be verified.

I hope that helps.

jd commented 6 years ago

Ok, I see what you mean after all. You basically want a big WIP flag that prevents the merge until the author approve its own PR. That's possible, the only question is "how" to implement it. A contributor does not have any right to put a label, etc.

Maybe Mergify could ignore PR with something like [WIP] in the title. That'd require filtering on PR, which is discussed in #42.

jguiditta commented 6 years ago

@fuzzball81 thanks for clarifying that, I was having trouble making my thought clear to others. @jd yes, some sort of flag that prevents merge until approved by author. However (and hopefully I don't confuse things again by saying this), I think it would be best if such a flag could only be enabled if additional patches are pushed into the PR branch as a result of reviews. I realize this last part may or may not even be possible, just describing what I think would be ideal behavior.