knot2: honor repo log path filters #4482

Open

nnabeyang.tngl.sh 3mo ago

targeting master from nnabeyang.tngl.sh/core:fix/repo-log-path-filter

Open opened by and assigned to

nnabeyang.tngl.sh 3mo ago targeting master from nnabeyang.tngl.sh/core:fix/repo-log-path-filter

sh.tangled.repo.log now filters commit history by the requested file or directory path.

Commits 7

knot2/xrpc: test merge, rename, and delete path history

+75

knot2: bound path-filtered log traversal

+63 -18

knot2/git: memoize path entries during log traversal

+26 -4

knot2/xrpc: keep history for deleted log paths

+29 -7

knot2/git: short-circuit path parent comparisons

+3 -2

knot2/git: name full-history path log traversal

+7 -2

knot2: honor repo log path filters

+99 -2

Comments

v0 3mo ago

oyster.cafe 3mo ago

thanks!

could you please add a log_path_budget_ms or something like last_commits does? a malicious repo could dump 20000 commits and currently we'll just wait N time. i guess while clamping down we could also memoize the lookups too.

the behavior here is git log --full-history -- path right? if we want git log -- path I think we should do some more interesting traversal. though i think it's fine keeping the full-hist mode here and naming as such, up to you 🤷

on knot2/crates/knot-xrpc/src/reads.rs:406 I believe we could just

let (commits, total) = repo.log_window_path(start, path, skip, limit, log_deadline)?;
if total == 0 && commits.is_empty() {
return Err(path_not_found());
}

right? that way a deleted path will still get history while a wrong one will 404

knot2/crates/knot-git/src/reads.rs:197-200 how about

commit
.parents
.iter()
.try_fold(false, |changed, parent| {
Ok(changed || self.entry_at(*parent, path)? != current)
})

Regarding tests, could you please add something that shows the code going over a fixture with a merge, a delete, and a rename?

nnabeyang.tngl.sh 2mo ago

Addressed the review feedback in separate commits:

added a configurable xrpc.log_path_budget_ms traversal budget (default: 2000 ms)
named the traversal explicitly as full-history behavior
short-circuited parent comparisons
kept history available for deleted paths while nonexistent paths still return 404
memoized path-entry lookups during traversal
added coverage for merge, rename, and delete history

nnabeyang.tngl.sh 2mo ago

Hi @oyster.cafe — is the review of this PR on hold because of #707? Should I close it?

Versions