Handle branch name with prefix in GitHub migration #20357

harryzcy · 2022-07-13T18:40:03Z

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

wxiaoguang

~~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.

zeripath · 2022-07-14T11:02:34Z

Have you considered what happens if you actually want to refer to refs/heads/branch? (i.e. refs/heads/refs/heads/branch)

zeripath · 2022-07-14T11:05:21Z

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.

wxiaoguang · 2022-07-14T11:13:09Z

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.

zeripath · 2022-07-14T11:28:26Z

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?

need a more clear solution

wxiaoguang · 2022-07-14T12:07:58Z

My thought was this change only causes problem if a user has the branch refs/heads/refs/heads/BRANCH, which means they use refs/heads/BRANCH for their branch name, which is already somewhat confusing, so it could be considered as the user's decision.

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 GetBranchCommitID etc covered by tests and add more comments for it, to avoid abuse or incorrect change.

harryzcy · 2022-07-14T18:27:06Z

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 git rev-parse handles <refname>

<refname>, e.g. master, heads/master, refs/heads/master
A symbolic ref name. E.g. master typically means the commit object referenced by refs/heads/master. If you happen to have both heads/master and tags/master, you can explicitly say heads/master to tell Git which one you mean. When ambiguous, a <refname> is disambiguated by taking the first match in the following rules:

If $GIT_DIR/<refname> exists, that is what you mean (this is usually useful only for HEAD, FETCH_HEAD, ORIG_HEAD, MERGE_HEAD and CHERRY_PICK_HEAD);

otherwise, refs/<refname> if it exists;

otherwise, refs/tags/<refname> if it exists;

otherwise, refs/heads/<refname> if it exists;

otherwise, refs/remotes/<refname> if it exists;

otherwise, refs/remotes/<refname>/HEAD if it exists.

Then, when both refs/heads/<refname> and refs/heads/refs/heads/<refname> exists, the latter should be explicitly expressed by heads/refs/heads/<refname> or refs/heads/refs/heads/<refname>. I think this behavior should be consistent throughout.

zeripath · 2022-07-14T19:02:23Z

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.

harryzcy · 2022-07-14T20:25:35Z

The Releases page should allow both refs/heads/<refname> and <refname>.

I think now the changes only apply to the Releases page and won't break any other parts.

codecov-commenter · 2022-07-14T21:14:59Z

Codecov Report

Merging #20357 (c3d4c98) into main (498352c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           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

