Skip to content

Conversation

@matthewpeterkort
Copy link
Contributor

@matthewpeterkort matthewpeterkort commented Jan 27, 2026

factor out indexd-client into data-client
add a bunch of tests to get coverage back up to where it was

bwalsh and others added 30 commits January 16, 2026 17:47
bwalsh and others added 18 commits January 25, 2026 14:10
- Kept refactored client code (interface.go, indexd_client.go, etc.)
- Removed files moved to data-client (convert.go, drs/hash/*)
- Preserved RegisterFile implementation and progress event integration
- Moved LFS files from drs/ to lfs/ directory
- Updated dependencies with go mod tidy
- Kept refactored client code (interface.go, indexd_client.go, etc.)
- Removed files moved to data-client (convert.go, drs/hash/*)
- Preserved RegisterFile implementation and progress event integration
- Moved LFS files from drs/ to lfs/ directory
- Updated dependencies with go mod tidy
- Added replace directive for local data-client
@bwalsh
Copy link
Contributor

bwalsh commented Jan 30, 2026

This is a big PR. It really should be merged or rebased to cleanly branch from main.

Config regression: getLfsCustomTransferBool/getLfsCustomTransferInt now always return defaults and never read git config values, which means lfs.customtransfer.drs.upsert and lfs.customtransfer.drs.multipart-threshold can no longer be configured and the previous validation behavior is lost. This will silently ignore user configuration and may lead to unexpected upload behavior. Consider restoring the git-config reads (and validation) or documenting why those settings are no longer supported.

https://github.com/calypr/git-drs/blob/feature/factor-out-indexdClient/client/indexd/client.go#L60-L228
https://github.com/calypr/git-drs/blob/feature/factor-out-indexdClient/client/indexd/client.go#L227-L229

@bwalsh
Copy link
Contributor

bwalsh commented Jan 30, 2026

📓 AI gave me these suggestions re. branch maintenance.

--

Since your branch contains commits that have already been squash-merged to main, the best approach is to rebase your feature branch onto the latest main. This will replay only your unique changes on top of the current main branch.

Here's the recommended approach:

Option 1: Interactive Rebase (Recommended)

# 1. Make sure you're on your feature branch
git checkout Feature/factor-out-indexd-client

# 2. Fetch the latest changes from remote
git fetch origin

# 3. Start an interactive rebase from your starting commit
git rebase -i c73b56c1c372d6c2cb11031813b0d99f3f1d8483 --onto origin/main

# 4. In the interactive editor, keep only YOUR commits (the ones not in main)
#    Delete or comment out lines for commits already in main

# 5. After resolving any conflicts, force push
git push --force-with-lease origin Feature/factor-out-indexd-client

Option 2: Simpler Rebase (if you know exactly which commits are yours)

# 1. Fetch latest
git fetch origin

# 2. Rebase onto latest main
git rebase origin/main

# 3. During rebase, Git may show conflicts for already-merged commits
#    For these, you can skip them with:
git rebase --skip

# 4. Force push when done
git push --force-with-lease origin Feature/factor-out-indexd-client

Option 3: Create a Fresh Branch (Cleanest)

If the rebase gets messy with too many conflicts:

# 1. Create a new branch from latest main
git checkout -b feature/factor-out-indexd-client-rebased origin/main

# 2. Cherry-pick only YOUR commits
git cherry-pick <your-first-commit>..<your-last-commit>

# 3. Push the new branch
git push origin feature/factor-out-indexd-client-rebased

# 4. Update the PR to point to the new branch (or close old PR and open new one)

Important Notes:

  • Always use --force-with-lease instead of --force to avoid overwriting others' work
  • Coordinate with your team before force-pushing to a shared branch
  • The interactive rebase (Option 1) gives you the most control to exclude already-merged commits
  • Since the commits were "squash and merged", they'll have different SHAs in main, so Git won't automatically recognize them as duplicates

@matthewpeterkort
Copy link
Contributor Author

closed in favor of #190

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants