Skip to content

Allow empty commit when merging pull request with squash style #35989

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Nov 22, 2025
Merged

Allow empty commit when merging pull request with squash style #35989

merged 6 commits into from
Nov 22, 2025
+38 −1

Conversation

@lunny
Copy link
Member

@lunny lunny commented Nov 20, 2025 •
edited
Loading

Before this PR, when merging an empty PR with squash style will result in 500.

How to reproduce the issue:
1. Create a new branch dev from main.
2. Add some changes and commit them on the dev branch.
3. Open a pull request from dev to main.
4. Manually cherry-pick the commit from dev into main.
5. Open the pull request page and attempt a Squash Merge → a 500 error occurs.

This PR will allow the behavior.

141a51853afac0686fa213af5d7c1f66

@lunny lunny added this to the 1.26.0 milestone Nov 20, 2025
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 20, 2025
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Nov 20, 2025
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Nov 20, 2025

Why user can't provide a message? What's the use case for keeping the message empty?

@lunny
Copy link
Member Author

lunny commented Nov 20, 2025

Why user can't provide a message? What's the use case for keeping the message empty?

It's an empty commit, not an empty commit message.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Nov 20, 2025

Why user can't provide a message? What's the use case for keeping the message empty?

It's an empty commit, not an empty commit message.

OK, then why an empty commit should be supported? What's the use case

@lunny
Copy link
Member Author

lunny commented Nov 20, 2025

Why user can't provide a message? What's the use case for keeping the message empty?

It's an empty commit, not an empty commit message.

OK, then why an empty commit should be supported? What's the use case

I updated the issue content. Looks at the image on that. The previous implementation in Gitea has a warning hint there but it said this will be an empty commit. So that to keep the behavior consistent, it's nature to allow commit the empty commit. I can also change the hint and disable the button but I think that should be break change?

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Nov 20, 2025

Without the AddArguments("--allow-empty"), your TestPullSquashMergeEmpty still passes.

If it tests nothing, you can delete the test to avoid wasting CI time.

@silverwind
Copy link
Member

silverwind commented Nov 20, 2025

I'm confused because the screenshot shows "1 files changed", but a empty commit would actually be "0 files changed". Are you sure it's targeting the correct mechanism?

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Nov 20, 2025

I'm confused because the screenshot shows "1 files changed", but a empty commit would actually be "0 files changed". Are you sure it's targeting the correct mechanism?

That's how git works.

  • On "main" branch: set "file.txt" content to "dummy"
  • Create a new "branch-1", edit "file.txt", change its content to "changed" and create a PR
  • On "main" branch: set "file.txt" content to "changed"
  • Then "branch-1" can be merged into "main" without conflict or change.

@silverwind
Copy link
Member

silverwind commented Nov 20, 2025 •
edited
Loading

So basically a branch that once had changes, but was then made to match the target branch. I still think the UI should show "0 changed files" in such a case as I'm pretty sure git diff would also show empty in such a case. In any case, this discussion is offtopic :)

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Nov 20, 2025

So basically a branch that once had changes, but was then made to match the target branch. I still think the UI should show "0 changed files" in such a case as I'm pretty sure git diff would also show empty in such a case. In any case, this discussion is offtopic :)

No. The "diff" is from its merge base.

If you want to see 0, click that "Update branch" button.

@lunny
Copy link
Member Author

lunny commented Nov 20, 2025

How to reproduce the issue:
1. Create a new branch dev from main.
2. Add some changes and commit them on the dev branch.
3. Open a pull request from dev to main.
4. Manually cherry-pick the commit from dev into main.
5. Open the pull request page and attempt a Squash Merge → a 500 error occurs.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Nov 20, 2025

Yep, it can be reproduced if there is nothing to merge. My comment #35989 (comment) can also easily reproduce.

But the problem is: your test code is not right