Impacted Files	Coverage Δ
services/migrations/github.go	`65.55% <100.00%> (+0.20%)`	⬆️
modules/queue/queue_channel.go	`78.70% <0.00%> (-1.86%)`	⬇️
modules/queue/queue_disk_channel.go	`68.33% <0.00%> (-1.25%)`	⬇️
services/pull/check.go	`27.95% <0.00%> (-1.19%)`	⬇️
services/pull/pull.go	`40.18% <0.00%> (-0.62%)`	⬇️
routers/api/v1/repo/pull.go	`47.00% <0.00%> (-0.18%)`	⬇️
modules/queue/queue_bytefifo.go	`49.09% <0.00%> (+1.80%)`	⬆️
modules/git/repo_attribute.go	`72.63% <0.00%> (+2.10%)`	⬆️
modules/charset/charset.go	`75.36% <0.00%> (+3.62%)`	⬆️
modules/process/manager_exec.go	`93.02% <0.00%> (+6.97%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

wxiaoguang · 2022-07-22T09:52:41Z

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 target be clarified to be a reference?

And cc @zeripath , how do you think?

harryzcy · 2022-07-22T19:23:21Z

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 target be clarified to be a reference?

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 BranchPrefix and try to resolve it as a reference as is.

zeripath · 2022-07-22T20:19:03Z

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.

harryzcy · 2022-08-22T06:34:19Z

I think now the branch names are resolved during migration. Could you please help to review the changes? @zeripath @wxiaoguang

wxiaoguang · 2022-08-22T07:09:43Z

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.

wxiaoguang · 2022-10-30T09:18:34Z

@zeripath how do you think about current approach?

zeripath

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.

wxiaoguang

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)

harryzcy · 2023-03-23T15:57:06Z

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.

harryzcy added 2 commits July 13, 2022 14:34

Handle branch name with prefix already

084c9c6

Merge remote-tracking branch 'upstream/main' into branch-with-prefix

a2c9b95

wxiaoguang previously approved these changes Jul 14, 2022

View reviewed changes

GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jul 14, 2022

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 14, 2022

wxiaoguang removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jul 14, 2022

harryzcy added 3 commits July 14, 2022 16:09

Allow refname resolution in Releases page only

05134fb

Update comment

98c466b

Implement the same method for gogit

6e1ee49

harryzcy requested a review from wxiaoguang July 22, 2022 08:22

Ensure reference has BranchPrefix

c3351dc

harryzcy added 3 commits July 22, 2022 21:14

Revert all changes

4fd2ff5

Revert errors

e4aedf2

Convert github release commitish during migration

ba30dbd

harryzcy changed the title ~~Handle branch name with prefix already~~ Handle branch name with prefix in migration Jul 23, 2022

Merge branch 'main' into branch-with-prefix

c3d4c98

Merge branch 'main' into branch-with-prefix

f72288b

wxiaoguang mentioned this pull request Sep 3, 2022

Fill the specified ref in webhook test payload #20961

Merged

Merge branch 'main' into branch-with-prefix

511e3a0

lunny added the type/bug label Oct 30, 2022

zeripath approved these changes Nov 2, 2022

View reviewed changes

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 2, 2022

wxiaoguang approved these changes Nov 2, 2022

View reviewed changes

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 2, 2022

harryzcy and others added 2 commits November 2, 2022 13:48

Merge branch 'main' into branch-with-prefix

8018359

Merge branch 'main' into branch-with-prefix

719c5ae

wxiaoguang approved these changes Nov 3, 2022

View reviewed changes

wxiaoguang merged commit 81ea4f9 into go-gitea:main Nov 3, 2022

wxiaoguang changed the title ~~Handle branch name with prefix in migration~~ Handle branch name with prefix in GitHub migration Nov 3, 2022

harryzcy deleted the branch-with-prefix branch November 3, 2022 06:48

GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Mar 23, 2023

go-gitea locked and limited conversation to collaborators May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle branch name with prefix in GitHub migration #20357

Handle branch name with prefix in GitHub migration #20357

harryzcy commented Jul 13, 2022 •

edited

Loading

wxiaoguang left a comment •

edited

Loading

zeripath commented Jul 14, 2022 •

edited

Loading

zeripath commented Jul 14, 2022

wxiaoguang commented Jul 14, 2022

zeripath commented Jul 14, 2022

wxiaoguang commented Jul 14, 2022 •

edited

Loading

harryzcy commented Jul 14, 2022

zeripath commented Jul 14, 2022

harryzcy commented Jul 14, 2022

codecov-commenter commented Jul 14, 2022 •

edited

Loading

wxiaoguang commented Jul 22, 2022

harryzcy commented Jul 22, 2022

zeripath commented Jul 22, 2022

harryzcy commented Aug 22, 2022

wxiaoguang commented Aug 22, 2022

wxiaoguang commented Oct 30, 2022

zeripath left a comment

wxiaoguang left a comment

harryzcy commented Mar 23, 2023

Handle branch name with prefix in GitHub migration #20357

Handle branch name with prefix in GitHub migration #20357

Conversation

harryzcy commented Jul 13, 2022 • edited Loading

wxiaoguang left a comment • edited Loading

Choose a reason for hiding this comment

zeripath commented Jul 14, 2022 • edited Loading

zeripath commented Jul 14, 2022

wxiaoguang commented Jul 14, 2022

zeripath commented Jul 14, 2022

wxiaoguang commented Jul 14, 2022 • edited Loading

harryzcy commented Jul 14, 2022

zeripath commented Jul 14, 2022

harryzcy commented Jul 14, 2022

codecov-commenter commented Jul 14, 2022 • edited Loading

Codecov Report

wxiaoguang commented Jul 22, 2022

harryzcy commented Jul 22, 2022

zeripath commented Jul 22, 2022

harryzcy commented Aug 22, 2022

wxiaoguang commented Aug 22, 2022

wxiaoguang commented Oct 30, 2022

zeripath left a comment

Choose a reason for hiding this comment

wxiaoguang left a comment

Choose a reason for hiding this comment

harryzcy commented Mar 23, 2023

harryzcy commented Jul 13, 2022 •

edited

Loading

wxiaoguang left a comment •

edited

Loading

zeripath commented Jul 14, 2022 •

edited

Loading

wxiaoguang commented Jul 14, 2022 •

edited

Loading

codecov-commenter commented Jul 14, 2022 •

edited

Loading