Use git partial clone and worktree to reduce network/file io

[Warashi]

What this PR does:

• Use not cp -rf but git worktree add to Copy the git.Repo
• Use git partial clone to reduce network io

Why we need it:

I want to reduce network/file io

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

• How are users affected by this change:
• Is this breaking change:
• How to migrate (if breaking change):

[codecov]

Codecov Report

Attention: Patch coverage is 17.27273% with 91 lines in your changes missing coverage. Please review.

Project coverage is 25.79%. Comparing base (8f23da4) to head (c5216c9).
Report is 11 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/git/repo.go	28.07%	35 Missing and 6 partials ⚠️
pkg/git/gittest/git.mock.go	0.00%	11 Missing ⚠️
pkg/app/piped/deploysource/deploysource.go	0.00%	10 Missing ⚠️
pkg/app/pipedv1/deploysource/deploysource.go	0.00%	10 Missing ⚠️
pkg/app/piped/driftdetector/cloudrun/detector.go	0.00%	3 Missing ⚠️
pkg/app/piped/driftdetector/ecs/detector.go	0.00%	3 Missing ⚠️
pkg/app/piped/driftdetector/kubernetes/detector.go	0.00%	3 Missing ⚠️
pkg/app/piped/driftdetector/lambda/detector.go	0.00%	3 Missing ⚠️
pkg/app/piped/driftdetector/terraform/detector.go	0.00%	3 Missing ⚠️
pkg/app/piped/eventwatcher/eventwatcher.go	0.00%	2 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5412      +/-   ##
==========================================
+ Coverage   25.74%   25.79%   +0.05%     
==========================================
  Files         449      450       +1     
  Lines       48452    48606     +154     
==========================================
+ Hits        12474    12539      +65     
- Misses      35005    35085      +80     
- Partials      973      982       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Warashi]

/review

[github-actions]

PR Analysis

Main theme

"Implementation of worktree for repository operations and addition of 'Clean' method for resource cleanup"

PR summary

"This PR introduces the use of git worktrees for operations on repositories in various parts of the application to improve isolation and management of changes. It also includes the implementation of a 'Clean' method to remove temporary resources, effectively cleaning up after worktree usage."

Type of PR

"Enhancement"

PR Feedback:

General suggestions

The use of worktrees is a solid approach to handle operations on repositories in an isolated environment. This prevents accidental impact on the main repository and provides a clean way to manage changes for detection, event watching, and other tasks requiring repo modifications. Including the 'Clean' method enhances resource management by ensuring temporary worktrees are correctly removed after use. This avoids potential issues with lingering resources and maintains a clean working environment.

Code feedback

- relevant file: "pkg/app/piped/driftdetector/cloudrun/detector.go"
  suggestion: |-
    Add error handling for the `repo.Clean()` call to ensure any issues during worktree cleanup are captured and handled appropriately. This should prevent unnoticed errors and allow for proper logging or retry mechanisms if needed. 
    ```go
    defer func() {
      if err := repo.Clean(); err != nil {
        // Log error or handle it accordingly
      }
    }()
    ```
    (important)
  relevant line: "+			defer repo.Clean()"

- relevant file: "pkg/git/client.go"
  suggestion: |-
    It is important to carefully manage the transition to using the `--filter=blob:none` option with `git clone`. This can change the behavior for users who may not expect a partial clone. Ensure that there is proper documentation and that it is applicable to all use cases where the client is used.
    Additionally, assess if this change might require a cleanup or migration step for existing clients or stored repositories where full clones were expected. Communicate this change with users to manage expectations.
    (medium)
  relevant line: "+				args := []string{"clone", "--mirror", "--filter=blob:none", remote, repoCachePath}"

Security concerns:

"no"
The code changes do not seem to introduce obvious security concerns. The enhancement focuses on repository management and cleanup, without altering the security posture of the application.

[Warashi]

I added this to restrict operation at worktree.

[khanhtc1202]

How about using a tar trick to avoid .git copy and remove. I think tar is available everywhere and fast enough compared to cp

tar -cpP --exclude=.git -C "$source_dir" . | tar -xpP -C "$target_dir"

[Warashi]

Thanks. I'll try.

[Warashi]

@khanhtc1202

We use the alpine as the base image for piped, so I checked the tar command in the piped image.
First, the tar command exists. But, the options differ from the GNU/BSD tar.
For example, short flags -p and -P do not exist.
IMO, it's not good to depend on this tar behavior because users may run the piped on a non-alpine-based OS.
WDYT?

Note: I checked tar --help in the piped container image. I paste the result below.

