Skip to content

Fix some migration and repo name problems #33986

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

Merged
merged 2 commits into from
Mar 25, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions models/user/user_system.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ import (
)

const (
GhostUserID = -1
GhostUserName = "Ghost"
GhostUserID int64 = -1
GhostUserName = "Ghost"
)

// NewGhostUser creates and returns a fake user for someone has deleted their account.
Expand All @@ -36,9 +36,9 @@ func (u *User) IsGhost() bool {
}

const (
ActionsUserID = -2
ActionsUserName = "gitea-actions"
ActionsUserEmail = "[email protected]"
ActionsUserID int64 = -2
ActionsUserName = "gitea-actions"
ActionsUserEmail = "[email protected]"
)

func IsGiteaActionsUserName(name string) bool {
Expand Down
6 changes: 6 additions & 0 deletions modules/json/json.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,12 @@ func Valid(data []byte) bool {
// UnmarshalHandleDoubleEncode - due to a bug in xorm (see https://gitea.com/xorm/xorm/pulls/1957) - it's
// possible that a Blob may be double encoded or gain an unwanted prefix of 0xff 0xfe.
func UnmarshalHandleDoubleEncode(bs []byte, v any) error {
if len(bs) == 0 {
// json.Unmarshal will report errors if input is empty (nil or zero-length)
// It seems that XORM ignores the nil but still passes zero-length string into this function
// To be consistent, we should treat all empty inputs as success
return nil
}
err := json.Unmarshal(bs, v)
if err != nil {
ok := true
Expand Down
18 changes: 18 additions & 0 deletions modules/json/json_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Copyright 2025 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT

package json

import (
"testing"

"github.com./stretchr/testify/assert"
)

func TestGiteaDBJSONUnmarshal(t *testing.T) {
var m map[any]any
err := UnmarshalHandleDoubleEncode(nil, &m)
assert.NoError(t, err)
err = UnmarshalHandleDoubleEncode([]byte(""), &m)
assert.NoError(t, err)
}
2 changes: 1 addition & 1 deletion services/auth/oauth2_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ func TestUserIDFromToken(t *testing.T) {

o := OAuth2{}
uid := o.userIDFromToken(t.Context(), token, ds)
assert.Equal(t, int64(user_model.ActionsUserID), uid)
assert.Equal(t, user_model.ActionsUserID, uid)
assert.Equal(t, true, ds["IsActionsToken"])
assert.Equal(t, ds["ActionsTaskID"], int64(RunningTaskID))
})
Expand Down
6 changes: 0 additions & 6 deletions services/doctor/fix16961_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,6 @@ func Test_fixUnitConfig_16961(t *testing.T) {
wantFixed bool
wantErr bool
}{
{
name: "empty",
bs: "",
wantFixed: true,
wantErr: false,
},
{
name: "normal: {}",
bs: "{}",
Expand Down
9 changes: 7 additions & 2 deletions services/migrations/gitlab.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"time"

issues_model "code.gitea.io/gitea/models/issues"
"code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/container"
"code.gitea.io/gitea/modules/log"
base "code.gitea.io/gitea/modules/migration"
Expand Down Expand Up @@ -535,11 +536,15 @@ func (g *GitlabDownloader) GetComments(ctx context.Context, commentable base.Com
}

for _, stateEvent := range stateEvents {
posterUserID, posterUsername := user.GhostUserID, user.GhostUserName
if stateEvent.User != nil {
posterUserID, posterUsername = int64(stateEvent.User.ID), stateEvent.User.Username
}
comment := &base.Comment{
IssueIndex: commentable.GetLocalIndex(),
Index: int64(stateEvent.ID),
PosterID: int64(stateEvent.User.ID),
PosterName: stateEvent.User.Username,
PosterID: posterUserID,
PosterName: posterUsername,
Content: "",
Created: *stateEvent.CreatedAt,
}
Expand Down
14 changes: 14 additions & 0 deletions services/repository/migrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"code.gitea.io/gitea/models/db"
"code.gitea.io/gitea/models/organization"
repo_model "code.gitea.io/gitea/models/repo"
unit_model "code.gitea.io/gitea/models/unit"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/git"
"code.gitea.io/gitea/modules/gitrepo"
Expand Down Expand Up @@ -246,6 +247,19 @@ func MigrateRepositoryGitData(ctx context.Context, u *user_model.User,
}
}

var enableRepoUnits []repo_model.RepoUnit
if opts.Releases && !unit_model.TypeReleases.UnitGlobalDisabled() {
enableRepoUnits = append(enableRepoUnits, repo_model.RepoUnit{RepoID: repo.ID, Type: unit_model.TypeReleases})
}
if opts.Wiki && !unit_model.TypeWiki.UnitGlobalDisabled() {
enableRepoUnits = append(enableRepoUnits, repo_model.RepoUnit{RepoID: repo.ID, Type: unit_model.TypeWiki})
}
if len(enableRepoUnits) > 0 {
err = UpdateRepositoryUnits(ctx, repo, enableRepoUnits, nil)
if err != nil {
return nil, err
}
}
return repo, committer.Commit()
}

Expand Down
30 changes: 21 additions & 9 deletions tests/integration/mirror_pull_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@
package integration

import (
"slices"
"testing"

"code.gitea.io/gitea/models/db"
repo_model "code.gitea.io/gitea/models/repo"
"code.gitea.io/gitea/models/unit"
"code.gitea.io/gitea/models/unittest"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/git"
Expand All @@ -19,11 +21,13 @@ import (
"code.gitea.io/gitea/tests"

"github.com./stretchr/testify/assert"
"github.com./stretchr/testify/require"
)

func TestMirrorPull(t *testing.T) {
defer tests.PrepareTestEnv(t)()

ctx := t.Context()
user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 1})
repoPath := repo_model.RepoPath(user.Name, repo.Name)
Expand All @@ -35,10 +39,10 @@ func TestMirrorPull(t *testing.T) {
Mirror: true,
CloneAddr: repoPath,
Wiki: true,
Releases: false,
Releases: true,
}

mirrorRepo, err := repo_service.CreateRepositoryDirectly(db.DefaultContext, user, user, repo_service.CreateRepoOptions{
mirrorRepo, err := repo_service.CreateRepositoryDirectly(ctx, user, user, repo_service.CreateRepoOptions{
Name: opts.RepoName,
Description: opts.Description,
IsPrivate: opts.Private,
Expand All @@ -48,22 +52,27 @@ func TestMirrorPull(t *testing.T) {
assert.NoError(t, err)
assert.True(t, mirrorRepo.IsMirror, "expected pull-mirror repo to be marked as a mirror immediately after its creation")

ctx := t.Context()

mirror, err := repo_service.MigrateRepositoryGitData(ctx, user, mirrorRepo, opts, nil)
mirrorRepo, err = repo_service.MigrateRepositoryGitData(ctx, user, mirrorRepo, opts, nil)
assert.NoError(t, err)

// these units should have been enabled
mirrorRepo.Units = nil
require.NoError(t, mirrorRepo.LoadUnits(ctx))
assert.True(t, slices.ContainsFunc(mirrorRepo.Units, func(u *repo_model.RepoUnit) bool { return u.Type == unit.TypeReleases }))
assert.True(t, slices.ContainsFunc(mirrorRepo.Units, func(u *repo_model.RepoUnit) bool { return u.Type == unit.TypeWiki }))

gitRepo, err := gitrepo.OpenRepository(git.DefaultContext, repo)
assert.NoError(t, err)
defer gitRepo.Close()

findOptions := repo_model.FindReleasesOptions{
IncludeDrafts: true,
IncludeTags: true,
RepoID: mirror.ID,
RepoID: mirrorRepo.ID,
}
initCount, err := db.Count[repo_model.Release](db.DefaultContext, findOptions)
assert.NoError(t, err)
assert.Zero(t, initCount) // no sync yet, so even though there is a tag in source repo, the mirror's release table is still empty

assert.NoError(t, release_service.CreateRelease(gitRepo, &repo_model.Release{
RepoID: repo.ID,
Expand All @@ -79,12 +88,15 @@ func TestMirrorPull(t *testing.T) {
IsTag: true,
}, nil, ""))

_, err = repo_model.GetMirrorByRepoID(ctx, mirror.ID)
_, err = repo_model.GetMirrorByRepoID(ctx, mirrorRepo.ID)
assert.NoError(t, err)

ok := mirror_service.SyncPullMirror(ctx, mirror.ID)
ok := mirror_service.SyncPullMirror(ctx, mirrorRepo.ID)
assert.True(t, ok)

// actually there is a tag in the source repo, so after "sync", that tag will also come into the mirror
initCount++

count, err := db.Count[repo_model.Release](db.DefaultContext, findOptions)
assert.NoError(t, err)
assert.EqualValues(t, initCount+1, count)
Expand All @@ -93,7 +105,7 @@ func TestMirrorPull(t *testing.T) {
assert.NoError(t, err)
assert.NoError(t, release_service.DeleteReleaseByID(ctx, repo, release, user, true))

ok = mirror_service.SyncPullMirror(ctx, mirror.ID)
ok = mirror_service.SyncPullMirror(ctx, mirrorRepo.ID)
assert.True(t, ok)

count, err = db.Count[repo_model.Release](db.DefaultContext, findOptions)
Expand Down
17 changes: 16 additions & 1 deletion web_src/js/features/repo-common.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,22 @@
import {substituteRepoOpenWithUrl} from './repo-common.ts';
import {sanitizeRepoName, substituteRepoOpenWithUrl} from './repo-common.ts';

test('substituteRepoOpenWithUrl', () => {
// For example: "x-github-client://openRepo/https://github.com./go-gitea/gitea"
expect(substituteRepoOpenWithUrl('proto://a/{url}', 'https://gitea')).toEqual('proto://a/https://gitea');
expect(substituteRepoOpenWithUrl('proto://a?link={url}', 'https://gitea')).toEqual('proto://a?link=https%3A%2F%2Fgitea');
});

test('sanitizeRepoName', () => {
expect(sanitizeRepoName(' a b ')).toEqual('a-b');
expect(sanitizeRepoName('a-b_c.git ')).toEqual('a-b_c');
expect(sanitizeRepoName('/x.git/')).toEqual('-x.git-');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For cases where the repo name starts or ends with '/', I think it's better to just remove it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. Actually in my mind we don't need to make it more complex than it should be.

The sanitizeRepoName only needs to make the name overall good, but doesn't need to do too much for end users. For example: there are far more edge cases: \hello\world, @asdf, etc

expect(sanitizeRepoName('.profile')).toEqual('.profile');
expect(sanitizeRepoName('.profile.')).toEqual('.profile');
expect(sanitizeRepoName('.pro..file')).toEqual('.pro.file');

expect(sanitizeRepoName('foo.rss.atom.git.wiki')).toEqual('foo');

expect(sanitizeRepoName('.')).toEqual('');
expect(sanitizeRepoName('..')).toEqual('');
expect(sanitizeRepoName('-')).toEqual('');
});
16 changes: 16 additions & 0 deletions web_src/js/features/repo-common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,3 +159,19 @@ export async function updateIssuesMeta(url: string, action: string, issue_ids: s
console.error(error);
}
}

export function sanitizeRepoName(name: string): string {
name = name.trim().replace(/[^-.\w]/g, '-');
for (let lastName = ''; lastName !== name;) {
lastName = name;
name = name.replace(/\.+$/g, '');
name = name.replace(/\.{2,}/g, '.');
for (const ext of ['.git', '.wiki', '.rss', '.atom']) {
if (name.endsWith(ext)) {
name = name.substring(0, name.length - ext.length);
}
}
}
if (['.', '..', '-'].includes(name)) name = '';
return name;
}
21 changes: 14 additions & 7 deletions web_src/js/features/repo-migration.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {hideElem, showElem, toggleElem} from '../utils/dom.ts';
import {sanitizeRepoName} from './repo-common.ts';

const service = document.querySelector<HTMLInputElement>('#service_type');
const user = document.querySelector<HTMLInputElement>('#auth_username');
Expand All @@ -25,13 +26,19 @@ export function initRepoMigration() {
});
lfs?.addEventListener('change', setLFSSettingsVisibility);

const cloneAddr = document.querySelector<HTMLInputElement>('#clone_addr');
cloneAddr?.addEventListener('change', () => {
const repoName = document.querySelector<HTMLInputElement>('#repo_name');
if (cloneAddr.value && !repoName?.value) { // Only modify if repo_name input is blank
repoName.value = /^(.*\/)?((.+?)(\.git)?)$/.exec(cloneAddr.value)[3];
}
});
const elCloneAddr = document.querySelector<HTMLInputElement>('#clone_addr');
const elRepoName = document.querySelector<HTMLInputElement>('#repo_name');
if (elCloneAddr && elRepoName) {
let repoNameChanged = false;
elRepoName.addEventListener('input', () => {repoNameChanged = true});
elCloneAddr.addEventListener('input', () => {
if (repoNameChanged) return;
let repoNameFromUrl = elCloneAddr.value.split(/[?#]/)[0];
repoNameFromUrl = /^(.*\/)?((.+?)\/?)$/.exec(repoNameFromUrl)[3];
repoNameFromUrl = repoNameFromUrl.split(/[?#]/)[0];
elRepoName.value = sanitizeRepoName(repoNameFromUrl);
});
}
}

function checkAuth() {
Expand Down
5 changes: 5 additions & 0 deletions web_src/js/features/repo-new.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {hideElem, showElem, toggleElem} from '../utils/dom.ts';
import {htmlEscape} from 'escape-goat';
import {fomanticQuery} from '../modules/fomantic/base.ts';
import {sanitizeRepoName} from './repo-common.ts';

const {appSubUrl} = window.config;

Expand Down Expand Up @@ -74,6 +75,10 @@ export function initRepoNew() {
}
};
inputRepoName.addEventListener('input', updateUiRepoName);
inputRepoName.addEventListener('change', () => {
inputRepoName.value = sanitizeRepoName(inputRepoName.value);
updateUiRepoName();
});
updateUiRepoName();

initRepoNewTemplateSearch(form);
Expand Down