Add safe pull mirror option to avoid original repository deletion by force-push #19165

jimmy201602 · 2022-03-22T09:09:29Z

add safe pull mirror option to avoid original repository delete

lafriks · 2022-03-22T11:52:47Z

options/locale/locale_zh-CN.ini

 can_write_info=写入
-key_state_desc=7 天内使用过该密钥 
-token_state_desc=7 天内使用过该密钥 
+key_state_desc=7 天内使用过该密钥


please revert changes to translation file

models/repo/mirror.go

lafriks · 2022-03-22T11:54:50Z

Also need to add database migration to add Sync changed struct with database table structure

KN4CK3R · 2022-03-23T19:57:28Z

models/migrations/migrations.go


 	// v211 -> v212
 	NewMigration("Create ForeignReference table", createForeignReferenceTable),
+


Suggested change

6543 · 2022-03-23T20:39:07Z

services/mirror/mirror_pull.go


+	if safeMirror {
+		// detect can safe mirror
+		canUpdate, err := detectCanUpdateMirror(ctx, m, gitArgs)


this will be resource intense on huge repos .. e.g. Linux kernel

git binary do not provide a option to filter the update ... :/

Gusted · 2022-03-23T20:22:44Z

options/locale/locale_en-US.ini

 mirror_password_blank_placeholder = (Unset)
 mirror_password_help = Change the username to erase a stored password.
+mirror_enable_safe = Safe Mirror
+mirror_enable_safe_desc = Enable safe mirror to avoid delete local repository data


Suggested change

mirror_enable_safe_desc = Enable safe mirror to avoid delete local repository data

mirror_enable_safe_desc = Enable safe mirroring to avoid deleting data from the local repository

Gusted · 2022-03-23T20:26:20Z

routers/web/repo/setting.go

+	mirrors, err := repo_model.GetMirrorByRepoID(ctx.Repo.Repository.ID)
+	if err != nil {
+		ctx.Data["EnableSafeMirror"] = false
+	} else {
+		ctx.Data["EnableSafeMirror"] = mirrors.EnableSafeMirror
+	}


Suggested change

mirrors, err := repo_model.GetMirrorByRepoID(ctx.Repo.Repository.ID)

if err != nil {

ctx.Data["EnableSafeMirror"] = false

} else {

ctx.Data["EnableSafeMirror"] = mirrors.EnableSafeMirror

}

mirror, err := repo_model.GetMirrorByRepoID(ctx.Repo.Repository.ID)

if err != nil {

ctx.Data["EnableSafeMirror"] = false

} else {

ctx.Data["EnableSafeMirror"] = mirror.EnableSafeMirror

}

I think that we should check if err is ErrMirrorNotExist and allow such error(without returning a 404), otherwise log the error and render a 404 page, because it would then be a database error.

Suggested change

mirrors, err := repo_model.GetMirrorByRepoID(ctx.Repo.Repository.ID)

if err != nil {

ctx.Data["EnableSafeMirror"] = false

} else {

ctx.Data["EnableSafeMirror"] = mirrors.EnableSafeMirror

}

mirror, err := repo_model.GetMirrorByRepoID(ctx.Repo.Repository.ID)

ctx.Data["EnableSafeMirror"] = err == nil && mirror.EnableSafeMirror

Gusted · 2022-03-23T20:33:13Z

services/mirror/mirror_pull.go

 func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bool) {
 	repoPath := m.Repo.RepoPath()
 	wikiPath := m.Repo.WikiPath()
+	safeMirror := m.EnableSafeMirror


Suggested change

safeMirror := m.EnableSafeMirror

Gusted · 2022-03-23T20:33:20Z

services/mirror/mirror_pull.go

 		log.Error("SyncMirrors [repo: %-v]: GetRemoteAddress Error %v", m.Repo, remoteErr)
 	}

