search

go-gitea / gitea

Git with a cup of tea! Painless self-hosted all-in-one software development service, including Git hosting, code review, team collaboration, package registry and CI/CD
https://gitea.com
MIT License
44.72k stars 5.46k forks source link

Mark PR as WIP if it has temporary commits #26788

Open XonqNopp opened 1 year ago

XonqNopp commented 1 year ago

Feature Description

When we do a PR and there is a review, we have to push new commits to fix things. We usually take advantage of git's autosquash feature, so the new commits are prefixed with squash! or fixup!. The intention is to squash the history before merging. But sometimes we have to act fast and it happened that we merged with those commits still there.

My idea would be that when we push new commits to an open PR, we check all the commits in the PR (including those pushed previously) and if any has the prefix squash! or fixup!, the PR would then be marked as WIP so it blocks the "merge" button.

One step further would be to act the same way for commits which are only done for the purpose of testing the PR but should not be merged. Such commits could be prefix by drop! or wip! (to be defined). One example is that we do this because we only want to run part of the tests.

Anyway, if this is not clear don't hesitate to ask follow-up questions. And thanks for the great work :)

cc @99rgosse @gzzi

Screenshots

No response

silverwind commented 1 year ago

This seems very specific to your workflow and not something I see as a general mechanism because nothing about fixup! or squash! indicates a wip status. Maybe it can be accepted as an global opt-in feature behind a app.ini pref, but certainly not the default as that would be very confusing to users not using such a workflow.

delvh commented 1 year ago

I think even better is using the CI coupled with the API to achieve that. Although the API might not be equipped yet with the necessary routes to ensure that.

wxiaoguang commented 1 year ago

But sometimes we have to act fast and it happened that we merged with those commits still there.

How could it happen?

The merger should pay enough attention to read the PR's title/description/changes and edit the merge message properly.

If you worry about "merging with 'squash!'" commit, you could also merge bugs/bad code even if there is no "squash!" but you merge too fast.

XonqNopp commented 1 year ago

I mean the code is thoroughly reviewed, but fixes are pushed as squash/fixup commits. And sometimes you just forget to squash the history. Anyway, squash/fixup commits are not meant to be regular commits, these prefixes are only here because git uses them for autosquash, so I don't think anybody would want to merge a branch with such commits...?

silverwind commented 1 year ago

I'd happily squash-merge such a PR I guess.

XonqNopp commented 1 year ago

What do you mean? You would squash all commits in only a single one? That is not our workflow at all... We want to keep all the commits which are intended to be there, we just do not want to merge fixup!/squash! commits... I am not sure to understand what you are saying...?

a1012112796 commented 1 year ago

I think checking it in ci is a good solution.

XonqNopp commented 1 year ago

That is not ideal at all for us... It would mean that we add a step after the clone where we check history and fails if it find any of those commits. But then we cannot test while having these commits, so we would need to squash the history before being able to test, which is not ideal because if the fixes broke the build it is interesting to have them in new commits to better check what was wrong...

But now I am wondering... Are there really people with a workflow where you merge such commits? For me it does not make sense, since the purpose of these commits is to be squashed.....?