-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Handle branch name with prefix in GitHub migration #20357
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not bad, since the ref inside Gitea was not clear before.
Update: as discussed below, there should be a clear solution for it, instead of making it more unclear.
Have you considered what happens if you actually want to refer to refs/heads/branch? (i.e. refs/heads/refs/heads/branch) |
I have been intermittently going through the code to double check that this actually still works - have you considered how your code will or will not break that? I think we need to be very very very very very careful with just switching to allow this kind of thing. When Gitea wants to check if there is a branch it should be ensuring that the ref head prefix is there - it should not be removing things. It will cause problems. |
Yup, I agree with your point. I have thought about the possibility that this change might change the behavior, that's what I mean "not bad" (indeed not ideal enough), anyway the ref messy problem is a longstanding problem. If we want to strict the ref usages inside Gitea and strict the behaviors, I think there should be some comments and tests to make sure the logic is clear and make sure future developers can know what is the right thing to do. |
I should add: does this code affect protected branches ? Is there a way of abusing this arbitrary removal of prefix to break through protected branches? |
My thought was this change only causes problem if a user has the branch And yes, this change is not ideal (and as your option, not correct, and now I also agree that it's wrong), so it's better to make |
I agree that we must be cautious about this, and the current change may not be correct (will cause issues in other parts). For how Gitea should be handling tags, I think we could follow how
Then, when both |
I think therefore we agree that this PR is not the correct thing to do then. We should be trying to always pass a ref when we can instead of arbitrarily removing the prefix. By always adding the ref prefix we avoid the ambiguity within git's refname resolution. The problem is that git's refname behaviour is sensible as a cli tool but for a server we have to be more explicit. |
The Releases page should allow both I think now the changes only apply to the Releases page and won't break any other parts. |
Codecov Report
@@ Coverage Diff @@
## main #20357 +/- ##
=======================================
Coverage 46.90% 46.91%
=======================================
Files 981 981
Lines 136262 136266 +4
=======================================
+ Hits 63915 63924 +9
+ Misses 64501 64498 -3
+ Partials 7846 7844 -2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
If I understand correctly, the new code is still "guessing" (ResolveBranch) whether the target is a branch name or a reference. To make the logic stable and clear, could the And cc @zeripath , how do you think? |
A branch name and a reference appeared to have the same effect in git. I think at best we can detect if a reference has the |
I think the correct place to fix this is in the migration - where we should remove/add the refs/heads/ prefix as necessary. Once things are in Gitea itself we should really be assuming that a reference to refs/heads/main is a reference to /refs/heads/refs/heads/main. |
I think now the branch names are resolved during migration. Could you please help to review the changes? @zeripath @wxiaoguang |
Thank you for asking me for review, I am quite busy recently and haven't thought about how to make the branch/tag/commit-id more consistent in this case. Maybe other maintainers could help out. |
@zeripath how do you think about current approach? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we currently don't prefix things with refs/heads/ I think this makes things consistent and therefore it can be merged. We should however, consider doing the opposite of this PR and instead always add the prefix in other contexts but I don't have time right now to go through all of the migration types to get this correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Current approach only affects the GithubDownloaderV3
, I think it's fine to take it as a quick fix (patch).
To make the commitish clear, it still needs a lot of work in the future.
* giteaofficial/main: Remove Gusted as Gitea maintainer (go-gitea#21676) Fix token generation when using INTERNAL_TOKEN_URI (go-gitea#21669) Clean up formatting on install page (go-gitea#21668) Add Webhook authorization header (go-gitea#20926) feat: notify doers of a merge when automerging (go-gitea#21553) Remove deprecated DSA host key from Docker Container (go-gitea#21522) Alter package_version.metadata_json to LONGTEXT (go-gitea#21667) Handle branch name with prefix in GitHub migration (go-gitea#20357) [skip ci] Updated translations via Crowdin Split migrations folder (go-gitea#21549)
GitHub has a blog post today about ambiguous branch names. I'm commenting here for references, and it maybe something to watch to make the commitish clear. |
GitHub allows releases with target commitish
refs/heads/BRANCH
, which then causes issues in Gitea after migration. This fix handles cases that a branch already has a prefix.Fixes #20317