From 35ca58e1f5353a26179645bfe1e409bd0e689ffc Mon Sep 17 00:00:00 2001 From: Chen-Wei Sun Date: Thu, 1 Oct 2026 02:02:17 +0800 Subject: [PATCH] fix(gitextractor): keep deepening shallow clones past merge boundaries --shallow-since makes a merge commit shallow when any of its parents is older than since, which hides newer commits on its other parent. A single --deepen=1 can leave them without their first parent, so the collectors skip them and later incremental runs never fetch them again. After --deepen=1, keep deepening (1, 2, 4, ... generations, at most 10 extra rounds) while the newest shallow boundary commit is newer than since. Boundary commits that were not fetched are filtered with cat-file, lazy fetches are disabled where git supports GIT_NO_LAZY_FETCH, and failures are only logged as warnings. Closes #9189 --- .../gitextractor/parser/clone_gitcli.go | 103 +++++++++++- .../gitextractor/parser/clone_gitcli_test.go | 157 ++++++++++++++++++ 2 files changed, 259 insertions(+), 1 deletion(-) create mode 100644 backend/plugins/gitextractor/parser/clone_gitcli_test.go diff --git a/backend/plugins/gitextractor/parser/clone_gitcli.go b/backend/plugins/gitextractor/parser/clone_gitcli.go index 3b5c9450b1d..f61dbc10d14 100644 --- a/backend/plugins/gitextractor/parser/clone_gitcli.go +++ b/backend/plugins/gitextractor/parser/clone_gitcli.go @@ -18,12 +18,14 @@ limitations under the License. package parser import ( + "bytes" "encoding/base64" "fmt" "net/url" "os" "os/exec" "path" + "strconv" "strings" "time" @@ -37,6 +39,10 @@ import ( var _ RepoCloner = (*GitcliCloner)(nil) var ErrNoData = errors.NotModified.New("No data to be collected") +// maxExtraDeepenRounds caps the extra deepen rounds in deepenUntilSince. The +// depth doubles every round, so up to 1023 extra generations are covered. +const maxExtraDeepenRounds = 10 + // CloneRepoConfig is the configuration for the CloneRepo method // the subtask should run in Full Sync mode whenever the configuration is changed type CloneRepoConfig struct { @@ -200,6 +206,7 @@ func (g *GitcliCloner) CloneRepo() errors.Error { if err := g.deepen(); err != nil { return err } + g.deepenUntilSince() g.success = true } return nil @@ -231,6 +238,76 @@ func (g *GitcliCloner) deepen() errors.Error { return nil } +// deepenUntilSince fetches more history for as long as a shallow boundary +// commit is newer than g.since, doubling the depth each round (1, 2, 4, ...). +// +// Git makes a commit shallow (cuts it off from all of its parents) when any of +// its parents is older than --shallow-since. For a merge commit whose +// feature-branch parent is old, this also hides newer commits on its other +// parent, and a single --deepen=1 can still leave them without their own first +// parent. The collectors skip commits whose first parent is missing, and the +// next incremental run starts after them, so they would never be collected. +// See https://github.com/apache/devlake/issues/9189 +func (g *GitcliCloner) deepenUntilSince() { + depth := 1 + for round := 0; ; round++ { + newest, err := g.newestShallowCommitTime() + if err != nil { + g.logger.Warn(err, "failed to read the shallow boundary") + return + } + if newest.Before(*g.since) { + return + } + if round == maxExtraDeepenRounds { + g.logger.Warn(nil, "shallow boundary is still newer than %s after %d extra deepen rounds", g.since.Format(time.RFC3339), maxExtraDeepenRounds) + return + } + if err := g.gitFetch(fmt.Sprintf("--deepen=%d", depth)); err != nil { + g.logger.Warn(err, "failed to deepen the cloned repo") + return + } + depth *= 2 + } +} + +// newestShallowCommitTime returns the newest committer time of the shallow +// boundary commits present in the local repo, or the zero time if there are none. +func (g *GitcliCloner) newestShallowCommitTime() (time.Time, errors.Error) { + // all clones are bare, so the shallow file is in the repo dir itself + shallow, e := os.ReadFile(path.Join(g.localDir, "shallow")) + if os.IsNotExist(e) { + return time.Time{}, nil + } + if e != nil { + return time.Time{}, errors.Convert(e) + } + // the shallow file may list commits that were not fetched; `git log` fails on those + objects, err := g.gitOutput(shallow, "cat-file", "--batch-check=%(objectname) %(objecttype)") + if err != nil { + return time.Time{}, err + } + var commits bytes.Buffer + for _, line := range strings.Split(objects, "\n") { + if sha, objectType, ok := strings.Cut(line, " "); ok && objectType == "commit" { + commits.WriteString(sha + "\n") + } + } + if commits.Len() == 0 { + return time.Time{}, nil + } + // --no-walk lists the given commits newest first by committer time + newest, err := g.gitOutput(commits.Bytes(), "log", "--no-walk", "--stdin", "-n1", "--format=%ct") + if err != nil { + return time.Time{}, err + } + seconds, e := strconv.ParseInt(strings.TrimSpace(newest), 10, 64) + if e != nil { + return time.Time{}, errors.Convert(e) + } + return time.Unix(seconds, 0), nil +} + func (g *GitcliCloner) shallowClone() errors.Error { // to fetch newly added commits from ALL branches, we need to the following guide: // https://stackoverflow.com/questions/23708231/git-shallow-clone-clone-depth-misses-remote-branches @@ -318,6 +395,11 @@ func (g *GitcliCloner) gitCmd(gitcmd string, args ...string) errors.Error { } func (g *GitcliCloner) git(env []string, dir string, gitcmd string, args ...string) errors.Error { + return g.execCommand(g.gitCommand(env, dir, gitcmd, args...)) +} + +// gitCommand builds a git command with the global config args (auth, credential helper). +func (g *GitcliCloner) gitCommand(env []string, dir string, gitcmd string, args ...string) *exec.Cmd { g.logger.Debug("git %s %v", gitcmd, sanitizeArgs(args)) // CWE-532: sanitize before logging cmdArgs := append([]string{}, g.globalConfigArgs...) cmdArgs = append(cmdArgs, gitcmd) @@ -325,7 +407,26 @@ func (g *GitcliCloner) git(env []string, dir string, gitcmd string, args ...stri cmd := exec.CommandContext(g.ctx.GetContext(), "git", cmdArgs...) cmd.Env = env cmd.Dir = dir - return g.execCommand(cmd) + return cmd +} + +// gitOutput runs a git command in the cloned repo and returns its stdout. +// +// In a partial clone (--filter=blob:none, used when SkipCommitStat is set), +// looking up a missing object makes git fetch it from origin. GIT_NO_LAZY_FETCH +// prevents that where it is supported (git 2.45+ and some backports); with older +// git the fetch uses the same auth and proxy settings as the other fetches. +func (g *GitcliCloner) gitOutput(stdin []byte, gitcmd string, args ...string) (string, errors.Error) { + cmd := g.gitCommand(g.syncEnvs, g.localDir, gitcmd, args...) + cmd.Env = append(cmd.Environ(), "GIT_NO_LAZY_FETCH=1") + cmd.Stdin = bytes.NewReader(stdin) + var stderr bytes.Buffer + cmd.Stderr = &stderr + output, e := cmd.Output() + if e != nil { + return "", errors.Default.New(fmt.Sprintf("git %s in %s failed: %s", gitcmd, cmd.Dir, generateErrMsg(stderr.Bytes(), e))) + } + return string(output), nil } func (g *GitcliCloner) execCommand(cmd *exec.Cmd) errors.Error { diff --git a/backend/plugins/gitextractor/parser/clone_gitcli_test.go b/backend/plugins/gitextractor/parser/clone_gitcli_test.go new file mode 100644 index 00000000000..49b6ea32d45 --- /dev/null +++ b/backend/plugins/gitextractor/parser/clone_gitcli_test.go @@ -0,0 +1,157 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package parser + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/apache/devlake/helpers/unithelper" + mockplugin "github.com/apache/devlake/mocks/core/plugin" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// runGit runs git in dir with a fixed identity (and date, if given), and returns its trimmed stdout. +func runGit(t *testing.T, dir, date string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=DevLake Test", "GIT_AUTHOR_EMAIL=test@devlake.apache.org", + "GIT_COMMITTER_NAME=DevLake Test", "GIT_COMMITTER_EMAIL=test@devlake.apache.org", + ) + if date != "" { + cmd.Env = append(cmd.Env, "GIT_AUTHOR_DATE="+date, "GIT_COMMITTER_DATE="+date) + } + output, err := cmd.Output() + var stderr []byte + if exitErr, ok := err.(*exec.ExitError); ok { + stderr = exitErr.Stderr + } + require.NoError(t, err, "git %v: %s", args, stderr) + return strings.TrimSpace(string(output)) +} + +// buildMergeBoundaryRepo creates a bare repo with the history from issue #9189: +// +// R ---- Q ---- P ---- M main +// \ / +// F ---------------+ feature +// +// The previous incremental run started at 12:00, after F (10:00) and before +// Q (13:00), P (14:00) and M (16:00). +func buildMergeBoundaryRepo(t *testing.T) (dir string, commits map[string]string) { + t.Helper() + dir = filepath.Join(t.TempDir(), "origin.git") + require.NoError(t, os.Mkdir(dir, 0o755)) + runGit(t, dir, "", "init", "-q", "--bare") + runGit(t, dir, "", "symbolic-ref", "HEAD", "refs/heads/main") + tree := runGit(t, dir, "", "mktree") + commit := func(date, message string, parents ...string) string { + args := []string{"commit-tree", tree, "-m", message} + for _, parent := range parents { + args = append(args, "-p", parent) + } + return runGit(t, dir, date, args...) + } + commits = map[string]string{} + commits["R"] = commit("2024-01-01T00:00:00Z", "R") + commits["F"] = commit("2024-01-02T10:00:00Z", "F", commits["R"]) + commits["Q"] = commit("2024-01-02T13:00:00Z", "Q", commits["R"]) + commits["P"] = commit("2024-01-02T14:00:00Z", "P", commits["Q"]) + commits["M"] = commit("2024-01-02T16:00:00Z", "M", commits["P"], commits["F"]) + runGit(t, dir, "", "update-ref", "refs/heads/main", commits["M"]) + runGit(t, dir, "", "update-ref", "refs/heads/feature", commits["F"]) + return dir, commits +} + +func newTestCloner(t *testing.T, remoteDir string, since time.Time) *GitcliCloner { + t.Helper() + ctx := new(mockplugin.SubTaskContext) + ctx.On("GetContext").Return(context.Background()) + return &GitcliCloner{ + ctx: ctx, + taskData: &GitExtractorTaskData{Options: &GitExtractorOptions{}}, + logger: unithelper.DummyLogger(), + since: &since, + remoteUrl: "file://" + remoteDir, + localDir: filepath.Join(t.TempDir(), "clone.git"), + } +} + +func hasObject(dir, sha string) bool { + cmd := exec.Command("git", "cat-file", "-e", sha) + cmd.Dir = dir + return cmd.Run() == nil +} + +func TestIncrementalCloneKeepsCommitsBehindMergeBoundary(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git is not installed") + } + origin, commits := buildMergeBoundaryRepo(t) + cloner := newTestCloner(t, origin, time.Date(2024, 1, 2, 12, 0, 0, 0, time.UTC)) + + require.NoError(t, cloner.CloneRepo()) + + // Q, P and M are newer than since. Each must be fetched together with its + // first parent, otherwise CollectCommits skips it and no later run collects it. + for _, name := range []string{"Q", "P", "M"} { + assert.True(t, hasObject(cloner.localDir, commits[name]), "%s should be fetched", name) + } + assert.True(t, hasObject(cloner.localDir, commits["R"]), "R, the first parent of Q, should be fetched") + newest, err := cloner.newestShallowCommitTime() + require.NoError(t, err) + assert.True(t, newest.Before(*cloner.since), "shallow boundary %s should be older than since", newest) +} + +func TestNewestShallowCommitTimeIgnoresMissingCommits(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git is not installed") + } + origin, commits := buildMergeBoundaryRepo(t) + cloner := newTestCloner(t, origin, time.Date(2024, 1, 2, 12, 0, 0, 0, time.UTC)) + runGit(t, "", "", "clone", "-q", "--bare", "--depth=1", cloner.remoteUrl, cloner.localDir) + + // a depth=1 clone of main leaves only M on the boundary + newest, err := cloner.newestShallowCommitTime() + require.NoError(t, err) + assert.Equal(t, time.Date(2024, 1, 2, 16, 0, 0, 0, time.UTC), newest.UTC()) + + // git may list a boundary commit it did not fetch; it must not fail the lookup + shallowFile := filepath.Join(cloner.localDir, "shallow") + shallow, e := os.ReadFile(shallowFile) + require.NoError(t, e) + require.NoError(t, os.WriteFile(shallowFile, append(shallow, commits["P"]+"\n"...), 0o644)) + require.False(t, hasObject(cloner.localDir, commits["P"])) + newest, err = cloner.newestShallowCommitTime() + require.NoError(t, err) + assert.Equal(t, time.Date(2024, 1, 2, 16, 0, 0, 0, time.UTC), newest.UTC()) + + // a full clone has no shallow file + require.NoError(t, os.Remove(shallowFile)) + newest, err = cloner.newestShallowCommitTime() + require.NoError(t, err) + assert.True(t, newest.IsZero()) +}