Re: [PATCH 2/2] ls-files: add --sparse option

From: Derrick Stolee <stolee@gmail.com>
Date: Dec 8, 2021, 15:14 UTC
Message-ID: <03a642fc-6310-1ea4-083e-9fe4530cf761@gmail.com>
In-Reply-To: <211123.86h7c3wrg2.gmgdl@evledraar.gmail.com>

On 11/22/2021 9:07 PM, Ævar Arnfjörð Bjarmason wrote:

> 
> On Tue, Nov 16 2021, Derrick Stolee via GitGitGadget wrote:

Things in the dependent topics are starting to simmer down, so I'm back revisiting this topic.

Show 57 quoted lines

>> From: Derrick Stolee <dstolee@microsoft.com>
>> [...]
>> +test_expect_success 'ls-files' '
>> +	init_repos &&
>> +
>> +	# Behavior agrees by default. Sparse index is expanded.
>> +	test_all_match git ls-files &&
>> +
>> +	# With --sparse, the sparse index data changes behavior.
>> +	git -C sparse-index ls-files --sparse >sparse-index-out &&
>> +	grep "^folder1/\$" sparse-index-out &&
>> +	grep "^folder2/\$" sparse-index-out &&
>> +
>> +	# With --sparse and no sparse index, nothing changes.
>> +	git -C sparse-checkout ls-files --sparse >sparse-checkout-out &&
>> +	grep "^folder1/0/0/0\$" sparse-checkout-out &&
>> +	! grep "/\$" sparse-checkout-out &&
> 
> I think all of this would be much clearer both in terms of explaining
> this change, and also for future test relability if it did away with the
> selective grepping, and simply ran tls-files with and without --sparse,
> and then test_cmp'd the full output (after munging away the OIDs).
> 
> I.e. the sort of output that's in my just-sent reply to the CL:
> https://lore.kernel.org/git/211123.86lf1fwrq5.gmgdl@evledraar.gmail.com/
> 
> We really don't need to optimize for lines of tests added, and having
> ~30 lines of plainly understood diff output is IMO preferrable to even 5
> lines of tricky positive & negative grep invocations that take some time
> to reason about and understand.
> 
> I.e. something like:
> 
>     cat >expected <<-\EOF &&
>      100644 blob OID   e
>      100644 blob OID   folder1-
>      100644 blob OID   folder1.x
>     -040000 tree OID   folder1/
>     +100644 blob OID   folder1/0/0/0
>     +100644 blob OID   folder1/0/1
>     +100644 blob OID   folder1/a
>      100644 blob OID   folder10
>     -040000 tree OID   folder2/
>     +100644 blob OID   folder2/0/0/0
>     +100644 blob OID   folder2/0/1
>     +100644 blob OID   folder2/a
>      100644 blob OID   g
>     -040000 tree OID   x/
>     +100644 blob OID   x/a
>      100644 blob OID   z
>     EOF
>     git [...] ls-files --sparse >actual.raw &&
>     [munge away OIDs] <actual.raw >actual &&
>     test_cmp expected actual
> 
> Would test everything you're trying to test here and more (would need 2x
> of those..), and would be easier to read & understand.

I don't think it is that hard to understand "I expect to see these lines and not these lines" but I am open to more fully verifying the full output and demonstrating the change that happens when the flag is added.

Taking your idea and applying it to 'ls-files' (without --stage to avoid OIDs which would change depending on the hash algorithm), the start of the test looks like this:

test_expect_success 'ls-files' '
	init_repos &&

	# Behavior agrees by default. Sparse index is expanded.
	test_all_match git ls-files &&

	# With --sparse, the sparse index data changes behavior.
	git -C sparse-index ls-files --stage >out &&
	git -C sparse-index ls-files --stage --sparse >sparse &&

	cat >expect <<-\EOF &&
	 e
	 folder1-
	 folder1.x
	-folder1/0/0/0
	-folder1/0/1
	-folder1/a
	+folder1/
	 folder10
	-folder2/0/0/0
	-folder2/0/1
	-folder2/a
	+folder2/
	 g
	-x/a
	+x/
	 z
	EOF

	diff -u out sparse | tail -n 16 >actual &&
	test_cmp expect actual
'

I can make similar adjustments throughout the test to match this style.

Thanks, -Stolee