From c9d925524417dc0fe9497dc0cfe78ec66d8549e2 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 12 Mar 2024 12:21:27 +0800 Subject: [PATCH] Lazy load object format with command line and don't do it in OpenRepository (#29712) Most time, when invoking `git.OpenRepository`, `objectFormat` will not be used, so it's a waste to invoke commandline to get the object format. This PR make it a lazy operation, only invoke that when necessary. (cherry picked from commit e84e5db6de0306d514b1f1a9657931fb7197a188) --- modules/git/blame_sha256_test.go | 5 +++-- modules/git/blame_test.go | 4 +++- modules/git/commit_sha256_test.go | 7 +++++-- modules/git/repo_base_nogogit.go | 5 ----- modules/git/repo_commit.go | 7 ++++++- modules/git/repo_commit_gogit.go | 5 ++++- modules/git/repo_commit_nogogit.go | 9 ++++++--- modules/git/repo_compare.go | 6 +++++- modules/git/repo_compare_test.go | 9 ++++++--- modules/git/repo_index.go | 6 +++++- modules/git/repo_tag.go | 4 ++-- modules/git/repo_tag_test.go | 3 +-- modules/git/repo_tree_gogit.go | 7 ++++++- modules/git/repo_tree_nogogit.go | 12 ++++++++++-- modules/git/tree_nogogit.go | 14 ++++++++++---- 15 files changed, 72 insertions(+), 31 deletions(-) diff --git a/modules/git/blame_sha256_test.go b/modules/git/blame_sha256_test.go index 92f8b5b02a..fcb00e2a38 100644 --- a/modules/git/blame_sha256_test.go +++ b/modules/git/blame_sha256_test.go @@ -120,11 +120,12 @@ func TestReadingBlameOutputSha256(t *testing.T) { }, } + objectFormat, err := repo.GetObjectFormat() + assert.NoError(t, err) for _, c := range cases { commit, err := repo.GetCommit(c.CommitID) assert.NoError(t, err) - - blameReader, err := CreateBlameReader(ctx, repo.objectFormat, "./tests/repos/repo6_blame_sha256", commit, "blame.txt", c.Bypass) + blameReader, err := CreateBlameReader(ctx, objectFormat, "./tests/repos/repo6_blame_sha256", commit, "blame.txt", c.Bypass) assert.NoError(t, err) assert.NotNil(t, blameReader) defer blameReader.Close() diff --git a/modules/git/blame_test.go b/modules/git/blame_test.go index 327edab767..4220c85600 100644 --- a/modules/git/blame_test.go +++ b/modules/git/blame_test.go @@ -118,11 +118,13 @@ func TestReadingBlameOutput(t *testing.T) { }, } + objectFormat, err := repo.GetObjectFormat() + assert.NoError(t, err) for _, c := range cases { commit, err := repo.GetCommit(c.CommitID) assert.NoError(t, err) - blameReader, err := CreateBlameReader(ctx, repo.objectFormat, "./tests/repos/repo6_blame", commit, "blame.txt", c.Bypass) + blameReader, err := CreateBlameReader(ctx, objectFormat, "./tests/repos/repo6_blame", commit, "blame.txt", c.Bypass) assert.NoError(t, err) assert.NotNil(t, blameReader) defer blameReader.Close() diff --git a/modules/git/commit_sha256_test.go b/modules/git/commit_sha256_test.go index 916169f4e2..a4309519cf 100644 --- a/modules/git/commit_sha256_test.go +++ b/modules/git/commit_sha256_test.go @@ -152,10 +152,13 @@ func TestHasPreviousCommitSha256(t *testing.T) { commit, err := repo.GetCommit("f004f41359117d319dedd0eaab8c5259ee2263da839dcba33637997458627fdc") assert.NoError(t, err) + objectFormat, err := repo.GetObjectFormat() + assert.NoError(t, err) + parentSHA := MustIDFromString("b0ec7af4547047f12d5093e37ef8f1b3b5415ed8ee17894d43a34d7d34212e9c") notParentSHA := MustIDFromString("42e334efd04cd36eea6da0599913333c26116e1a537ca76e5b6e4af4dda00236") - assert.Equal(t, repo.objectFormat, parentSHA.Type()) - assert.Equal(t, repo.objectFormat.Name(), "sha256") + assert.Equal(t, objectFormat, parentSHA.Type()) + assert.Equal(t, objectFormat.Name(), "sha256") haz, err := commit.HasPreviousCommit(parentSHA) assert.NoError(t, err) diff --git a/modules/git/repo_base_nogogit.go b/modules/git/repo_base_nogogit.go index 86b6a93567..50a0a975b8 100644 --- a/modules/git/repo_base_nogogit.go +++ b/modules/git/repo_base_nogogit.go @@ -71,11 +71,6 @@ func OpenRepository(ctx context.Context, repoPath string) (*Repository, error) { repo.batchWriter, repo.batchReader, repo.batchCancel = CatFileBatch(ctx, repoPath) repo.checkWriter, repo.checkReader, repo.checkCancel = CatFileBatchCheck(ctx, repoPath) - repo.objectFormat, err = repo.GetObjectFormat() - if err != nil { - return nil, err - } - return repo, nil } diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go index 9c9ee7768f..44273d2253 100644 --- a/modules/git/repo_commit.go +++ b/modules/git/repo_commit.go @@ -246,7 +246,12 @@ func (repo *Repository) CommitsByFileAndRange(opts CommitsByFileAndRangeOptions) } }() - len := repo.objectFormat.FullLength() + objectFormat, err := repo.GetObjectFormat() + if err != nil { + return nil, err + } + + len := objectFormat.FullLength() commits := []*Commit{} shaline := make([]byte, len+1) for { diff --git a/modules/git/repo_commit_gogit.go b/modules/git/repo_commit_gogit.go index 4cab957564..84580be9a5 100644 --- a/modules/git/repo_commit_gogit.go +++ b/modules/git/repo_commit_gogit.go @@ -41,7 +41,10 @@ func (repo *Repository) RemoveReference(name string) error { // ConvertToHash returns a Hash object from a potential ID string func (repo *Repository) ConvertToGitID(commitID string) (ObjectID, error) { - objectFormat := repo.objectFormat + objectFormat, err := repo.GetObjectFormat() + if err != nil { + return nil, err + } if len(commitID) == hash.HexSize && objectFormat.IsValid(commitID) { ID, err := NewIDFromString(commitID) if err == nil { diff --git a/modules/git/repo_commit_nogogit.go b/modules/git/repo_commit_nogogit.go index a7031184e2..ae4c21aaa3 100644 --- a/modules/git/repo_commit_nogogit.go +++ b/modules/git/repo_commit_nogogit.go @@ -132,8 +132,11 @@ func (repo *Repository) getCommitFromBatchReader(rd *bufio.Reader, id ObjectID) // ConvertToGitID returns a GitHash object from a potential ID string func (repo *Repository) ConvertToGitID(commitID string) (ObjectID, error) { - IDType := repo.objectFormat - if len(commitID) == IDType.FullLength() && IDType.IsValid(commitID) { + objectFormat, err := repo.GetObjectFormat() + if err != nil { + return nil, err + } + if len(commitID) == objectFormat.FullLength() && objectFormat.IsValid(commitID) { ID, err := NewIDFromString(commitID) if err == nil { return ID, nil @@ -142,7 +145,7 @@ func (repo *Repository) ConvertToGitID(commitID string) (ObjectID, error) { wr, rd, cancel := repo.CatFileBatchCheck(repo.Ctx) defer cancel() - _, err := wr.Write([]byte(commitID + "\n")) + _, err = wr.Write([]byte(commitID + "\n")) if err != nil { return nil, err } diff --git a/modules/git/repo_compare.go b/modules/git/repo_compare.go index 0e9a0c70d7..b6e9d2b44a 100644 --- a/modules/git/repo_compare.go +++ b/modules/git/repo_compare.go @@ -283,8 +283,12 @@ func (repo *Repository) GetPatch(base, head string, w io.Writer) error { // If base is undefined empty SHA (zeros), it only returns the files changed in the head commit // If base is the SHA of an empty tree (EmptyTreeSHA), it returns the files changes from the initial commit to the head commit func (repo *Repository) GetFilesChangedBetween(base, head string) ([]string, error) { + objectFormat, err := repo.GetObjectFormat() + if err != nil { + return nil, err + } cmd := NewCommand(repo.Ctx, "diff-tree", "--name-only", "--root", "--no-commit-id", "-r", "-z") - if base == repo.objectFormat.EmptyObjectID().String() { + if base == objectFormat.EmptyObjectID().String() { cmd.AddDynamicArguments(head) } else { cmd.AddDynamicArguments(base, head) diff --git a/modules/git/repo_compare_test.go b/modules/git/repo_compare_test.go index 526b213550..9983873186 100644 --- a/modules/git/repo_compare_test.go +++ b/modules/git/repo_compare_test.go @@ -126,17 +126,20 @@ func TestGetCommitFilesChanged(t *testing.T) { assert.NoError(t, err) defer repo.Close() + objectFormat, err := repo.GetObjectFormat() + assert.NoError(t, err) + testCases := []struct { base, head string files []string }{ { - repo.objectFormat.EmptyObjectID().String(), + objectFormat.EmptyObjectID().String(), "95bb4d39648ee7e325106df01a621c530863a653", []string{"file1.txt"}, }, { - repo.objectFormat.EmptyObjectID().String(), + objectFormat.EmptyObjectID().String(), "8d92fc957a4d7cfd98bc375f0b7bb189a0d6c9f2", []string{"file2.txt"}, }, @@ -146,7 +149,7 @@ func TestGetCommitFilesChanged(t *testing.T) { []string{"file2.txt"}, }, { - repo.objectFormat.EmptyTree().String(), + objectFormat.EmptyTree().String(), "8d92fc957a4d7cfd98bc375f0b7bb189a0d6c9f2", []string{"file1.txt", "file2.txt"}, }, diff --git a/modules/git/repo_index.go b/modules/git/repo_index.go index 47705a92af..6aaab242c1 100644 --- a/modules/git/repo_index.go +++ b/modules/git/repo_index.go @@ -94,6 +94,10 @@ func (repo *Repository) LsFiles(filenames ...string) ([]string, error) { // RemoveFilesFromIndex removes given filenames from the index - it does not check whether they are present. func (repo *Repository) RemoveFilesFromIndex(filenames ...string) error { + objectFormat, err := repo.GetObjectFormat() + if err != nil { + return err + } cmd := NewCommand(repo.Ctx, "update-index", "--remove", "-z", "--index-info") stdout := new(bytes.Buffer) stderr := new(bytes.Buffer) @@ -101,7 +105,7 @@ func (repo *Repository) RemoveFilesFromIndex(filenames ...string) error { for _, file := range filenames { if file != "" { buffer.WriteString("0 ") - buffer.WriteString(repo.objectFormat.EmptyObjectID().String()) + buffer.WriteString(objectFormat.EmptyObjectID().String()) buffer.WriteByte('\t') buffer.WriteString(file) buffer.WriteByte('\000') diff --git a/modules/git/repo_tag.go b/modules/git/repo_tag.go index ae5dbd171f..e8c5ce6fb8 100644 --- a/modules/git/repo_tag.go +++ b/modules/git/repo_tag.go @@ -141,7 +141,7 @@ func (repo *Repository) GetTagInfos(page, pageSize int) ([]*Tag, int, error) { break } - tag, err := parseTagRef(repo.objectFormat, ref) + tag, err := parseTagRef(ref) if err != nil { return nil, 0, fmt.Errorf("GetTagInfos: parse tag: %w", err) } @@ -161,7 +161,7 @@ func (repo *Repository) GetTagInfos(page, pageSize int) ([]*Tag, int, error) { } // parseTagRef parses a tag from a 'git for-each-ref'-produced reference. -func parseTagRef(objectFormat ObjectFormat, ref map[string]string) (tag *Tag, err error) { +func parseTagRef(ref map[string]string) (tag *Tag, err error) { tag = &Tag{ Type: ref["objecttype"], Name: ref["refname:lstrip=2"], diff --git a/modules/git/repo_tag_test.go b/modules/git/repo_tag_test.go index 9816e311a8..785c3442a7 100644 --- a/modules/git/repo_tag_test.go +++ b/modules/git/repo_tag_test.go @@ -194,7 +194,6 @@ func TestRepository_GetAnnotatedTag(t *testing.T) { } func TestRepository_parseTagRef(t *testing.T) { - sha1 := Sha1ObjectFormat tests := []struct { name string @@ -351,7 +350,7 @@ Add changelog of v1.9.1 (#7859) for _, test := range tests { tc := test // don't close over loop variable t.Run(tc.name, func(t *testing.T) { - got, err := parseTagRef(sha1, tc.givenRef) + got, err := parseTagRef(tc.givenRef) if tc.wantErr { require.Error(t, err) diff --git a/modules/git/repo_tree_gogit.go b/modules/git/repo_tree_gogit.go index 6391959e6a..dc97ce1344 100644 --- a/modules/git/repo_tree_gogit.go +++ b/modules/git/repo_tree_gogit.go @@ -21,7 +21,12 @@ func (repo *Repository) getTree(id ObjectID) (*Tree, error) { // GetTree find the tree object in the repository. func (repo *Repository) GetTree(idStr string) (*Tree, error) { - if len(idStr) != repo.objectFormat.FullLength() { + objectFormat, err := repo.GetObjectFormat() + if err != nil { + return nil, err + } + + if len(idStr) != objectFormat.FullLength() { res, _, err := NewCommand(repo.Ctx, "rev-parse", "--verify").AddDynamicArguments(idStr).RunStdString(&RunOpts{Dir: repo.Path}) if err != nil { return nil, err diff --git a/modules/git/repo_tree_nogogit.go b/modules/git/repo_tree_nogogit.go index 582247b4a4..e82012de6f 100644 --- a/modules/git/repo_tree_nogogit.go +++ b/modules/git/repo_tree_nogogit.go @@ -51,7 +51,11 @@ func (repo *Repository) getTree(id ObjectID) (*Tree, error) { case "tree": tree := NewTree(repo, id) tree.ResolvedID = id - tree.entries, err = catBatchParseTreeEntries(repo.objectFormat, tree, rd, size) + objectFormat, err := repo.GetObjectFormat() + if err != nil { + return nil, err + } + tree.entries, err = catBatchParseTreeEntries(objectFormat, tree, rd, size) if err != nil { return nil, err } @@ -69,7 +73,11 @@ func (repo *Repository) getTree(id ObjectID) (*Tree, error) { // GetTree find the tree object in the repository. func (repo *Repository) GetTree(idStr string) (*Tree, error) { - if len(idStr) != repo.objectFormat.FullLength() { + objectFormat, err := repo.GetObjectFormat() + if err != nil { + return nil, err + } + if len(idStr) != objectFormat.FullLength() { res, err := repo.GetRefCommitID(idStr) if err != nil { return nil, err diff --git a/modules/git/tree_nogogit.go b/modules/git/tree_nogogit.go index 28d02c7e81..a591485082 100644 --- a/modules/git/tree_nogogit.go +++ b/modules/git/tree_nogogit.go @@ -77,8 +77,11 @@ func (t *Tree) ListEntries() (Entries, error) { return nil, runErr } - var err error - t.entries, err = parseTreeEntries(t.repo.objectFormat, stdout, t) + objectFormat, err := t.repo.GetObjectFormat() + if err != nil { + return nil, err + } + t.entries, err = parseTreeEntries(objectFormat, stdout, t) if err == nil { t.entriesParsed = true } @@ -101,8 +104,11 @@ func (t *Tree) listEntriesRecursive(extraArgs TrustedCmdArgs) (Entries, error) { return nil, runErr } - var err error - t.entriesRecursive, err = parseTreeEntries(t.repo.objectFormat, stdout, t) + objectFormat, err := t.repo.GetObjectFormat() + if err != nil { + return nil, err + } + t.entriesRecursive, err = parseTreeEntries(objectFormat, stdout, t) if err == nil { t.entriesRecursiveParsed = true }