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

Fix sync fork for consistency #33147

Merged
merged 11 commits into from
Jan 10, 2025
Merged

Fix sync fork for consistency #33147

merged 11 commits into from
Jan 10, 2025

Conversation

changchaishi
Copy link
Contributor

Fixes #33145

An integration test could be added.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 8, 2025
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 8, 2025
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Jan 8, 2025
@lunny lunny requested a review from wxiaoguang January 8, 2025 06:11
@changchaishi changchaishi marked this pull request as draft January 8, 2025 06:12
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jan 8, 2025

An integration test could be added.

I have some tests and will add them (see Improve "merge upstream" (sync fork) #33148 )

But I am not sure whether we should still use the "add remote" trick here, it seems fragile and I am not sure whether it is really helpful in daily usage.

@changchaishi
Copy link
Contributor Author

But I am not sure whether we should still use the "add remote" trick here, it seems fragile and I am not sure whether it is really helpful in daily usage.

This is really what I am concerned about before submitting the PR.

One more bug I found on services/repository/merge_upstream.go:35, the current sync fork is not correct, should be base repo's defaultBranch.

https://github.com/orgs/community/discussions/11729

@wxiaoguang
Copy link
Contributor

But I am not sure whether we should still use the "add remote" trick here, it seems fragile and I am not sure whether it is really helpful in daily usage.

This is really what I am concerned about before submitting the PR.

One more bug I found on services/repository/merge_upstream.go:35, the current sync fork is not correct, should be base repo's defaultBranch.

https://github.com/orgs/community/discussions/11729

If you have more ideas about the problem, feel free to take #33148 (there are more tests, I could close my PR if you'd like to use it in your PR)

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jan 8, 2025

One more bug I found on services/repository/merge_upstream.go:35, the current sync fork is not correct, should be base repo's defaultBranch.

https://github.com/orgs/community/discussions/11729

At the moment, there is something unclear in my mind.

There could be 2 behaviors:

  1. "Sync fork" only sync the default branch (or name-matched branch), the current behavior:
    • The typical workflow is like this: the user forks the repo, then base repo's default changes a lot, then the user sync the default branch into the fork, and start a new branch from the fork's default branch.
    • In this case, if a user would like to update (sync) a PR, they could just go to the PR's page and click the "update" button there
  2. "Sync fork" sync everything basing on the base repo's default branch (your suggestion?)
    • There will be more edge cases to handle conflicts, and it duplicates with the "update" button on the PR page.

@changchaishi
Copy link
Contributor Author

I see~

For 1. I did not notice this is the intended behavior of Gitea right now, as I am studying the code, I refer to GitHub a lot, Github has
the Number 2 behavior, so I thought this was a kind of bug.

I think we can simplify this PR without this issue first.

One more bug I found on services/repository/merge_upstream.go:35, the current sync fork is not correct, should be the base repo's defaultBranch.

https://github.com/orgs/community/discussions/11729


If you have more ideas about the problem, feel free to take #33148 (there are more tests, I could close my PR if you'd like to use it in your PR)

I reviewed it, and this is nice, how can I use the test? Must you close it so that I can use it, or I can just reference it to create something like repo_sync_fork_rest.py

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jan 8, 2025

Actually I am not sure which behavior is better for end users. Maybe following GitHub's behavior is better since it is widely used. 🤣


If you have more ideas about the problem, feel free to take #33148 (there are more tests, I could close my PR if you'd like to use it in your PR)

I reviewed it, and this is nice, how can I use the test? Must you close it so that I can use it, or I can just reference it to create something like repo_sync_fork_rest.py

Some possible choices:

@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 9, 2025
@changchaishi changchaishi changed the title [WIP] Fix sync fork for consistency Fix sync fork for consistency Jan 9, 2025
@changchaishi changchaishi marked this pull request as ready for review January 9, 2025 10:15
@wxiaoguang
Copy link
Contributor

If you don't mind, I think I can merge #33148 here, then we do not need to use a separate PR to handle the similar tests.

@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 10, 2025
@wxiaoguang wxiaoguang added type/bug backport/v1.23 This PR should be backported to Gitea 1.23 labels Jan 10, 2025
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 10, 2025
@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Jan 10, 2025
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jan 10, 2025

Made some new changes:

  1. merge WIP: Improve "merge upstream" (sync fork) #33148 and reuse the tests
    • the new case is covered by BaseChangeAfterHeadChange
  2. use base repo's default branch to sync (as you suggested and as GitHub does)

@github-actions github-actions bot added the modifies/templates This PR modifies the template files label Jan 10, 2025
@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 Jan 10, 2025
@wxiaoguang wxiaoguang added this to the 1.24.0 milestone Jan 10, 2025
@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 Jan 10, 2025
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jan 10, 2025
@lunny lunny enabled auto-merge (squash) January 10, 2025 05:27
@lunny lunny merged commit e5f3c16 into go-gitea:main Jan 10, 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 Jan 10, 2025
@changchaishi changchaishi deleted the fix-sync-fork branch January 10, 2025 05:30
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Jan 10, 2025
Fixes go-gitea#33145

An integration test could be added.

---------

Co-authored-by: wxiaoguang <[email protected]>
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Jan 10, 2025
wxiaoguang added a commit that referenced this pull request Jan 10, 2025
Backport #33147 by changchaishi

Fixes #33145

An integration test could be added.

---------

Co-authored-by: Chai-Shi <[email protected]>
Co-authored-by: wxiaoguang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/done All backports for this PR have been created backport/v1.23 This PR should be backported to Gitea 1.23 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strange behaviors of the Sync Fork feature
4 participants