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:53:40Z

options/locale/locale_zh-CN.ini

@@ -690,8 +690,8 @@ last_used=上次使用在
 no_activity=没有最近活动
 can_read_info=读取
 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-23T20:06:46Z

models/migrations/migrations.go

@@ -378,6 +378,9 @@ var migrations = []Migration{

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


Suggested change

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

services/mirror/mirror_pull.go

@@ -204,6 +205,25 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo
 		log.Error("SyncMirrors [repo: %-v]: GetRemoteAddress Error %v", m.Repo, remoteErr)
 	}

+	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:52:37Z

options/locale/locale_en-US.ini

@@ -869,6 +869,8 @@ mirror_last_synced = Last Synchronized
 mirror_password_placeholder = (Unchanged)
 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:52:37Z

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:52:37Z

services/mirror/mirror_pull.go

@@ -190,6 +190,7 @@ func pruneBrokenReferences(ctx context.Context,
 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:52:37Z

services/mirror/mirror_pull.go

@@ -204,6 +205,25 @@ func runSync(ctx context.Context, m *repo_model.Mirror) ([]*mirrorSyncResult, bo
 		log.Error("SyncMirrors [repo: %-v]: GetRemoteAddress Error %v", m.Repo, remoteErr)
 	}

+	if safeMirror {


Suggested change

if safeMirror {

if m.EnableSafeMirror {

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

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:52:37Z

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:52:37Z

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:52:37Z

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:52:37Z

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:37Z

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-23T22:02:37Z

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-23T22:02:37Z

options/locale/locale_en-US.ini

@@ -869,6 +869,8 @@ mirror_last_synced = Last Synchronized
 mirror_password_placeholder = (Unchanged)
 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-23T22:02:37Z

models/migrations/migrations.go

@@ -378,6 +378,9 @@ var migrations = []Migration{

 	// v211 -> v212
 	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-23T22:02:37Z

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-23T22:02:37Z

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-23T22:02:37Z

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-23T22:02:37Z

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-23T22:02:37Z

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-23T22:02:37Z

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-23T22:02:37Z

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.

jimmy201602 added 2 commits Mar 22, 2022

add safe mirror option

5c01fc0

fix confilict

571ef30

lunny added the kind/enhancement 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 changes

GiteaBot added the lgtm/need 2 label Mar 22, 2022

jimmy201602 added 22 commits Mar 22, 2022

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 Mar 23, 2022

reorganize the code

a1cb58f

fix push mirror safe option bug

e68e5c6

fix push mirror safe option bug

0ec287c

KN4CK3R reviewed Mar 23, 2022

View changes

6543 reviewed Mar 23, 2022

View changes

Gusted reviewed Mar 23, 2022

View changes

delvh reviewed Mar 23, 2022

View 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

Apr	MAY	Jun
	07
2021	2022	2023

go-gitea / gitea Public

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

jimmy201602 commented Mar 22, 2022 •

edited by 6543

lafriks Mar 22, 2022

lafriks commented Mar 22, 2022

KN4CK3R Mar 23, 2022

6543 Mar 23, 2022 •

edited

Gusted Mar 23, 2022

Gusted Mar 23, 2022

Gusted Mar 23, 2022 •

edited

delvh Mar 23, 2022

Gusted Mar 23, 2022

Gusted Mar 23, 2022

Gusted Mar 23, 2022

delvh Mar 23, 2022

Gusted Mar 23, 2022

delvh Mar 23, 2022

Gusted Mar 23, 2022

Gusted Mar 23, 2022

Gusted Mar 23, 2022

Gusted Mar 23, 2022

wxiaoguang Mar 24, 2022

jimmy201602 Mar 25, 2022

42wim Apr 9, 2022

delvh Mar 23, 2022

delvh Mar 23, 2022

delvh Mar 23, 2022

delvh Mar 23, 2022

delvh Mar 23, 2022

delvh Mar 23, 2022

delvh Mar 23, 2022

delvh Mar 23, 2022

delvh Mar 23, 2022

delvh Mar 23, 2022

jimmy201602 commented Mar 24, 2022 •

edited

		@@ -378,6 +378,9 @@ var migrations = []Migration{

		// 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)

go-gitea / gitea Public

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

Are you sure you want to change the base?

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

Conversation

jimmy201602 commented Mar 22, 2022 • edited by 6543

Choose a reason for hiding this comment

lafriks commented Mar 22, 2022

Choose a reason for hiding this comment

6543 Mar 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gusted Mar 23, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmy201602 commented Mar 24, 2022 • edited

jimmy201602 commented Mar 22, 2022 •

edited by 6543

6543 Mar 23, 2022 •

edited

Gusted Mar 23, 2022 •

edited

jimmy201602 commented Mar 24, 2022 •

edited