+	if safeMirror {


Suggested change

if safeMirror {

if m.EnableSafeMirror {

Gusted · 2022-03-23T20:34:02Z

services/mirror/mirror_pull.go

+		canUpdate, err := detectCanUpdateMirror(ctx, m, gitArgs)
+		newRepoPath := fmt.Sprintf("%s_update", repoPath)
+		// delete the temp directory
+		errDelete := util.RemoveAll(newRepoPath)
+		if errDelete != nil {
+			log.Error("DeleteRepositoryTempDirectoryError: %v", errDelete)
+		}
+		if err != nil {
+			log.Error("CheckRepositoryCanSafeMirrorError: %v", err)
+		}


Why is the error checked later here? In 100% cases you want to check the error immediately.

Also, I'd say that every single method call inside these error logs is wrong.

Gusted · 2022-03-23T20:46:27Z

services/mirror/mirror_pull_safe_check.go

+		desc := fmt.Sprintf("Failed to get mirror repository can update '%s': %s", newRepoPath, stderrMessage)
+		log.Error("GetMirrorCanUpdate [repo: %-v]: failed to get mirror repository can update:\nStdout: %s\nStderr: %s\nErr: %v", m.Repo, stdoutMessage, stderrMessage, err)


Suggested change

desc := fmt.Sprintf("Failed to get mirror repository can update '%s': %s", newRepoPath, stderrMessage)

log.Error("GetMirrorCanUpdate [repo: %-v]: failed to get mirror repository can update:\nStdout: %s\nStderr: %s\nErr: %v", m.Repo, stdoutMessage, stderrMessage, err)

log.Error("GetMirrorCanUpdate [repo: %-v]: failed to get mirror repository can update:\nStdout: %s\nStderr: %s\nErr: %v", m.Repo, stdoutMessage, stderrMessage, err)

desc := fmt.Sprintf("Failed to get mirror repository can update '%s': %s", newRepoPath, stderrMessage)

More logical to re-order this.

Suggested change

desc := fmt.Sprintf("Failed to get mirror repository can update '%s': %s", newRepoPath, stderrMessage)

log.Error("GetMirrorCanUpdate [repo: %-v]: failed to get mirror repository can update:\nStdout: %s\nStderr: %s\nErr: %v", m.Repo, stdoutMessage, stderrMessage, err)

log.Error("getGitCommandStdoutStderr [repo: %-v]: failed to check if mirror can be updated:\nStdout: %s\nStderr: %s\nErr: %v", m.Repo, stdoutMessage, stderrMessage, err)

desc := fmt.Sprintf("Failed to check if mirror '%s' can be updated: %s", newRepoPath, stderrMessage)

Gusted · 2022-03-23T20:47:16Z

services/mirror/mirror_pull_safe_check.go

+		log.Error("GetMirrorCanUpdate [repo: %-v]: failed to get mirror repository can update:\nStdout: %s\nStderr: %s\nErr: %v", m.Repo, stdoutMessage, stderrMessage, err)
+		if err = admin_model.CreateRepositoryNotice(desc); err != nil {
+			log.Error("GetMirrorCanUpdateNotice: %v", err)
+		}


We need to return here with a error.

Gusted · 2022-03-23T20:47:44Z

services/mirror/mirror_pull_safe_check.go

+	remoteAddr, remoteErr := git.GetRemoteAddress(ctx, newRepoPath, m.GetRemoteName())
+	if remoteErr != nil {
+		log.Error("GetMirrorCanUpdate [repo: %-v]: GetRemoteAddress Error %v", m.Repo, remoteErr)


Suggested change

remoteAddr, remoteErr := git.GetRemoteAddress(ctx, newRepoPath, m.GetRemoteName())

if remoteErr != nil {

log.Error("GetMirrorCanUpdate [repo: %-v]: GetRemoteAddress Error %v", m.Repo, remoteErr)

remoteAddr, err := git.GetRemoteAddress(ctx, newRepoPath, m.GetRemoteName())

if err != nil {

log.Error("GetMirrorCanUpdate [repo: %-v]: GetRemoteAddress Error %v", m.Repo, err)

Gusted · 2022-03-23T20:50:38Z

services/mirror/mirror_pull_safe_check.go

+}
+
+// detect user can update the mirror
+func detectCanUpdateMirror(ctx context.Context, m *repo_model.Mirror, gitArgs []string) (bool, error) {


This function needs some more comments in order to not have a 10 minutes hard time understanding how this function is achieving it's "detection method".

Gusted · 2022-03-23T20:52:34Z

services/mirror/mirror_pull_safe_check.go

+	if repoCommitCount > newRepoCommitCount {
+		return false, nil
+	} else if repoCommitCount == newRepoCommitCount {
+		// noting to happen
+		return true, nil
+	} else {
+		// compare commit id
+		skipcout := newRepoCommitCount - repoCommitCount
+		gitNewRepoLastCommitIDArgs := []string{"log", "-1", fmt.Sprintf("--skip=%d", skipcout), "--format='%H'"}
+		stdoutNewRepoCommitID, _, err := getGitCommandStdoutStderr(ctx, m, gitNewRepoLastCommitIDArgs, newRepoPath)
+		if err != nil {
+			return false, err
+		}
+		gitRepoLastCommitIDArgs := []string{"log", "--format='%H'", "-n", "1"}
+		stdoutRepoCommitID, _, err := getGitCommandStdoutStderr(ctx, m, gitRepoLastCommitIDArgs, repoPath)
+		if err != nil {
+			return false, err
+		}
+		if stdoutNewRepoCommitID != stdoutRepoCommitID {
+			return false, fmt.Errorf("Old repo commit id: %s not match new repo id: %s", stdoutRepoCommitID, stdoutNewRepoCommitID)
+		}
+	}
+	return true, nil


Just for the sake of understanding here, it's more or less trying to detect if a force-push has happen on the upstream's repo?

It is the purpose. However, I think copying the repo again and again is not ideal.

A better solution could be: track all branches locally, fetch remote, compare the merge-base, if the merge-base is not the locally tracked one, then there is a force-push.

And there might be other better solutions, but copying the repo is not the proper way.

yep, I'm also looking for an ideal solution to figure out this sitution.

Did you seem my option with the reference-transaction hook ? #14076 (comment)
You'll also need an option to allow/disallow force-push on specific branches otherwise mirrors will break quickly (people force-push on PR branches)

delvh · 2022-03-23T20:21:30Z

models/migrations/v222.go

+	"xorm.io/xorm"
+)
+
+func addColorColToMirror(x *xorm.Engine) error {


I think that method name is not correct anymore?

delvh · 2022-03-23T20:51:06Z

options/locale/locale_en-US.ini

 mirror_password_blank_placeholder = (Unset)
 mirror_password_help = Change the username to erase a stored password.
+mirror_enable_safe = Safe Mirror
+mirror_enable_safe_desc = Enable safe mirror to avoid delete local repository data


Suggested change

mirror_enable_safe_desc = Enable safe mirror to avoid delete local repository data

mirror_enable_safe_desc = Enable safe mirroring to avoid deleting local data such as branches or tags once they are deleted remotely

delvh · 2022-03-23T20:54:08Z

models/migrations/migrations.go

 	NewMigration("Create ForeignReference table", createForeignReferenceTable),
+
+	// v212 -> v213
+	NewMigration("Add new safe mirror option", addColorColToMirror),


Suggested change

NewMigration("Add new safe mirror option", addColorColToMirror),

NewMigration("Add safe pull mirrors", addColorColToMirror),

delvh · 2022-03-23T20:58:00Z

services/mirror/mirror_pull_safe_check.go

+	stdoutBuilder := strings.Builder{}
+	stderrBuilder := strings.Builder{}


According to https://github.com/go-gitea/gitea/pull/19165/files#r833702091 I'd say large.

delvh · 2022-03-23T20:59:59Z

routers/web/repo/setting.go

+	mirrors, err := repo_model.GetMirrorByRepoID(ctx.Repo.Repository.ID)
+	if err != nil {
+		ctx.Data["EnableSafeMirror"] = false
+	} else {
+		ctx.Data["EnableSafeMirror"] = mirrors.EnableSafeMirror
+	}


Suggested change

mirrors, err := repo_model.GetMirrorByRepoID(ctx.Repo.Repository.ID)

if err != nil {

ctx.Data["EnableSafeMirror"] = false

} else {

ctx.Data["EnableSafeMirror"] = mirrors.EnableSafeMirror

}

mirror, err := repo_model.GetMirrorByRepoID(ctx.Repo.Repository.ID)

ctx.Data["EnableSafeMirror"] = err == nil && mirror.EnableSafeMirror

delvh · 2022-03-23T21:01:41Z

services/mirror/mirror_pull.go

+		}
+		// can not safe mirror
+		if !canUpdate {
+			log.Error("CheckSyncMirrors [repo: %-v]: can not safe mirror...", m.Repo)


Suggested change

log.Error("CheckSyncMirrors [repo: %-v]: can not safe mirror...", m.Repo)

log.Error("CheckSyncMirrors [repo: %-v]: cannot sync safe mirror...", m.Repo)

delvh · 2022-03-23T21:10:10Z

services/mirror/mirror_pull.go

+		newRepoPath := fmt.Sprintf("%s_update", repoPath)
+		// delete the temp directory
+		errDelete := util.RemoveAll(newRepoPath)


What temp directory?
As that wasn't present previously, I'd assume that it is constructed within detectCanUpdateMirror.
In that case I'd recommend a defer inside the method instead.

delvh · 2022-03-23T21:11:39Z

services/mirror/mirror_pull_safe_check.go

+)
+
+// get git command running stdout and stderr
+func getGitCommandStdoutStderr(ctx context.Context, m *repo_model.Mirror, gitArgs []string, newRepoPath string) (string, string, error) {


And also a better comment, please.
The comment does not help to understand what this function is doing.
Also, I just noticed that the comment does not start with the obligatory method name
(// getGitCommandStdoutStderr ...)

delvh · 2022-03-23T21:15:33Z

services/mirror/mirror_pull_safe_check.go

+		desc := fmt.Sprintf("Failed to get mirror repository can update '%s': %s", newRepoPath, stderrMessage)
+		log.Error("GetMirrorCanUpdate [repo: %-v]: failed to get mirror repository can update:\nStdout: %s\nStderr: %s\nErr: %v", m.Repo, stdoutMessage, stderrMessage, err)
+		if err = admin_model.CreateRepositoryNotice(desc); err != nil {
+			log.Error("GetMirrorCanUpdateNotice: %v", err)


Suggested change

log.Error("GetMirrorCanUpdateNotice: %v", err)

log.Error("CreateRepositoryNotice: %v", err)

delvh · 2022-03-23T21:18:59Z

services/mirror/mirror_pull_safe_check.go

+		desc := fmt.Sprintf("Failed to get mirror repository can update '%s': %s", newRepoPath, stderrMessage)
+		log.Error("GetMirrorCanUpdate [repo: %-v]: failed to get mirror repository can update:\nStdout: %s\nStderr: %s\nErr: %v", m.Repo, stdoutMessage, stderrMessage, err)


Suggested change

desc := fmt.Sprintf("Failed to get mirror repository can update '%s': %s", newRepoPath, stderrMessage)

log.Error("GetMirrorCanUpdate [repo: %-v]: failed to get mirror repository can update:\nStdout: %s\nStderr: %s\nErr: %v", m.Repo, stdoutMessage, stderrMessage, err)

log.Error("getGitCommandStdoutStderr [repo: %-v]: failed to check if mirror can be updated:\nStdout: %s\nStderr: %s\nErr: %v", m.Repo, stdoutMessage, stderrMessage, err)

desc := fmt.Sprintf("Failed to check if mirror '%s' can be updated: %s", newRepoPath, stderrMessage)

jimmy201602 · 2022-03-24T12:17:34Z

I will fix these errors later.

lunny · 2022-06-03T04:15:46Z

Please resolve the conflicts.

kotovalexarian · 2022-08-18T18:30:19Z

What is the state of this feature?

I think that this would be better as part of branch protection, not as a setting of the whole repo. It is not very useful for all branches (such as feature branches).

wxiaoguang · 2023-04-24T13:35:22Z

It seems that this PR got stale after first round review. And the approach has some problems, eg: err := util.CopyDir(repoPath, newRepoPath), copying large repositories again and again would cause performance problems.

wxiaoguang · 2023-05-01T03:58:40Z

It has been stale for a long time and I can't think of a way to handling it other than closing it. Feel free to reopen if there's any new progress and I could also help.

jimmy201602 added 2 commits March 22, 2022 16:23

add safe mirror option

5c01fc0

fix confilict

571ef30

lunny added the type/enhancement An improvement of existing functionality label Mar 22, 2022

lunny added this to the 1.17.0 milestone Mar 22, 2022

remove translation and chinese comment

4bdb918

lafriks requested changes Mar 22, 2022

View reviewed changes

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

jimmy201602 added 22 commits March 22, 2022 20:10

add migrate for safe mirror function

1dc838e

add model migration file

a9cd187

remove chinese comment

a8066a5

replace fmt errorf

d46a5de

format the migration code

93c5f50

remove unclear statement code

724030a

remove some unnecessary code

ed8b20f

remove some unnecessary code

f602aab

lint the code

f02d5b3

lint the code

44e4912

lint the code

9b40dee

lint the code

5e16d8a

lint the code

aaaac62

abstract the code to function

a7eb9cf

abstract the code to function

6801ef1

format the code

e324609

format the code

fff683b

abstract the code to function

061b46a

abstract the code to function

e3b10d2

lint the code

ad9a062

lint the code

8b7479a

fix the lint error

b703f0a

jimmy201602 added 3 commits March 23, 2022 19:41

reorganize the code

a1cb58f

fix push mirror safe option bug

e68e5c6

fix push mirror safe option bug

0ec287c

KN4CK3R reviewed Mar 23, 2022

View reviewed changes

6543 reviewed Mar 23, 2022

View reviewed changes

Gusted reviewed Mar 23, 2022

View reviewed changes

delvh reviewed Mar 23, 2022

View reviewed changes

wxiaoguang changed the title ~~add safe pull mirror option to avoid original repository delete~~ Add safe pull mirror option to avoid original repository deletion by force-push Mar 24, 2022

lint the code and rewrite the suggestion for log

8f78236

jimmy201602 added 2 commits June 7, 2022 08:46

Update migrations.go

bf243ee

merge the latest code

0bf7a7c

techknowlogick modified the milestones: 1.17.0, 1.18.0 Jun 8, 2022

kotovalexarian mentioned this pull request Aug 18, 2022

Branch protection for pull mirrors #20849

Closed

lunny modified the milestones: 1.18.0, 1.19.0 Oct 17, 2022

lunny removed this from the 1.19.0 milestone Feb 1, 2023

wxiaoguang added the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label Apr 24, 2023

wxiaoguang closed this May 1, 2023

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


		// v211 -> v212
		NewMigration("Create ForeignReference table", createForeignReferenceTable),

	mirror_enable_safe_desc = Enable safe mirror to avoid delete local repository data
	mirror_enable_safe_desc = Enable safe mirroring to avoid deleting data from the local repository

		desc := fmt.Sprintf("Failed to get mirror repository can update '%s': %s", newRepoPath, stderrMessage)
		log.Error("GetMirrorCanUpdate [repo: %-v]: failed to get mirror repository can update:\nStdout: %s\nStderr: %s\nErr: %v", m.Repo, stdoutMessage, stderrMessage, err)

	NewMigration("Add new safe mirror option", addColorColToMirror),
	NewMigration("Add safe pull mirrors", addColorColToMirror),

		stdoutBuilder := strings.Builder{}
		stderrBuilder := strings.Builder{}

	log.Error("CheckSyncMirrors [repo: %-v]: can not safe mirror...", m.Repo)
	log.Error("CheckSyncMirrors [repo: %-v]: cannot sync safe mirror...", m.Repo)

	log.Error("GetMirrorCanUpdateNotice: %v", err)
	log.Error("CreateRepositoryNotice: %v", err)

Uh oh!

Add safe pull mirror option to avoid original repository deletion by force-push #19165

Add safe pull mirror option to avoid original repository deletion by force-push #19165

Uh oh!

Conversation

jimmy201602 commented Mar 22, 2022 • edited by 6543 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lafriks commented Mar 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

6543 Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gusted Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimmy201602 commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lunny commented Jun 3, 2022

Uh oh!

jimmy201602 commented Mar 22, 2022 •

edited by 6543

Loading

6543 Mar 23, 2022 •

edited

Loading

Gusted Mar 23, 2022 •

edited

Loading

jimmy201602 commented Mar 24, 2022 •

edited

Loading