[GitHub] [incubator-devlake] klesh commented on a diff in pull request #2050: issues/1914: refactor run() method

[GitBox <gi...@apache.org>]

klesh commented on code in PR #2050:
URL: https://github.com/apache/incubator-devlake/pull/2050#discussion_r896423503


##########
plugins/gitextractor/parser/clone.go:
##########
@@ -64,29 +71,46 @@ func (l *LibGit2) CloneOverHTTP(repoId, url, user, password, proxy string) error
 	}
 	dir, err := ioutil.TempDir("", "gitextractor")
 	if err != nil {
-		return err
+		return nil, err

Review Comment:
   I think the program would panic if this line was hit.
   Because `defer func`  would be executed and `repo` and `cleanup` both being nil.



##########
plugins/gitextractor/parser/clone.go:
##########
@@ -64,29 +71,46 @@ func (l *LibGit2) CloneOverHTTP(repoId, url, user, password, proxy string) error
 	}
 	dir, err := ioutil.TempDir("", "gitextractor")
 	if err != nil {
-		return err
+		return nil, err
+	}
+	cleanup = func() {
+		os.RemoveAll(dir)
 	}
-	defer os.RemoveAll(dir)
-	repo, err := git.Clone(url, dir, cloneOptions)
+	clonedRepo, err := git.Clone(url, dir, cloneOptions)
 	if err != nil {
-		return err
+		return nil, err
 	}
-	return l.run(repo, repoId)
+	repo = l.newGitRepo(repoId, clonedRepo, cleanup)
+	return repo, err
 }
 
-func (l *LibGit2) CloneOverSSH(repoId, url, privateKey, passphrase string) error {
+func (l *GitRepoCreator) CloneOverSSH(repoId, url, privateKey, passphrase string) (*GitRepo, error) {
+	var cleanup func()
+	var repo *GitRepo
+	defer func() {
+		if repo == nil {
+			cleanup()
+		}
+	}()
 	dir, err := ioutil.TempDir("", "gitextractor")
 	if err != nil {
-		return err
+		return nil, err

Review Comment:
   Same as above, the `Temporary Directory` maintenance seems complicated, I would suggest that we don't delete it here, and handle it in `CloseablePluginTask` altogether instead. We are going to delete the cloned repo anyway.



##########
plugins/gitextractor/gitextractor.go:
##########
@@ -50,12 +56,40 @@ func (plugin GitExtractor) PrepareTaskData(taskCtx core.TaskContext, options map
 	if err != nil {
 		return nil, err
 	}
-	return op, nil
+	storage := store.NewDatabase(taskCtx.GetDb(), taskCtx.GetLogger())
+	repo, err := initGitRepo(taskCtx, storage, op)
+	if err != nil {
+		return nil, err
+	}
+	return repo, nil
+}
+
+func (plugin GitExtractor) Close(taskCtx core.TaskContext) error {
+	if repo, ok := taskCtx.GetData().(*parser.GitRepo); ok {
+		if err := repo.Close(); err != nil {
+			return err
+		}
+	}
+	return nil
 }
 
 func (plugin GitExtractor) RootPkgPath() string {
 	return "github.com/apache/incubator-devlake/plugins/gitextractor"
 }
 
+func initGitRepo(ctx core.TaskContext, storage models.Store, op tasks.GitExtractorOptions) (*parser.GitRepo, error) {

Review Comment:
   By convention, this function should be `newGitRepo`



##########
plugins/gitextractor/parser/repo.go:
##########
@@ -0,0 +1,335 @@
+package parser
+
+import (
+	"context"
+	"fmt"
+	"github.com/apache/incubator-devlake/models/domainlayer"
+	"github.com/apache/incubator-devlake/models/domainlayer/code"
+	"github.com/apache/incubator-devlake/plugins/core"
+	"github.com/apache/incubator-devlake/plugins/gitextractor/models"
+	git "github.com/libgit2/git2go/v33"
+)
+
+type GitRepo struct {
+	store   models.Store
+	taskCtx core.TaskContext
+	ctx     context.Context
+	logger  core.Logger
+	id      string
+	repo    *git.Repository
+	cleanup func()
+}
+
+func (r *GitRepo) CollectAll(subtaskCtx core.SubTaskContext) error {
+	r.taskCtx.SetProgress(0, -1)
+	err := r.CollectTags(subtaskCtx)
+	if err != nil {
+		return err
+	}
+	err = r.CollectBranches(subtaskCtx)
+	if err != nil {
+		return err
+	}
+	return r.CollectCommits(subtaskCtx)
+}
+
+func (r *GitRepo) Close() error {
+	defer func() {
+		if r.cleanup != nil {
+			r.cleanup()
+		}
+	}()
+	return r.store.Close()
+}
+
+func (r *GitRepo) CountTags() (int, error) {
+	tags, err := r.repo.Tags.List()
+	if err != nil {
+		return 0, err
+	}
+	return len(tags), nil
+}
+
+func (r *GitRepo) CountBranches() (int, error) {
+	var repoInter *git.BranchIterator
+	repoInter, err := r.repo.NewBranchIterator(git.BranchAll)
+	if err != nil {
+		return 0, err
+	}
+	count := 0
+	err = repoInter.ForEach(func(branch *git.Branch, branchType git.BranchType) error {
+		select {
+		case <-r.ctx.Done():
+			return r.ctx.Err()
+		default:
+			break
+		}
+		if branch.IsBranch() {
+			count++
+		}
+		return nil
+	})
+	return count, err
+}
+
+func (r *GitRepo) CountCommits() (int, error) {
+	odb, err := r.repo.Odb()
+	if err != nil {
+		return 0, err
+	}
+	count := 0
+	err = odb.ForEach(func(id *git.Oid) error {
+		select {
+		case <-r.ctx.Done():
+			return r.ctx.Err()
+		default:
+			break
+		}
+		commit, _ := r.repo.LookupCommit(id)
+		if commit != nil {
+			count++
+		}
+		return nil
+	})
+	return count, err
+}
+
+func (r *GitRepo) CollectTags(_ core.SubTaskContext) error {

Review Comment:
   I would suggest that we delete this parameter for all 3 methods since we already have it as a field of the `struct`



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org