$ docker run --rm -it --entrypoint sh  ghcr.io/pipe-cd/piped:v0.50.0-23-g8f23da4@sha256:88ec60ebf2b7e3131ff38b1af4a043117ef688bc636ddd95afd6bd368293651b
/ $ tar --help
BusyBox v1.36.1 (2024-06-10 07:11:47 UTC) multi-call binary.

Usage: tar c|x|t [-ZzJjahmvokO] [-f TARFILE] [-C DIR] [-T FILE] [-X FILE] [LONGOPT]... [FILE]...

Create, extract, or list files from a tar file

        c       Create
        x       Extract
        t       List
        -f FILE Name of TARFILE ('-' for stdin/out)
        -C DIR  Change to DIR before operation
        -v      Verbose
        -O      Extract to stdout
        -m      Don't restore mtime
        -o      Don't restore user:group
        -k      Don't replace existing files
        -Z      (De)compress using compress
        -z      (De)compress using gzip
        -J      (De)compress using xz
        -j      (De)compress using bzip2
        --lzma  (De)compress using lzma
        -a      (De)compress based on extension
        -h      Follow symlinks
        -T FILE File with names to include
        -X FILE File with glob patterns to exclude
        --exclude PATTERN       Glob pattern to exclude
        --overwrite             Replace existing files
        --strip-components NUM  NUM of leading components to strip
        --no-recursion          Don't descend in directories
        --numeric-owner         Use numeric user:group
        --no-same-permissions   Don't restore access permissions

[khanhtc1202]

This is the version without -P, the more universal solution

tar --exclude='.git' -cf - source_dir | tar -xf - -C destination_dir

@Warashi

[Warashi]

OKay, thank you for providing this. I'll use this script.

[Warashi]

I tried some tar options and finally reached this commit.
I tested it with busybox's tar (the same as alpine's) and GNU tar.
c5216c9

[khanhtc1202]

Overall LGTM. Just left a nits, want to give it a shot 👍

[khanhtc1202]

LGreatTM 💯
This could be the patch we're looking for 🥇

[ffjlabo]

@Warashi
it seems that the dir is removed after executing the loadHeadServiceManifest
So we might not call repo.Clean(). WDYT?

		// We have to copy repository into another directory because
		// decrypting the sealed secrets or attaching files might change the git repository.
		if attachmentUsed || encryptionUsed {
			dir, err := os.MkdirTemp("", "detector-git-processing")
			if err != nil {
				return nil, fmt.Errorf("failed to prepare a temporary directory for git repository (%w)", err)
			}
			defer os.RemoveAll(dir)

			repo, err = repo.Copy(filepath.Join(dir, "repo"))
			if err != nil {
				return nil, fmt.Errorf("failed to copy the cloned git repository (%w)", err)
			}
			defer repo.Clean()

			repoDir = repo.GetPath()
			appDir = filepath.Join(repoDir, app.GitPath.Path)
		}

https://github.com/pipe-cd/pipecd/blob/c5216c91bffc906f57b30133f3d0e557f1b1888f/pkg/app/piped/driftdetector/kubernetes/detector.go#L270C1-L281C22

[Warashi]

@ffjlabo

I think we should call repo.Clean() here because the 'repo.Clean()' not only does the directory deletion.
IMO, we should call some methods like Clean() or Close() everywhere because the caller doesn't know what happens with such methods. The caller only knows the cleanup methods exist.

[ffjlabo]

@Warashi

I think we should call repo.Clean() here because the 'repo.Clean()' not only does the directory deletion.

I got it. The git worktree needs additional operation other than deleting the directory to keep the consistency of the worktree as you said.

IMO, we should call some methods like Clean() or Close() everywhere because the caller doesn't know what happens with such methods. The caller only knows the cleanup methods exist.

Also I agree with it.

[ffjlabo]

A little off topic, but the detector reuses the one repo. loadHeadServiceManifest also uses it as an arg. So I feel a little bit confused about cleaning at this point.

But I noticed that this section is used temporarily and different from the actual repo passed as arg. 🙏

[ffjlabo]

[IMO] How about using the Worktree and Copy?

	Worktree(dest string) (Worktree, error)
	Copy(dest string) (Repo, error)

[Warashi]

It sounds clear, but I want to leave here as-is.
I want to clear when to use the CopyToModify method with its naming.
Almost all of implementation, we don't need modification of git commits/histories, so we should not use CopyToModify, and we can know from its naming.
If we rename them as Worktree and Copy, some implementation may call Copy method mistakenly even if it's enough to call the Worktree method.

WDYT?

[ffjlabo]

Almost all of implementation, we don't need modification of git commits/histories, so we should not use CopyToModify, and we can know from its naming.

I got your point! I agree with it.

[ffjlabo]

LGTM