Skip to content

Commit e32d0a3

Browse files
zeripathzeripath
committed
as per review
Signed-off-by: Andrew Thornton <art27@cantab.net>
1 parent f1de1b2 commit e32d0a3

File tree

13 files changed

+63
-40
lines changed

13 files changed

+63
-40
lines changed

‎modules/migration/release.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,8 @@ type ReleaseAsset struct {
2626

2727
// Release represents a release
2828
type Release struct {
29-
TagName string `yaml:"tag_name"` // SECURITY: This must pass git.IsValidGitReference
30-
TargetCommitish string `yaml:"target_commitish"` // SECURITY: This must pass git.IsValidGitReference
29+
TagName string `yaml:"tag_name"` // SECURITY: This must pass git.IsValidRefPattern
30+
TargetCommitish string `yaml:"target_commitish"` // SECURITY: This must pass git.IsValidRefPattern
3131
Name string
3232
Body string
3333
Draft bool

‎routers/api/v1/repo/commits.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ func GetSingleCommit(ctx *context.APIContext) {
5252
// "$ref": "#/responses/notFound"
5353

5454
sha := ctx.Params(":sha")
55-
if !git.IsValidRefPattern(sha) && !git.IsValidSHAPattern(sha) {
55+
if !git.IsValidRefPattern(sha) {
5656
ctx.Error(http.StatusUnprocessableEntity, "no valid ref or sha", fmt.Sprintf("no valid ref or sha: %s", sha))
5757
return
5858
}

‎routers/api/v1/repo/notes.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ func GetNote(ctx *context.APIContext) {
4646
// "$ref": "#/responses/notFound"
4747

4848
sha := ctx.Params(":sha")
49-
if !git.IsValidRefPattern(sha) && !git.IsValidSHAPattern(sha) {
49+
if !git.IsValidRefPattern(sha) {
5050
ctx.Error(http.StatusUnprocessableEntity, "no valid ref or sha", fmt.Sprintf("no valid ref or sha: %s", sha))
5151
return
5252
}

‎services/migrations/codebase.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,9 +107,24 @@ func NewCodebaseDownloader(ctx context.Context, projectURL *url.URL, project, re
107107
commitMap: make(map[string]string),
108108
}
109109

110+
log.Trace("Create Codebase downloader. BaseURL: %s Project: %s RepoName: %s", baseURL, project, repoName)
110111
return downloader
111112
}
112113

114+
// String implements Stringer
115+
func (g *CodebaseDownloader) String() string {
116+
return fmt.Sprintf("migration from codebase server %s %s/%s", g.baseURL, g.project, g.repoName)
117+
}
118+
119+
// ColorFormat provides a basic color format for a GogsDownloader
120+
func (g *CodebaseDownloader) ColorFormat(s fmt.State) {
121+
if g == nil {
122+
log.ColorFprintf(s, "<nil: CodebaseDownloader>")
123+
return
124+
}
125+
log.ColorFprintf(s, "migration from codebase server %s %s/%s", g.baseURL, g.project, g.repoName)
126+
}
127+
113128
// FormatCloneURL add authentication into remote URLs
114129
func (d *CodebaseDownloader) FormatCloneURL(opts base.MigrateOptions, remoteAddr string) (string, error) {
115130
return opts.CloneAddr, nil

‎services/migrations/common.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ func hasBaseURL(toCheck, baseURL string) bool {
2929
return strings.HasPrefix(toCheck, baseURL)
3030
}
3131

32-
// CheckAndEnsureSafePR will check that a give PR is safe
32+
// CheckAndEnsureSafePR will check that a given PR is safe to download
3333
func CheckAndEnsureSafePR(pr *base.PullRequest, commonCloneBaseURL string, g base.Downloader) bool {
3434
valid := true
3535
// SECURITY: the patchURL must be checked to have the same baseURL as the current to prevent open redirect

‎services/migrations/dump.go

Lines changed: 27 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -388,28 +388,29 @@ func (g *RepositoryDumper) createItems(dir string, itemFiles map[int64]*os.File,
388388
}
389389

390390
for number, items := range itemsMap {
391-
var err error
392-
itemFile := itemFiles[number]
393-
if itemFile == nil {
394-
itemFile, err = os.Create(filepath.Join(dir, fmt.Sprintf("%d.yml", number)))
395-
if err != nil {
396-
return err
397-
}
398-
itemFiles[number] = itemFile
399-
}
400-
401-
encoder := yaml.NewEncoder(itemFile)
402-
if err := encoder.Encode(items); err != nil {
403-
_ = encoder.Close()
391+
if err := g.encodeItems(number, items, dir, itemFiles); err != nil {
404392
return err
405393
}
394+
}
395+
396+
return nil
397+
}
406398

407-
if err := encoder.Close(); err != nil {
399+
func (g *RepositoryDumper) encodeItems(number int64, items []interface{}, dir string, itemFiles map[int64]*os.File) error {
400+
itemFile := itemFiles[number]
401+
if itemFile == nil {
402+
var err error
403+
itemFile, err = os.Create(filepath.Join(dir, fmt.Sprintf("%d.yml", number)))
404+
if err != nil {
408405
return err
409406
}
407+
itemFiles[number] = itemFile
410408
}
411409

412-
return nil
410+
encoder := yaml.NewEncoder(itemFile)
411+
defer encoder.Close()
412+
413+
return encoder.Encode(items)
413414
}
414415

415416
// CreateComments creates comments of issues
@@ -445,7 +446,7 @@ func (g *RepositoryDumper) handlePullRequest(pr *base.PullRequest) error {
445446

446447
// SECURITY: We will assume that the pr.PatchURL has been checked
447448
// pr.PatchURL maybe a local file - but note EnsureSafe should be asserting that this safe
448-
resp, err := http.Get(u)
449+
resp, err := http.Get(u) // TODO: This probably needs to use the downloader as there may be rate limiting issues here
449450
if err != nil {
450451
return err
451452
}
@@ -559,8 +560,12 @@ func (g *RepositoryDumper) handlePullRequest(pr *base.PullRequest) error {
559560
_, _, err = git.NewCommand(g.ctx, "fetch", "--no-tags", "--", remote, fetchArg).RunStdString(&git.RunOpts{Dir: g.gitPath()})
560561
if err != nil {
561562
log.Error("Fetch branch from %s failed: %v", pr.Head.CloneURL, err)
563+
// We need to continue here so that the Head.Ref is reset and we attempt to set the gitref for the PR
564+
// (This last step will likely fail but we should try to do as much as we can.)
565+
} else {
566+
// Cache the localRef as the Head.Ref - if we've failed we can always try again.
567+
g.prHeadCache[pr.Head.CloneURL+":"+pr.Head.Ref] = localRef
562568
}
563-
g.prHeadCache[pr.Head.CloneURL+":"+pr.Head.Ref] = localRef
564569
}
565570

566571
// Set the pr.Head.Ref to the localRef
@@ -575,9 +580,11 @@ func (g *RepositoryDumper) handlePullRequest(pr *base.PullRequest) error {
575580
}
576581
pr.Head.SHA = headSha
577582
}
578-
_, _, err = git.NewCommand(g.ctx, "update-ref", "--no-deref", pr.GetGitRefName(), pr.Head.SHA).RunStdString(&git.RunOpts{Dir: g.gitPath()})
579-
if err != nil {
580-
log.Error("unable to set %s as the local head for PR #%d from %s in %s/%s. Error: %v", pr.Head.SHA, pr.Number, pr.Head.Ref, g.repoOwner, g.repoName, err)
583+
if pr.Head.SHA != "" {
584+
_, _, err = git.NewCommand(g.ctx, "update-ref", "--no-deref", pr.GetGitRefName(), pr.Head.SHA).RunStdString(&git.RunOpts{Dir: g.gitPath()})
585+
if err != nil {
586+
log.Error("unable to set %s as the local head for PR #%d from %s in %s/%s. Error: %v", pr.Head.SHA, pr.Number, pr.Head.Ref, g.repoOwner, g.repoName, err)
587+
}
581588
}
582589

583590
return nil

‎services/migrations/gitbucket.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ func (f *GitBucketDownloaderFactory) New(ctx context.Context, opts base.MigrateO
3939
oldOwner := fields[1]
4040
oldName := strings.TrimSuffix(fields[2], ".git")
4141

42+
log.Trace("Create GitBucket downloader. BaseURL: %s RepoOwner: %s RepoName: %s", baseURL, oldOwner, oldName)
4243
return NewGitBucketDownloader(ctx, baseURL, opts.AuthUsername, opts.AuthPassword, opts.AuthToken, oldOwner, oldName), nil
4344
}
4445

@@ -55,7 +56,7 @@ type GitBucketDownloader struct {
5556

5657
// String implements Stringer
5758
func (g *GitBucketDownloader) String() string {
58-
return fmt.Sprintf("migration from gitbucket server as %s/%s", g.repoOwner, g.repoName)
59+
return fmt.Sprintf("migration from gitbucket server %s %s/%s", g.baseURL, g.repoOwner, g.repoName)
5960
}
6061

6162
// ColorFormat provides a basic color format for a GitBucketDownloader
@@ -64,7 +65,7 @@ func (g *GitBucketDownloader) ColorFormat(s fmt.State) {
6465
log.ColorFprintf(s, "<nil: GitBucketDownloader>")
6566
return
6667
}
67-
log.ColorFprintf(s, "migration from gitbucket server as %s/%s", g.repoOwner, g.repoName)
68+
log.ColorFprintf(s, "migration from gitbucket server %s %s/%s", g.baseURL, g.repoOwner, g.repoName)
6869
}
6970

7071
// NewGitBucketDownloader creates a GitBucket downloader

‎services/migrations/gitea_downloader.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ func (g *GiteaDownloader) SetContext(ctx context.Context) {
132132

133133
// String implements Stringer
134134
func (g *GiteaDownloader) String() string {
135-
return fmt.Sprintf("migration from gitea server as %s/%s", g.repoOwner, g.repoName)
135+
return fmt.Sprintf("migration from gitea server %s %s/%s", g.baseURL, g.repoOwner, g.repoName)
136136
}
137137

138138
// ColorFormat provides a basic color format for a GiteaDownloader
@@ -141,7 +141,7 @@ func (g *GiteaDownloader) ColorFormat(s fmt.State) {
141141
log.ColorFprintf(s, "<nil: GiteaDownloader>")
142142
return
143143
}
144-
log.ColorFprintf(s, "migration from gitea server as %s/%s", g.repoOwner, g.repoName)
144+
log.ColorFprintf(s, "migration from gitea server %s %s/%s", g.baseURL, g.repoOwner, g.repoName)
145145
}
146146

147147
// GetRepoInfo returns a repository information

‎services/migrations/gitea_uploader.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -700,7 +700,7 @@ func (g *GiteaLocalUploader) newPullRequest(pr *base.PullRequest) (*issues_model
700700
log.Error("Cannot determine the merge base for PR #%d in %s/%s. Error: %v", pr.Number, g.repoOwner, g.repoName, err)
701701
}
702702
} else {
703-
log.Error("Cannot determine the merge base for PR #%d in %s/%s. Not enough information", pr.Number, g.repoOwner, g.repoName, err)
703+
log.Error("Cannot determine the merge base for PR #%d in %s/%s. Not enough information", pr.Number, g.repoOwner, g.repoName)
704704
}
705705
}
706706

‎services/migrations/github.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,7 @@ func (f *GithubDownloaderV3Factory) New(ctx context.Context, opts base.MigrateOp
5151
oldOwner := fields[1]
5252
oldName := strings.TrimSuffix(fields[2], ".git")
5353

54-
log.Trace("Create github downloader: %s/%s", oldOwner, oldName)
54+
log.Trace("Create github downloader BaseURL: %s %s/%s", baseURL, oldOwner, oldName)
5555

5656
return NewGithubDownloaderV3(ctx, baseURL, opts.AuthUsername, opts.AuthPassword, opts.AuthToken, oldOwner, oldName), nil
5757
}
@@ -122,7 +122,7 @@ func NewGithubDownloaderV3(ctx context.Context, baseURL, userName, password, tok
122122

123123
// String implements Stringer
124124
func (g *GithubDownloaderV3) String() string {
125-
return fmt.Sprintf("migration from github server as %s/%s", g.repoOwner, g.repoName)
125+
return fmt.Sprintf("migration from github server %s %s/%s", g.baseURL, g.repoOwner, g.repoName)
126126
}
127127

128128
// ColorFormat provides a basic color format for a GithubDownloader
@@ -131,7 +131,7 @@ func (g *GithubDownloaderV3) ColorFormat(s fmt.State) {
131131
log.ColorFprintf(s, "<nil: GithubDownloaderV3>")
132132
return
133133
}
134-
log.ColorFprintf(s, "migration from github server as %s/%s", g.repoOwner, g.repoName)
134+
log.ColorFprintf(s, "migration from github server %s %s/%s", g.baseURL, g.repoOwner, g.repoName)
135135
}
136136

137137
func (g *GithubDownloaderV3) addClient(client *http.Client, baseURL string) {
@@ -370,7 +370,7 @@ func (g *GithubDownloaderV3) convertGithubRelease(rel *github.RepositoryRelease)
370370
resp, err := httpClient.Do(req)
371371
err1 := g.RefreshRate()
372372
if err1 != nil {
373-
log.Error("g.getClient().RateLimits: %s", err1)
373+
log.Error("g.RefreshRate(): %s", err1)
374374
}
375375
if err != nil {
376376
return nil, err

‎services/migrations/gitlab.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@ func NewGitlabDownloader(ctx context.Context, baseURL, repoPath, username, passw
135135

136136
// String implements Stringer
137137
func (g *GitlabDownloader) String() string {
138-
return fmt.Sprintf("migration from gitlab server as [%d]/%s", g.repoID, g.repoName)
138+
return fmt.Sprintf("migration from gitlab server %s [%d]/%s", g.baseURL, g.repoID, g.repoName)
139139
}
140140

141141
// ColorFormat provides a basic color format for a GitlabDownloader
@@ -144,7 +144,7 @@ func (g *GitlabDownloader) ColorFormat(s fmt.State) {
144144
log.ColorFprintf(s, "<nil: GitlabDownloader>")
145145
return
146146
}
147-
log.ColorFprintf(s, "migration from gitlab server as [%d]/%s", g.repoID, g.repoName)
147+
log.ColorFprintf(s, "migration from gitlab server %s [%d]/%s", g.baseURL, g.repoID, g.repoName)
148148
}
149149

150150
// SetContext set context

‎services/migrations/gogs.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ type GogsDownloader struct {
7575

7676
// String implements Stringer
7777
func (g *GogsDownloader) String() string {
78-
return fmt.Sprintf("migration from gogs server as %s/%s", g.repoOwner, g.repoName)
78+
return fmt.Sprintf("migration from gogs server %s %s/%s", g.baseURL, g.repoOwner, g.repoName)
7979
}
8080

8181
// ColorFormat provides a basic color format for a GogsDownloader
@@ -84,7 +84,7 @@ func (g *GogsDownloader) ColorFormat(s fmt.State) {
8484
log.ColorFprintf(s, "<nil: GogsDownloader>")
8585
return
8686
}
87-
log.ColorFprintf(s, "migration from gogs server as %s/%s", g.repoOwner, g.repoName)
87+
log.ColorFprintf(s, "migration from gogs server %s %s/%s", g.baseURL, g.repoOwner, g.repoName)
8888
}
8989

9090
// SetContext set context

‎services/migrations/onedev.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ func NewOneDevDownloader(ctx context.Context, baseURL *url.URL, username, passwo
112112

113113
// String implements Stringer
114114
func (d *OneDevDownloader) String() string {
115-
return fmt.Sprintf("migration from oneDev server as [%d]/%s", d.repoID, d.repoName)
115+
return fmt.Sprintf("migration from oneDev server %s [%d]/%s", d.baseURL, d.repoID, d.repoName)
116116
}
117117

118118
// ColorFormat provides a basic color format for a OneDevDownloader
@@ -121,7 +121,7 @@ func (d *OneDevDownloader) ColorFormat(s fmt.State) {
121121
log.ColorFprintf(s, "<nil: OneDevDownloader>")
122122
return
123123
}
124-
log.ColorFprintf(s, "migration from oneDev server as [%d]/%s", d.repoID, d.repoName)
124+
log.ColorFprintf(s, "migration from oneDev server %s [%d]/%s", d.baseURL, d.repoID, d.repoName)
125125
}
126126

127127
func (d *OneDevDownloader) callAPI(endpoint string, parameter map[string]string, result interface{}) error {

0 commit comments

Comments
 (0)