Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refactor 7 return values in parseGitUrl to RepoSpec(issue 4866, Task1) #4900

Merged
merged 2 commits into from
Dec 2, 2022
Merged

Refactor 7 return values in parseGitUrl to RepoSpec(issue 4866, Task1) #4900

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0408844
initial commit to refactor 7 return values in parseGitUrl to RepoSpec
kishorerj Dec 1, 2022
4bcfd37
fix review comments
kishorerj Dec 2, 2022
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
69 changes: 33 additions & 36 deletions api/internal/git/repospec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,22 +86,19 @@ func NewRepoSpecFromURL(n string) (*RepoSpec, error) {
if filepath.IsAbs(n) {
return nil, fmt.Errorf("uri looks like abs path: %s", n)
}
host, orgRepo, path, gitRef, gitSubmodules, suffix, gitTimeout := parseGitURL(n)
if orgRepo == "" {
repoSpecVal := parseGitURL(n)
if repoSpecVal.OrgRepo == "" {
return nil, fmt.Errorf("url lacks orgRepo: %s", n)
}
if host == "" {
if repoSpecVal.Host == "" {
return nil, fmt.Errorf("url lacks host: %s", n)
}
cleanedPath := filepath.Clean(strings.TrimPrefix(path, string(filepath.Separator)))
cleanedPath := filepath.Clean(strings.TrimPrefix(repoSpecVal.Path, string(filepath.Separator)))
if pathElements := strings.Split(cleanedPath, string(filepath.Separator)); len(pathElements) > 0 &&
pathElements[0] == filesys.ParentDir {
return nil, fmt.Errorf("url path exits repo: %s", n)
}
return &RepoSpec{
raw: n, Host: host, OrgRepo: orgRepo,
Dir: notCloned, Path: path, Ref: gitRef, GitSuffix: suffix,
Submodules: gitSubmodules, Timeout: gitTimeout}, nil
return repoSpecVal, nil
}

const (
Expand All @@ -112,68 +109,68 @@ const (

// From strings like git@github.com:someOrg/someRepo.git or
// https://github.com/someOrg/someRepo?ref=someHash, extract
// the parts.
func parseGitURL(n string) (
host string, orgRepo string, path string, gitRef string, gitSubmodules bool, gitSuff string, gitTimeout time.Duration) {
// the different parts of URL , set into a RepoSpec object and return RepoSpec object.
func parseGitURL(n string) *RepoSpec {
repoSpec := &RepoSpec{raw: n, Dir: notCloned, Timeout: defaultTimeout, Submodules: defaultSubmodules}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we initialize Timeout and Submodules here?

We should set raw and Dir because they remain constant. Their initialized values are the final return values.

On the other hand, Timeout and Submodules are always initialized on line 121 below. If they aren't present in n, parseQuery returns their default values. We are doing unnecessary work by initializing them here. Would recommend removing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I am initialising the default values is because I think its good practice to initialize with user defined default values where possible during object creation itself.. If we create any new logic between 114 and 121 in future which will return RepoSpec we can always ensure that RepoSpect has timeout set to the default values that we want (27secs) instead of the type defaults.. That was the reasoning.. please let me know what you think

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see. That makes sense. Thanks for the explanation!

// parse query first
// safe because according to rfc3986: ? only allowed in query
// and not recognized %-encoded
beforeQuery, query, _ := strings.Cut(n, "?")
n = beforeQuery
// if no query, defaults returned
gitRef, gitTimeout, gitSubmodules = parseQuery(query)
repoSpec.Ref, repoSpec.Timeout, repoSpec.Submodules = parseQuery(query)

if strings.Contains(n, gitDelimiter) {
index := strings.Index(n, gitDelimiter)
// Adding _git/ to host
host = normalizeGitHostSpec(n[:index+len(gitDelimiter)])
orgRepo = strings.Split(n[index+len(gitDelimiter):], "/")[0]
path = parsePath(n[index+len(gitDelimiter)+len(orgRepo):])
return
repoSpec.Host = normalizeGitHostSpec(n[:index+len(gitDelimiter)])
repoSpec.OrgRepo = strings.Split(n[index+len(gitDelimiter):], "/")[0]
repoSpec.Path = parsePath(n[index+len(gitDelimiter)+len(repoSpec.OrgRepo):])
return repoSpec
}
host, n = parseHostSpec(n)
isLocal := strings.HasPrefix(host, "file://")
repoSpec.Host, n = parseHostSpec(n)
isLocal := strings.HasPrefix(repoSpec.Host, "file://")
if !isLocal {
gitSuff = gitSuffix
repoSpec.GitSuffix = gitSuffix
}
if strings.Contains(n, gitSuffix) {
gitSuff = gitSuffix
repoSpec.GitSuffix = gitSuffix
index := strings.Index(n, gitSuffix)
orgRepo = n[0:index]
repoSpec.OrgRepo = n[0:index]
n = n[index+len(gitSuffix):]
if len(n) > 0 && n[0] == '/' {
n = n[1:]
}
path = parsePath(n)
return
repoSpec.Path = parsePath(n)
return repoSpec
}

if isLocal {
if idx := strings.Index(n, "//"); idx > 0 {
orgRepo = n[:idx]
repoSpec.OrgRepo = n[:idx]
n = n[idx+2:]
path = parsePath(n)
return
repoSpec.Path = parsePath(n)
return repoSpec
}
orgRepo = parsePath(n)
return
repoSpec.OrgRepo = parsePath(n)
return repoSpec
}

i := strings.Index(n, "/")
if i < 1 {
path = parsePath(n)
return
repoSpec.Path = parsePath(n)
return repoSpec
}
j := strings.Index(n[i+1:], "/")
if j >= 0 {
j += i + 1
orgRepo = n[:j]
path = parsePath(n[j+1:])
return
repoSpec.OrgRepo = n[:j]
repoSpec.Path = parsePath(n[j+1:])
return repoSpec
}
path = ""
orgRepo = parsePath(n)
return host, orgRepo, path, gitRef, gitSubmodules, gitSuff, gitTimeout
repoSpec.Path = ""
repoSpec.OrgRepo = parsePath(n)
return repoSpec
}

// Clone git submodules by default.
Expand Down