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

On Wed, Dec 8, 2021 at 7:14 AM Derrick Stolee [off-list ref] wrote:

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

quoted

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.

quoted

quoted

From: Derrick Stolee <redacted>
[...]
+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/ (local)

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.

The loss of checking for trees would be bad; the point of testing a
sparse-index is that it has tree objects in it.  However, the basic
suggestion inspired Stolee's variant below that does check for trees.
So...

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
'

This actually looks quite nice, though the magic '16' is kind of
annoying.  Could we get rid of that -- perhaps using something to rip
out the diff header, or using comm instead?

Also, perhaps 'dense' rather than 'out'?