fix/incomplete-add-url by bwalsh · Pull Request #185 · calypr/git-drs

bwalsh · 2026-01-28T05:54:34Z

Copilot

Pull request overview

This PR restructures the git drs add-url command to directly fetch data from S3 into the local Git LFS store and introduces a new lfss3 helper package for S3 + Git LFS introspection, along with associated tests and updated coverage artifacts.

Changes:

Replace the old add-url implementation with a new Cmd/runAddURL that inspects an S3 object, streams it into the local LFS store, and writes a Git LFS pointer file in the worktree.
Add cmd/addurl/lfss3 with S3 URL parsing, LFS storage resolution, S3 HeadObject logic, an HTTP-based fetch helper with progress reporting, and unit tests for these helpers.
Wire addurl.Cmd into cmd/root.go and refresh coverage outputs to reflect new code and tests; remove the legacy cmd/addurl/main_test.go.

Reviewed changes

Copilot reviewed 7 out of 11 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
coverage/integration/coverage.out	Updated integration coverage numbers to include the new `addurl`/`lfss3` code paths.
coverage/combined.out	Updated combined coverage metadata, reflecting new and removed lines in `addurl`, indexd client, config, logging, and other packages.
cmd/root.go	Switch root command to register `addurl.Cmd` (new command variable) instead of the removed `AddURLCmd`.
cmd/addurl/main.go	Replaces the previous Gen3/indexd-driven `add-url` flow with a new implementation that inspects S3 via `lfss3`, downloads the object into the LFS store, computes SHA256, and writes a Git LFS pointer in the worktree.
cmd/addurl/main_test.go	Deletes the old `generatePointerFile` unit test, which is no longer applicable after removing that helper.
cmd/addurl/lfss3/is_lfs_tracked_test.go	New tests verifying `IsLFSTracked` correctly interprets `.gitattributes` and distinguishes LFS-tracked vs non-tracked paths.
cmd/addurl/lfss3/inspect_test.go	New tests covering S3 URL parsing, SHA256 normalization/extraction, Git common-dir/LFS root resolution (including `lfs.storage` variants), and helpers for running git commands in temp repos.
cmd/addurl/lfss3/inspect.go	New `lfss3` package logic for resolving LFS storage, parsing S3 URLs, making authenticated `HeadObject` calls, extracting metadata (including SHA256), and determining whether a path is LFS-tracked.
cmd/addurl/lfss3/agent_fetch_reader.go	New helper that converts S3/HTTP URLs to downloadable form, issues an HTTP GET, and wraps the response in a progress-reporting `io.ReadCloser` for use by `runAddURL`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

cmd/addurl/main.go

cmd/addurl/lfss3/agent_fetch_reader.go

cloud/agent_fetch_reader.go

cmd/addurl/lfss3/inspect.go

Copilot · 2026-01-28T06:00:22Z

cmd/addurl/main.go