Without the AddArguments("--allow-empty"), your TestPullSquashMergeEmpty still passes.
If it tests nothing, you can delete the test to avoid wasting CI time.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 22, 2025
wxiaoguang
wxiaoguang reviewed Nov 22, 2025
View reviewed changes
@wxiaoguang
Update .editorconfig
7167a5e 
Signed-off-by: wxiaoguang <wxiaoguang@gmail.com>
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Nov 22, 2025
@wxiaoguang wxiaoguang enabled auto-merge (squash) November 22, 2025 05:33
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Nov 22, 2025
@wxiaoguang wxiaoguang merged commit a60a8c6 into go-gitea:main Nov 22, 2025
25 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Nov 22, 2025
@lunny lunny deleted the lunny/allow_empty_squash_merge_pr branch November 22, 2025 06:27
lunny added a commit to lunny/gitea that referenced this pull request Nov 22, 2025
@lunny @Zettat123 @wxiaoguang
Allow empty commit when merging pull request with squash style (go-gi…
2d58cbf 
…tea#35989)

Before this PR, when merging an empty PR with squash style will result
in 500.

---------

Signed-off-by: wxiaoguang <wxiaoguang@gmail.com>
Co-authored-by: Zettat123 <zettat123@gmail.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@lunny lunny mentioned this pull request Nov 22, 2025
Merged
@lunny lunny added the backport/done All backports for this PR have been created label Nov 22, 2025
zjjhot added a commit to zjjhot/gitea that referenced this pull request Nov 22, 2025
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
4030b8b 
* giteaofficial/main:
  Fix various permission & login related bugs (go-gitea#36002)
  Allow empty commit when merging pull request with squash style (go-gitea#35989)
  [skip ci] Updated translations via Crowdin
  Mention proc-receive in text for dashboard.resync_all_hooks func (go-gitea#35991)
  Update JS deps (go-gitea#35978)
  wiki: reuse selectable style for wiki (go-gitea#35990)
lunny added a commit that referenced this pull request Nov 22, 2025
@lunny
Allow empty commit when merging pull request with squash style (#35989)…
91901c2 
… (#36003)
zjjhot added a commit to zjjhot/gitea that referenced this pull request Nov 24, 2025
@zjjhot
Merge remote-tracking branch 'giteaofficial/release/v1.25'
0a09b13 
* giteaofficial/release/v1.25: (77 commits)
  Add "site admin" back to profile menu (go-gitea#36010) (go-gitea#36013)
  release notes for 1.25.2 (go-gitea#35986)
  Allow empty commit when merging pull request with squash style (go-gitea#35989) (go-gitea#36003)
  Fix various permission & login related bugs (go-gitea#36002) (go-gitea#36004)
  upgrade golang.org/x/crypto to 0.45.0 (go-gitea#35988)
  Change project default column icon to 'star' (go-gitea#35967) (go-gitea#35979)
  Misc CSS fixes (go-gitea#35888) (go-gitea#35981)
  Fix container push tag overwriting (go-gitea#35936) (go-gitea#35954)
  Fix corrupted external render content (go-gitea#35946) (go-gitea#35950)
  Don't show unnecessary error message to end users for DeleteBranchAfterMerge (go-gitea#35937) (go-gitea#35941)
  Limit read bytes instead of ReadAll (go-gitea#35928) (go-gitea#35934)
  Load jQuery as early as possible to support custom scripts (go-gitea#35926) (go-gitea#35929)
  Allow to display embed images/pdfs when SERVE_DIRECT was enabled on MinIO storage (go-gitea#35882) (go-gitea#35917)
  Use correct form field for allowed force push users in branch protection API (go-gitea#35894) (go-gitea#35908)
  Make OAuth2 issuer configurable (go-gitea#35915) (go-gitea#35916)
  Fix go-gitea#35763: Add proper page title for project pages (go-gitea#35773) (go-gitea#35909)
  Display source code downloads last for release attachments (go-gitea#35897) (go-gitea#35903)
  Fix team member access check (go-gitea#35899) (go-gitea#35905)
  Fix conda null depend issue (go-gitea#35900) (go-gitea#35902)
  Fix avatar upload error handling (go-gitea#35887) (go-gitea#35890)
  ...

# Conflicts:
#	go.mod
#	go.sum
#	models/actions/run_test.go
#	models/fixtures/action_run.yml
#	models/fixtures/action_run_job.yml
#	models/fixtures/action_task.yml
#	models/fixtures/branch.yml
#	models/fixtures/repo_unit.yml
#	modules/git/tree_entry_gogit.go
#	modules/git/tree_gogit.go
#	routers/web/repo/actions/view.go
#	routers/web/repo/issue_comment.go
#	services/actions/workflow.go
#	services/doctor/actions_test.go
#	services/pull/comment.go
#	services/pull/pull.go
#	services/pull/temp_repo.go
#	templates/base/head_navbar.tmpl
#	templates/swagger/v1_json.tmpl
#	tests/integration/actions_schedule_test.go
#	tests/integration/git_lfs_ssh_test.go
#	tests/integration/pull_create_test.go
#	tests/integration/pull_merge_test.go
#	tests/sqlite.ini.tmpl
#	web_src/js/components/ContextPopup.vue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wxiaoguang wxiaoguang wxiaoguang approved these changes

+1 more reviewer

@Zettat123 Zettat123 Zettat123 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

backport/done All backports for this PR have been created backport/v1.25 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code modifies/internal

Projects

None yet

Milestone

1.26.0

Development

Successfully merging this pull request may close these issues.

5 participants

@lunny @wxiaoguang @silverwind @Zettat123 @GiteaBot