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

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, gitster@pobox.com, newren@gmail.com,
	vdye@github.com, Derrick Stolee <derrickstolee@github.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 2/2] ls-files: add --sparse option
Date: Tue, 23 Nov 2021 03:07:08 +0100	[thread overview]
Message-ID: <211123.86h7c3wrg2.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <e42c0feec94de0e4869cda1fc6b28bd7055774e3.1637077083.git.gitgitgadget@gmail.com>

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

> 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.