Match unqualified references when syncing pulls as well #23070

lflare · 2023-02-22T11:31:56Z

It seems that opts.RefFullName may occassionally be set to just the branch name, without the refs/heads/ prefixing.

wxiaoguang · 2023-02-22T13:58:08Z

Good catch. There are some details about git ref.

IMO the bug is in parseRemoteUpdateOutput, it should prepare a correct refName (with refs/xxx prefix) in all cases.

In other functions, when they see a variable called refName , the refName should be guaranteed to have the refs/ prefix if it's a branch name or a tag name, to avoid guessing ref names like refs/heads/main or refs/heads/refs/heads/main.

lflare · 2023-02-22T14:31:57Z

Good catch. There are some details about git ref.

IMO the bug is in parseRemoteUpdateOutput, it should prepare a correct refName (with refs/xxx prefix) in all cases.

In other functions, when they see a variable called refName , the refName should be guaranteed to have the refs/ prefix if it's a branch name or a tag name, to avoid guessing ref names like refs/heads/main or refs/heads/refs/heads/main.

I actually expected someone to point that out, unfortunately, it seems to be parsing remote output to get the tag, and from my logging as depicted below,

	for i := range lines {
		// Make sure reference name is presented before continue
		idx := strings.Index(lines[i], "-> ")
		if idx == -1 {
			continue
		}

		refName := lines[i][idx+3:]

		log.Info("stats_indexer.parseRemoteUpdateOutput(%d) RefFullName: %s, %s", 0, refName, lines[i])

		switch {
		case strings.HasPrefix(lines[i], " * "): // New reference
			if strings.HasPrefix(lines[i], " * [new tag]") {
				refName = git.TagPrefix + refName
			} else if strings.HasPrefix(lines[i], " * [new branch]") {
				refName = git.BranchPrefix + refName
			}
			results = append(results, &mirrorSyncResult{
				refName:

This is the output it processes when it is doing a pull,

2023/02/22 11:22:38 ...irror/mirror_pull.go:100:parseRemoteUpdateOutput() [I] [63f5fafd-9] stats_indexer.parseRemoteUpdateOutput(0) RefFullName: main,    feedaf2..cfd6d4d  main       -> main
2023/02/22 11:22:38 ...irror/mirror_pull.go:158:parseRemoteUpdateOutput() [I] [63f5fafd-9] stats_indexer.parseRemoteUpdateOutput(1) RefFullName: main,    feedaf2..cfd6d4d  main       -> main

Specifically the line feedaf2..cfd6d4d main -> main. This could be a tag, like refs/tag/main, or it could be a branch, like refs/head/main, no way to tell.

Personally, I'm not opposed to just assuming it's a branch by default, or with my current PR, matching it regardless of whether it has a prefix. I am more concerned about not letting this issue remain an issue, when the "fix" for it, is fairly clear.

wxiaoguang · 2023-02-22T14:59:40Z

According to my test. git remote update only outputs the following logs for branches:

From github.com:wxiaoguang/gitea
   f74293f9c2..90a7bba1f2  test-branch-1 -> wxiaoguang/test-branch-1

If I change/delete the tags on remote, git remote update outputs nothing. So I think it's fine to make refName as branch in parseRemoteUpdateOutput in such case.

lflare · 2023-02-22T15:21:39Z

According to my test. git remote update only outputs the following logs for branches:
From github.com:wxiaoguang/gitea
   f74293f9c2..90a7bba1f2  test-branch-1 -> wxiaoguang/test-branch-1
If I change/delete the tags on remote, git remote update outputs nothing. So I think it's fine to make refName as branch in parseRemoteUpdateOutput in such case.

Given that the code in this PR is only targeting UpdateRepoIndexer(), I don't believe there's any serious implications if we just allowed (opts.RefFullName == git.BranchPrefix+repo.DefaultBranch || opts.RefFullName == repo.DefaultBranch)?

wxiaoguang · 2023-02-22T15:27:53Z

Im my mind, storing a non-ref value into a variable called ref seems misleading ....

Maybe @zeripath could help to take a look at this problem, he has spent a lot of time cleaning up the legacy ref problems in Gitea.

lunny · 2023-02-24T07:32:57Z

Maybe we should change func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bool) to make the returned result as full ref name.

   342b573..cfce25f  main         -> origin/main
   c00418e..fff1af1  release/v0.9 -> origin/release/v0.9
 * [new tag]         v0.9.2       -> v0.9.2
 * [new tag]         v0.9.1       -> v0.9.1

Maybe it's not about missing refs but have an extra origin/? So this should be a bug of parseRemoteUpdateOutput when handle new commits for a branch. The branch name became origin/main, but it should be refs/heads/main.

lflare · 2023-02-25T01:26:54Z

Maybe we should change func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bool) to make the returned result as full ref name.
   342b573..cfce25f  main         -> origin/main
   c00418e..fff1af1  release/v0.9 -> origin/release/v0.9
 * [new tag]         v0.9.2       -> v0.9.2
 * [new tag]         v0.9.1       -> v0.9.1
Maybe it's not about missing refs but have an extra origin/? So this should be a bug of parseRemoteUpdateOutput when handle new commits for a branch. The branch name became origin/main, but it should be refs/heads/main.

There are also instances where origin/ may not even be in the response. As mentioned in my previous reply, there are times where the output is,

    feedaf2..cfd6d4d  main       -> main

lunny · 2023-03-02T05:38:55Z

Maybe we should change func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bool) to make the returned result as full ref name.
   342b573..cfce25f  main         -> origin/main
   c00418e..fff1af1  release/v0.9 -> origin/release/v0.9
 * [new tag]         v0.9.2       -> v0.9.2
 * [new tag]         v0.9.1       -> v0.9.1
Maybe it's not about missing refs but have an extra origin/? So this should be a bug of parseRemoteUpdateOutput when handle new commits for a branch. The branch name became origin/main, but it should be refs/heads/main.
There are also instances where origin/ may not even be in the response. As mentioned in my previous reply, there are times where the output is,
    feedaf2..cfd6d4d  main       -> main

Yes, so I mean maybe we should correct the function runSync.

lflare · 2023-04-23T10:29:13Z

Sorry @lunny, I have been out of touch of this issue for awhile as I've been running my own custom build with this, but how do you suppose we can rectify this issue? Is there a drawback to just calling a re-index every time (opts.RefFullName == git.BranchPrefix+repo.DefaultBranch || opts.RefFullName == repo.DefaultBranch) matches? I'm looking through documentations and I'm not entirely sure how to go about getting git to return the correct tags for parsing.

wxiaoguang · 2023-05-10T05:41:14Z

I have no objection for this change.

Leaving enough comments, then it could look good to me.

lflare · 2023-05-10T05:59:35Z

I have no objection for this change.

Leaving enough comments, then it could look good to me.

Great to hear, what kind of comments are you expecting from this PR?

wxiaoguang · 2023-05-10T06:04:18Z

I have no objection for this change.
Leaving enough comments, then it could look good to me.

Great to hear, what kind of comments are you expecting from this PR?

Some explanation about why need this logic? And take some test result from the commens like f74293f9c2..90a7bba1f2 test-branch-1 -> wxiaoguang/test-branch-1 ?

lunny · 2023-05-10T09:44:37Z

I have sent #24634. I think that PR did more work than this PR.

lflare · 2023-05-11T12:41:51Z

I have sent #24634. I think that PR did more work than this PR.

No problem with that, so long as that PR doesn't take a couple of months to review and merge, given how much more complicated it is.

lunny

Since my PR will take more time, I think we can merge this first.

…nc bugs (#24634) This PR replaces all string refName as a type `git.RefName` to make the code more maintainable. Fix #15367 Replaces #23070 It also fixed a bug that tags are not sync because `git remote --prune origin` will not remove local tags if remote removed. We in fact should use `git fetch --prune --tags origin` but not `git remote update origin` to do the sync. Some answer from ChatGPT as ref. > If the git fetch --prune --tags command is not working as expected, there could be a few reasons why. Here are a few things to check: > >Make sure that you have the latest version of Git installed on your system. You can check the version by running git --version in your terminal. If you have an outdated version, try updating Git and see if that resolves the issue. > >Check that your Git repository is properly configured to track the remote repository's tags. You can check this by running git config --get-all remote.origin.fetch and verifying that it includes +refs/tags/*:refs/tags/*. If it does not, you can add it by running git config --add remote.origin.fetch "+refs/tags/*:refs/tags/*". > >Verify that the tags you are trying to prune actually exist on the remote repository. You can do this by running git ls-remote --tags origin to list all the tags on the remote repository. > >Check if any local tags have been created that match the names of tags on the remote repository. If so, these local tags may be preventing the git fetch --prune --tags command from working properly. You can delete local tags using the git tag -d command. --------- Co-authored-by: delvh <[email protected]>

lunny · 2023-05-26T01:36:00Z

Since #24634 is merged but not backport to v1.19 because there are too many conflicts, I think this patch could be sent to v1.19.

silverwind · 2023-05-27T08:22:22Z

Conflict needs to be fixed.

lflare · 2023-05-27T09:28:00Z

Conflict needs to be fixed.

Am I suppose to fix the conflict based off v1.19 code?

silverwind · 2023-05-27T09:36:18Z

Conflict needs to be fixed.

Am I suppose to fix the conflict based off v1.19 code?

No, just fix it against the main branch. 1.19 would be handled with a separate backport pull request.

wxiaoguang · 2023-05-27T09:52:10Z

Conflict needs to be fixed.

Am I suppose to fix the conflict based off v1.19 code?

No, just fix it against the main branch. 1.19 would be handled with a separate backport pull request.

The original issue's problem has been fixed by Use the type RefName for all the needed places and fix pull mirror sync bugs #24634 in 1.20

So this PR could be applied to 1.19

silverwind · 2023-05-27T10:08:47Z

Ok, let me try changing PR base.

silverwind · 2023-05-27T10:09:56Z

No, didn't work, it showed a massive diff. I think we may need a fresh PR targeting release/v1.19.

lflare · 2023-05-27T12:17:17Z

@silverwind, does this look good for merging?

silverwind · 2023-05-27T18:33:23Z

Seems good, but @lunny should take a final look as well.

dboreham · 2023-05-29T16:26:51Z

fwiw I tested release/v1.19 just now and it does not seem to fix #24824
(#24634 in main does fix it)

wxiaoguang · 2023-05-29T16:28:15Z

fwiw I tested release/v1.19 just now and it does not seem to fix #24824 (#24634 in main does fix it)

This PR is for stats_indexer.UpdateRepoIndexer only, no fix for action.

That's why the #24634 is the complete fix.

dboreham · 2023-05-29T16:30:57Z

That's why the #24634 is the complete fix.

Oh, that's too bad.

wxiaoguang · 2023-05-29T16:32:39Z

That's why the #24634 is the complete fix.

Oh, that's too bad.

That refactoring PR is too big, it's not suitable to backport.

I think 1.20 is pretty stable, and its release day is not too far.

Maybe you could try 1.20 nightly, any bug could be fixed in the first time.

lunny · 2023-05-29T16:38:04Z

Maybe #24868 could be reopen and retarget to 1.19? @sillyguodong

dboreham · 2023-05-29T16:38:19Z

That refactoring PR is too big, it's not suitable to backport.

I was thinking of this one-line fix: #24868
Noticed that there is a comment on that PR saying it was closed in preference to this one, but in fact it doesn't fix the same problem.

I think 1.20 is pretty stable, and its release day is not too far.

Maybe you could try 1.20 nightly, any bug could be fixed in the first time.

We run 1.20 in dev, but for production I have to deploy something stable :(

sillyguodong · 2023-05-30T02:16:21Z

@dboreham create a new PR #24994, try to merge into v1.19

It seems that `opts.RefFullName` may occassionally be set to just the branch name, without the `refs/heads/` prefixing. (cherry picked from commit 7dc46ff)

lflare changed the title ~~Allow for short-hand references in Sync Pushes.~~ Allow for matching short-hand references in sync pulls Feb 22, 2023

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 22, 2023

pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 10, 2023

pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 10, 2023

wxiaoguang approved these changes May 10, 2023

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 May 10, 2023

lunny mentioned this pull request May 10, 2023

Use the type RefName for all the needed places and fix pull mirror sync bugs #24634

Merged

wxiaoguang mentioned this pull request May 23, 2023

Fix missing reference prefix of commits when sync mirror repository #24868

Closed

lunny approved these changes May 23, 2023

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 May 23, 2023

wxiaoguang mentioned this pull request May 25, 2023

Add branch prefix to mirror synced push results #24927

Closed

silverwind changed the base branch from main to release/v1.19 May 27, 2023 10:09

silverwind changed the base branch from release/v1.19 to main May 27, 2023 10:09

lflare changed the base branch from main to release/v1.19 May 27, 2023 11:38

Attempt to resolve #15367

d0f9c8a

wxiaoguang added this to the 1.19.4 milestone May 27, 2023

wxiaoguang added the type/bug label May 27, 2023

lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 29, 2023

Merge branch 'release/v1.19' into fix/15367-mirror-indexer-update

2d15ea4

delvh changed the title ~~Allow for matching short-hand references in sync pulls~~ Match unqualified references when syncing pulls as well May 29, 2023

delvh merged commit 7dc46ff into go-gitea:release/v1.19 May 29, 2023

GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 29, 2023

dboreham mentioned this pull request May 29, 2023

Gitea broke CI/Actions for mirrored repos cerc-io/hosting#40

Closed

go-gitea locked as resolved and limited conversation to collaborators Aug 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Match unqualified references when syncing pulls as well #23070

Match unqualified references when syncing pulls as well #23070

lflare commented Feb 22, 2023 •

edited by delvh

Loading

wxiaoguang commented Feb 22, 2023

lflare commented Feb 22, 2023 •

edited

Loading

wxiaoguang commented Feb 22, 2023 •

edited

Loading

lflare commented Feb 22, 2023

wxiaoguang commented Feb 22, 2023

lunny commented Feb 24, 2023 •

edited

Loading

lflare commented Feb 25, 2023

lunny commented Mar 2, 2023

lflare commented Apr 23, 2023

wxiaoguang commented May 10, 2023

lflare commented May 10, 2023

wxiaoguang commented May 10, 2023

lunny commented May 10, 2023

lflare commented May 11, 2023

lunny left a comment

lunny commented May 26, 2023

silverwind commented May 27, 2023

lflare commented May 27, 2023

silverwind commented May 27, 2023

wxiaoguang commented May 27, 2023

silverwind commented May 27, 2023

silverwind commented May 27, 2023 •

edited

Loading

lflare commented May 27, 2023

silverwind commented May 27, 2023

dboreham commented May 29, 2023

wxiaoguang commented May 29, 2023

dboreham commented May 29, 2023

wxiaoguang commented May 29, 2023 •

edited

Loading

lunny commented May 29, 2023

dboreham commented May 29, 2023

sillyguodong commented May 30, 2023

Match unqualified references when syncing pulls as well #23070

Match unqualified references when syncing pulls as well #23070

Conversation

lflare commented Feb 22, 2023 • edited by delvh Loading

wxiaoguang commented Feb 22, 2023

lflare commented Feb 22, 2023 • edited Loading

wxiaoguang commented Feb 22, 2023 • edited Loading

lflare commented Feb 22, 2023

wxiaoguang commented Feb 22, 2023

lunny commented Feb 24, 2023 • edited Loading

lflare commented Feb 25, 2023

lunny commented Mar 2, 2023

lflare commented Apr 23, 2023

wxiaoguang commented May 10, 2023

lflare commented May 10, 2023

wxiaoguang commented May 10, 2023

lunny commented May 10, 2023

lflare commented May 11, 2023

lunny left a comment

Choose a reason for hiding this comment

lunny commented May 26, 2023

silverwind commented May 27, 2023

lflare commented May 27, 2023

silverwind commented May 27, 2023

wxiaoguang commented May 27, 2023

silverwind commented May 27, 2023

silverwind commented May 27, 2023 • edited Loading

lflare commented May 27, 2023

silverwind commented May 27, 2023

dboreham commented May 29, 2023

wxiaoguang commented May 29, 2023

dboreham commented May 29, 2023

wxiaoguang commented May 29, 2023 • edited Loading

lunny commented May 29, 2023

dboreham commented May 29, 2023

sillyguodong commented May 30, 2023

lflare commented Feb 22, 2023 •

edited by delvh

Loading

lflare commented Feb 22, 2023 •

edited

Loading

wxiaoguang commented Feb 22, 2023 •

edited

Loading

lunny commented Feb 24, 2023 •

edited

Loading

silverwind commented May 27, 2023 •

edited

Loading

wxiaoguang commented May 29, 2023 •

edited

Loading