Re: [PATCH v3 0/5] Sparse index: fetch, pull, ls-files

On Fri, Dec 10, 2021 at 6:28 PM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

On Fri, Dec 10 2021, Elijah Newren wrote:

quoted

On Fri, Dec 10, 2021 at 8:31 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:
[...]

quoted

So it's very nice to have the new diff test in 2/5, but would be much
nicer/assuring to have that split into a trivial function followed by
seeing how the diff looked in combination with each of the other option
that "ls-files" accepts.

There's no point testing in combination with flags that only affect
untracked files.  And I'm very dubious of adding testing for a case
where we would need to add an explicit disclaimer that "We have no
idea what the output should be but we are testing it anyway".  So the
options you suggest at least are things I'd rather not see us trying
to add to the testing here.

This series is adding a new flag to ls-files, it doesn't error out when
combined with other existing flags, and observably changes their output.

Ah, I think you had a misunderstanding here.  If what you say here
were true, then indeed we would need some testing and it'd suggest
some kind of bug.  But the combination here does not observably change
the output.  You were missing an important testcase for comparison.
Let me repeat your testing and sprinkle in some commentary:

quoted

quoted

    $ ls -l sparse-index/x
    ls: cannot access 'sparse-index/x': No such file or directory

Right, this is a sparse directory; good to double check.

quoted

quoted

    $ git -C sparse-index ls-files --stage 'x/*'
    100644 78981922613b2afb6025042ff6bd878ac1994e85 0       x/a
    $ git -C sparse-index ls-files --stage --no-empty-directory 'x/*'
    100644 78981922613b2afb6025042ff6bd878ac1994e85 0       x/a

Right, --no-empty-directory by itself is a useless option that won't
affect the output of ls-files; it only takes affect with --directory,
which in turn only takes affect with other options.  Since that
options is useless, the output is the same for both of these.

quoted

quoted

    $ git -C sparse-index ls-files --stage --no-empty-directory --sparse 'x/*'
    040000 aaff74984cccd156a469afa7d9ab10e4777beb24 0       x/

Here you added --sparse, but you neglected what it would show without
--no-empty-directory, which is a critical comparison point.  So let me
fill it in:

     $ git -C sparse-index ls-files --stage --sparse 'x/*'
     040000 aaff74984cccd156a469afa7d9ab10e4777beb24 0       x/

Now, this case I added in comparison to the one three above it shows
that, yes, --sparse does indeed change the output relative to --stage.
And it does so by design.  Now if you compare my added case to the
last one you showed, you can verify that adding --no-empty-directory
to that mix does not change the output further; --no-empty-directory
is a useless/ignored option unless you also include other flags that
were not involved here.

I think erroring out would be fine, or doing whatever it's doing now,
but either way the gap in test coverage should be closed, shouldn't it?

I'd think the easiest and probably most prudent fix would just be to say
that we don't think some of these make sense with --sparse and have them
error out if they're combined, no?

ls-files offers several options that allow you to either slice and
dice or tweak the output, and function on two kinds of files: tracked,
and not tracked.  Several examples of such flags:
   * tracked: --cached, --stage, --unmerged, --modified, --sparse (and
I think --error-unmatch)
   * not tracked: --others, --ignored, --exclude, --exclude-from,
--exclude-standard, --directory, --no-empty-directory

Now, in particular, specifying any of --exclude, --exclude-from,
--exclude-standard, --directory, or --no-empty-directory is a complete
waste of breath and will do nothing unless you also specify --others
or --ignored.  None of these options interact in any way with any of
the flags from the --tracked category.

I don't think we want an n! permutation of all combinations tested.  I
don't even think an n^2 pair-wise combination makes sense when we know
that some flags have no effect on their own.  What would make sense is
perhaps adding a warning to ls-files when specified flags will have no
utility due to depending on other flags that have not been specified.
But that's in no way specific to --sparse and does not make sense to
me to make part of this topic.