On 11/1/24 6:23 PM, Jonathan Tan wrote:
I haven't looked thoroughly at the rest of the patches yet, but had a comment about this test. Rearranging: "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:+test_expect_success 'all' ' + test-tool path-walk -- --all >out && + + cat >expect <<-EOF && + TREE::$(git rev-parse topic^{tree}) + TREE::$(git rev-parse base^{tree}) + TREE::$(git rev-parse base~1^{tree}) + TREE::$(git rev-parse base~2^{tree}) + TREE:left/:$(git rev-parse base:left) + TREE:left/:$(git rev-parse base~2:left) + TREE:right/:$(git rev-parse topic:right) + TREE:right/:$(git rev-parse base~1:right) + TREE:right/:$(git rev-parse base~2:right) + trees:9[snip rest of "expect"] The way you're testing this, wouldn't the tests pass even if the OIDs aren't emitted in path order? (E.g. if topic:right and base~1:right were somehow grouped into two different groups, even though they have the same path.)
You are correct that if the path-walk API emitted multiple batches with the same path name, then we would not detect that via the current testing strategy. The main reason to use the sort is to avoid adding a restriction on the order in which objects appear within the batch. Your recommendation to group a batch into a single line does not strike me as a suitable approach, because long lines become hard to read and difficult to parse diffs. (Also, the order within the batch becomes baked in as a requirement.) The biggest question I'd like to ask is this: do you see a risk of a path being repeated? There are cases where it will happen, such as indexed objects that are not reachable anywhere else. The way I would consider modifying these tests to reflect the batching would be to associate each batch with a number, causing the order of the paths to become hard-coded in the test. Something like 0:COMMIT::$(git rev-parse ...) 0:COMMIT::$(git rev-parse ...) 1:TREE::$(git rev-parse ...) 1:TREE::$(git rev-parse ...) 2:TREE:right/:$(git rev-parse ...) 3:BLOB:right/a:$(...) 4:TREE:left/:$(git rev-parse ...) 5:BLOB:left/b:$(...) This would imply some amount of order that maybe should become a requirement of the API. Thanks, -Stolee
