bisync: make max-delete aware of tracked renames - #9812

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

This is off to a good start, thank you! Some thoughts on the design:

1. The new b.march.objects1 and b.march.objects2 retain an fs.Object for every file on both paths for the whole run. That's a significant memory cost for large runs, and I'm not sure it's necessary, given that we have a lighter option in plain sight: b.march.ls1 and ls2 (*fileList) already store the file's size and modtime, and have a hash field we simply aren't populating by default -- all the components needed for the renames strategy analysis.

   objects1 and objects2 exist only because the preflight needs hashes the listing doesn't carry -- the easier fix is to make the listings carry them. I'd suggest deleting the changes in march.go and instead, forcing a common hash into both listings in compare.go. setHashType's early-return branch already forces a common hash -- it's just a matter of making sure we end up there when --max-delete-renames-aware is in use with --track-renames-strategy hash, and reconciling conflicting flags (--ignore-listing-checksum) with appropriate ignore / error logic (see setCompareDefaults for examples). Note that the preflight's strategy must be equal to or stronger than what the sync will actually use -- so when in doubt, it's safer to error or disable, not fall back to a looser strategy.

   Since CountGuaranteedMatches is not currently used by sync, we could change the function signature without changing anything in sync. Currently you have:

func (m *Matcher) CountGuaranteedMatches(sources []fs.Object) int

Consider instead something like:

func CountGuaranteedMatches(strategy Strategy, modifyWindow time.Duration, srcs, dsts []Candidate) int

where Candidate is a struct like:

type Candidate struct {
    Remote  string
    Size    int64
    ModTime time.Time
    Hash    string
}

2. An invalid --track-renames-strategy (say, --track-renames-strategy banana) fails late, if at all. That's because trackrenames.ParseStrategy is called only in the event that someone exceeds the delete threshold. I'd move that check up to where your existing if opt.MaxDeleteRenamesAware && !ci.TrackRenames check is in Bisync().

3. I like that it's gated behind if !opt.Force, but a consequence of this is that with --force --max-delete-renames-aware, it still does all of the setup but never reads it. Under the change I'm proposing in point 1, this still matters, as we'd be storing (perhaps even calculating) lots of hashes we might never need. I'd suggest checking for --force early and skipping most of the --max-delete-renames-aware logic when true.

4. trackedRenameExemptionsForPath duplicates many of the checks from fs/sync/sync.go:240-260 (CanServerSideMove, strategy.UsesHash(), etc.), which risks the requirements diverging over time. I'd suggest refactoring these checks to one function in the fs/sync/trackrenames library, which both sync and bisync will call.

5. Rather than threading an exemptions int parameter through effectiveDeletes / exceedsDeletes / excessDeletes, I'd suggest making it a field on deltaSet alongside deleted and oldCount. That also lets excessDeletes() keep its original zero-argument signature and shrinks the deltas.go diff.
   Housekeeping:

6. cmd/bisync/rc.md is autogenerated and should not be edited (your edits would eventually get overwritten anyway)

7. The refactoring from fs/sync/sync.go to fs/sync/trackrenames/trackrenames.go should be its own self-contained, behavior-preserving commit with no bisync changes. The bisync changes should then be a separate commit on top of that.

8. Both commits should have a commit message in the house style (see CONTRIBUTING.md)

Overall design note: as much as possible, try to keep the --track-renames logic in fs/sync/trackrenames instead of duplicating it in sync and bisync, and try to keep the bisync-specific stuff in cmd/bisync/trackrenames.go when possible -- there's a maintenance cost to having to reason with this code in lots of key parts of bisync in the future, rather than reasoning with it once now in a separate file we shouldn't have to revisit much.

Thank you!

Very nice job with the revisions! It's very close now. See my comments inline.

Great work, thanks! I have just one inline comment here, and it's an easy fix 🙂

Looks good to me. Thanks again for your work on this 🙂

(The lint CI failure is an unrelated issue already fixed on master)