Re: [PATCH 3/6] t6601: add helper for testing path-walk API

[Derrick Stolee <stolee@gmail.com>]

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@gmail.com> 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