Skip to content

Conversation

lunny
Copy link
Member

@lunny lunny commented Oct 8, 2025

To prevent potential bugs, the logic in #35543 makes gitcmd.Command panic when attempting to override stdout or stderr. Instead of using PrepareCmd, this PR now uses the WithXXX methods directly to avoid the panic.

Fix #35603

@lunny lunny added this to the 1.26.0 milestone Oct 8, 2025
@lunny lunny added type/bug skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Oct 8, 2025
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 8, 2025
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Oct 8, 2025
@badhezi
Copy link
Contributor

badhezi commented Oct 8, 2025

we should probably also add a testcase that merges a pull request with the head_commit_id form value which is the default when merging from UI

@lunny
Copy link
Member Author

lunny commented Oct 9, 2025

we should probably also add a testcase that merges a pull request with the head_commit_id form value which is the default when merging from UI

The merge tests already exist, but they contained bugs that prevented the previous issue from being detected. I’ve submitted commit 50d3d94 to fix the test bug.

@lunny lunny requested a review from wxiaoguang October 9, 2025 05:07
@badhezi
Copy link
Contributor

badhezi commented Oct 9, 2025

@lunny
I meant that we need to modify this test case to include head_commit_id here

options := map[string]string{
  "_csrf": htmlDoc.GetCSRF(),
  "do":    string(mergeStyle),
}

Otherwise we will not reach the show-ref command and this is why this test did not catch this bug before

@wxiaoguang wxiaoguang removed their request for review October 9, 2025 07:30
@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 Oct 10, 2025
@lunny
Copy link
Member Author

lunny commented Oct 10, 2025

@lunny I meant that we need to modify this test case to include head_commit_id here

options := map[string]string{
  "_csrf": htmlDoc.GetCSRF(),
  "do":    string(mergeStyle),
}

Otherwise we will not reach the show-ref command and this is why this test did not catch this bug before

763d8c8 added a new merge test with head commit id.

@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 Oct 11, 2025
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Oct 11, 2025
@techknowlogick techknowlogick merged commit 662a44d into go-gitea:main Oct 12, 2025
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Oct 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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 skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cannot merge pull request

6 participants