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

[Elijah Newren <newren@gmail.com>]

On Wed, Dec 8, 2021 at 7:14 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> 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.
>
> >> 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.

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'?