Repository navigation
Conversation
This reverts commit 41fd01c.
|
@ethomson sorry to disturb you but what do you think about these changes? Do they seem valid or not? |
|
Hi @yerseg - no disturbance, thanks for the ping. I'll try to 👀 this soon. 🙏 |
|
@ethomson what’s up? will you have free time to review this? |
|
hi @ethomson The branch is based on v1.8.2+27, so I cherry-picked its 7 commits onto v1.9.7 — Test repo is libgit2.git itself at
git on the same 3000 commits: Both builds produce identical output: same delta count per commit (21394 total), same Two caveats: this is measured through pygit2 rather than a C harness (same overhead on |
Compare only the ids of two trees at the same path instead of calling maybe_modified on them. maybe_modified passed a delta for the tree to the notify callback, and a callback that skipped it made the diff skip the whole tree. With GIT_DIFF_IGNORE_CASE and GIT_DIFF_INCLUDE_CASECHANGE it added two deltas for a tree, and only one of them was removed. Detect trees with S_ISDIR like the tree iterator does, so that trees with a mode like 040755 are expanded too. Expand only the tree that comes next. With GIT_DIFF_INCLUDE_TYPECHANGE_TREES, also expand the trees that sort between a file `a` and `a/`, since typechange detection compares `a` with the next entry on the other side. Do not skip trees in case insensitive diffs: the case insensitive tree iterator merges trees whose names differ only in case, so one of them cannot be skipped on its own. Return tree entries also when the diff options are NULL. Add tests for these cases.
yerseg
changed the title
This branch has not been deployed
3 participants
git_diff_tree_to_treecurrently expands every subtree on both sides, so diffing two big trees that differ in a single file still walks and compares every file.This makes the tree iterators return tree entries instead. If both sides have a tree at the same path with the same id, the diff skips it without loading it. Any other tree gets expanded before the rest of the diff sees it, so the resulting deltas don't change.
Trees aren't skipped with
GIT_DIFF_INCLUDE_UNMODIFIED(the unmodified entries have to be listed) or withGIT_DIFF_IGNORE_CASE(the case-insensitive tree iterator merges trees whose names differ only in case, so you can't skip just one of them).Numbers from the benchmark in #7395, RelWithDebInfo, tree with 32768 files:
Testing: there are new tests in
tests/libgit2/diff/tree.c.does_not_load_unmodified_subtreesis expected to fail on main, since the shared subtree isn't in the odb and main tries to read it. I also ran the fuzzer from #7395, which checks every diff against the same diff withGIT_DIFF_INCLUDE_UNMODIFIED(that one expands all trees). About 33M runs, no differences. On the first version of this PR it found a difference within 90 seconds every time.git_blame_fileon a few files of this repo gives the same hunks as main.The first version had several bugs: tree deltas were passed to the notify callback,
IGNORE_CASE | INCLUDE_CASECHANGEleft a delta for the directory itself, trees with a mode other than040000weren't expanded, and nothing was skipped whenoptswas NULL. The last commit fixes them and adds the tests. The branch is still based on an older main, but it merges cleanly with the current one. I ran the tests on the merge result, and the fuzzer on current main with this change and #7394 (without that one the fuzzer finds the older bug instead).