+func runAddURL(cmd *cobra.Command, args []string) (err error) {
+	ctx := cmd.Context()
+	if ctx == nil {
+		ctx = context.Background()
+	}
+
+	s3URL := args[0]

-		// Call client.AddURL to handle Gen3 interactions
-		meta, err := drsClient.AddURL(s3URL, sha256, awsAccessKey, awsSecretKey, awsRegion, awsEndpoint)
-		if err != nil {
-			return err
+	// Determine path: use provided optional arg, otherwise derive from URL path
+	var pathArg string
+	if len(args) == 2 {
+		pathArg = args[1]
+	} else {
+		u, perr := url.Parse(s3URL)
+		if perr != nil {
+			return perr
 		}
+		pathArg = strings.TrimPrefix(u.Path, "/")
+	}
+
+	sha256Param, ferr := cmd.Flags().GetString("sha256")
+	if ferr != nil {
+		return fmt.Errorf("read flag sha256: %w", ferr)
+	}
+
+	awsKey, ferr := cmd.Flags().GetString(s3_utils.AWS_KEY_FLAG_NAME)
+	if ferr != nil {
+		return fmt.Errorf("read flag %s: %w", s3_utils.AWS_KEY_FLAG_NAME, ferr)
+	}
+	awsSecret, ferr := cmd.Flags().GetString(s3_utils.AWS_SECRET_FLAG_NAME)
+	if ferr != nil {
+		return fmt.Errorf("read flag %s: %w", s3_utils.AWS_SECRET_FLAG_NAME, ferr)
+	}
+	awsRegion, ferr := cmd.Flags().GetString(s3_utils.AWS_REGION_FLAG_NAME)
+	if ferr != nil {
+		return fmt.Errorf("read flag %s: %w", s3_utils.AWS_REGION_FLAG_NAME, ferr)
+	}
+	awsEndpoint, ferr := cmd.Flags().GetString(s3_utils.AWS_ENDPOINT_URL_FLAG_NAME)
+	if ferr != nil {
+		return fmt.Errorf("read flag %s: %w", s3_utils.AWS_ENDPOINT_URL_FLAG_NAME, ferr)
+	}
+
+	if awsKey == "" || awsSecret == "" {
+		return errors.New("AWS credentials must be provided via flags or environment variables")
+	}
+	if awsRegion == "" {
+		return errors.New("AWS region must be provided via flag or environment variable")
+	}
+
+	s3Input := lfss3.InspectInput{
+		S3URL:        s3URL,
+		AWSAccessKey: awsKey,
+		AWSSecretKey: awsSecret,
+		AWSRegion:    awsRegion,
+		AWSEndpoint:  awsEndpoint,
+		SHA256:       sha256Param,
+		WorktreeName: pathArg,
+	}
+	info, err := lfss3.InspectS3ForLFS(ctx, s3Input)
+	if err != nil {
+		return err
+	}


This new runAddURL implementation only talks to S3 and the local Git LFS store and never constructs a DRS client or calls DRSClient.AddURL, so git drs add-url no longer registers the S3 URL with the configured Gen3/Anvil remote. If that behavior is still required, consider layering the new "copy into local LFS" step on top of the existing indexd/DRS registration flow rather than replacing it outright.

cmd/addurl/lfss3/agent_fetch_reader.go

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 7 out of 12 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-28T16:46:28Z

cmd/addurl/main.go

+	isLFSTracked, err := lfss3.IsLFSTracked(pathArg)
+	if err != nil {
+		return fmt.Errorf("check LFS tracking for %s: %w", pathArg, err)
+	}
+
+	if _, err := fmt.Fprintf(cmd.OutOrStdout(), `
+Resolved Git LFS info
+---------------------
+Git common dir : %s
+LFS storage    : %s
+
+S3 object
+---------
+Bucket         : %s
+Key            : %s
+Worktree name  : %s
+Size (bytes)   : %d
+SHA256 (meta)  : %s
+ETag           : %s
+Last modified  : %s
+
+Worktree
+-------------
+path           : %s
+tracked by LFS : %v


isLFSTracked is only used for printing and no longer enforces that the target path is tracked by Git LFS, whereas the previous AddURL implementation (client/indexd/add_url.go:304-316) explicitly failed when the path was not LFS-tracked. This allows git drs add-url to succeed while writing a pointer file for a path that Git LFS will never manage, which can leave consumers with a plain text pointer instead of real data and undermines the goal of avoiding incomplete LFS contracts. Consider treating a non‑LFS‑tracked path as an error (with a clear message about running git lfs track and committing .gitattributes), to preserve the earlier safety guarantees.

Copilot · 2026-01-28T16:46:29Z

cmd/addurl/main.go

+	// 5) write pointer file in working tree
+	pointer := fmt.Sprintf(
+		"version https://git-lfs.github.com/spec/v1\noid sha256:%s\nsize %d\n",
+		oid, info.SizeBytes,
+	)
+	// write pointer file to working tree pathArg
+	if pathArg == "" {
+		return fmt.Errorf("empty worktree path")
+	}
+	safePath := filepath.Clean(pathArg)
+	dir := filepath.Dir(safePath)
+	if dir != "." && dir != "/" {
+		if err := os.MkdirAll(dir, 0755); err != nil {
+			return fmt.Errorf("mkdir %s: %w", dir, err)
+		}
+	}
+	if err = os.WriteFile(safePath, []byte(pointer), 0644); err != nil {
+		return fmt.Errorf("write %s: %w", safePath, err)
 	}


The new implementation writes the Git LFS pointer file to safePath but never stages it with git add, whereas the previous generatePointerFile helper also ran git add on the created pointer. This means git drs add-url no longer ensures that the pointer is added to the index, so callers expecting the command to fully "add" the file must remember to run git add themselves, which is a behavioral regression and easy to miss in scripts. Consider either restoring the git add step (or equivalent) here or clearly changing the command’s contract and documentation to reflect that it only writes the file without staging it.

cmd/addurl/main.go

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

matthewpeterkort

You had talked in standup about how there might be too many lines of code in this PR and things could be simplified.

Wrote a PR that implements a Go caching library instead of all the boiler plate that you had it should cut the caching code in half but added another testing file so that's why the +/- is only net -300 ish lines:
https://github.com/calypr/git-drs/pull/193/changes

matthewpeterkort · 2026-02-02T17:32:06Z

.github/workflows/pr-checks.yaml

        go-version-file: go.mod

    - name: Run tests
-      run: go test -v -race $(go list ./... | grep -v 'tests/integration/calypr' | grep -v 'client/indexd/tests')


this is going to bork some integration tests I had on another branch probably better to keep this how it is an a your | grep -v '/cmd/addurl$' string to it

matthewpeterkort · 2026-02-02T18:00:34Z

cmd/addurl/main.go

+// AddURLService groups injectable dependencies used to implement the add-url
+// behavior (logger factory, S3 inspection, LFS helpers, config loader, etc.).
+type AddURLService struct {
+	newLogger    func(string, bool) (*slog.Logger, error)


this technically works, but it's not idiomatic go. Let's keep it for now since alot of changes are being made but in the future it will probably need to be refactored to use an interface-first approach since that is how dependency injection is done in go.

matthewpeterkort · 2026-02-02T19:39:45Z

cmd/prepush/main.go

+}

-		myLogger.Debug("~~~~~~~~~~~~~ START: pre-push ~~~~~~~~~~~~~")
+type PrePushService struct {


like I said earlier this pattern is generally not advisable, but lets keep it for now.

matthewpeterkort · 2026-02-02T19:43:08Z

precommit_cache/helpers.go

+
+// Cache provides read-only access to the `.git/drs/pre-commit` cache.
+// Use Open to construct an instance with correct paths resolved.
+type Cache struct {


This works, but there are a-lot of caching libraries that exist in Go that will do a-lot of this boiler plate for you, see maypok86.github.io/otter/blog/cache-evolution

I've used otterv2 before, it's pretty good. The attached PR uses that library to cut down on the amount of code.

bwalsh · 2026-02-05T02:24:02Z

@matthewpeterkort closing this and continuing on #197

initial #141

7ae334d

Copilot AI review requested due to automatic review settings January 28, 2026 05:54

Copilot started reviewing on behalf of bwalsh January 28, 2026 05:54 View session

Copilot AI reviewed Jan 28, 2026

View reviewed changes

bwalsh and others added 11 commits January 28, 2026 05:00

updated unit coverage

9dce35a

test coverage/combined.out

8982f01

Apply suggestions from code review

6de4d91

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

PR suggestions

3ae9554

filename input

8c9423d

filename coverage

c322f1a

signed url

97438c8

signed url coverage

c87e418

rm noop test

09b3cd1

rm noop test

04124dd

rm noop test coverage

fbf5028

bwalsh requested a review from Copilot January 28, 2026 16:38

Copilot started reviewing on behalf of bwalsh January 28, 2026 16:38 View session

Copilot AI reviewed Jan 28, 2026

View reviewed changes

bwalsh and others added 12 commits January 28, 2026 10:08

improve comment

0469de1

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

improves unit tests

41aec59

adds simple add-url test

44e03f5

add-url coverage

0c7b1e6

wip add-url xfer

2fdd34d

wip: add-url experiments

f4fabff

add-url

f482b72

improve err log

4a41aa3

improve err log

b496fc8

Improve err msg

662d345

improve err log

8ac2886

improve error msg

1da9d6a

bwalsh added 26 commits January 30, 2026 08:03

incremental changes

d91a3c4

incremental changes

1ef3f05

refactor packages

364ef47

refactor packages

e36ec2a

refactor add-url tests and deps

7967227

refactor add-url tests and deps

8d8951d

improve test & doc

96ce2d1

improve test & doc

c3cf765

pre-commit-changes #192

8100e5a

pre-commit-changes #192

f3797e5

pre-commit-changes #192 tests

42b7205

pre-commit-changes #192 tests

203205a

doc changes to precommit/addurl

6d59b0d

improve tests, doc

d9df177

improve tests, doc

eb60c22

improve precommit test

7c59619

improve precommit test

72aafb1

install precommit

3f85ffe

install precommit

90a40fa

install precommit test

f35b875

install precommit test

71c1d7b

update user doc

f99a454

even files 20M

1a2e47f

2 files in TARGET-ALL-P2

a9b5c6f

feature/pre-commit-changes #192

fa5eb43

feature/pre-commit-changes #192

6a629f1

matthewpeterkort reviewed Feb 2, 2026

View reviewed changes

bwalsh mentioned this pull request Feb 5, 2026

fix/incomplete-add-url-3 #197

Merged

bwalsh closed this Feb 5, 2026

Comments

Conversation

bwalsh commented Jan 28, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matthewpeterkort left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewpeterkort Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

matthewpeterkort Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

matthewpeterkort Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

matthewpeterkort Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bwalsh commented Feb 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

matthewpeterkort left a comment •

edited

Loading

matthewpeterkort Feb 2, 2026 •

edited

Loading