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

On Fri, Dec 10 2021, Elijah Newren wrote:

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

quoted

Other than that 2/5 adds this documentation about ls-files --sparse:

        If the index is sparse, show the sparse directories without expanding
        to the contained files.

Shouldn't we at least add:

        Sparse directories will be shown with a trailing slash,
        e.g. "x/" for a sparse directory "x".q

Makes sense.  Except I don't understand the trailing 'q' -- typo?

Yes, sorry.

quoted

In addition to that I think this may have a buggy/unexpected interaction
with the --eol option:

    040000 aaff74984cccd156a469afa7d9ab10e4777beb24 0       i/      w/      attr/                   x/

I.e. should we be saying anything about the EOL state of these? OTOHO I
tried adding a submodule and it says the same, which seems similarly
odd, so maybe it's either correct, or this isn't updated for those
either.

If it matches what we do for submodules, for which eol values are also
non-sensical, then I think we're good enough for this series.  Perhaps
we just shouldn't print anything eol related for directories with
--eol, but that sounds like an orthogonal series rather than something
that should go in this one.

*nod*, probably. 

quoted

Is the behavior of:

    $ git -C sparse-index ls-files --stage --sparse -- 'folder2/a'
    $ echo $?
    0

Expected? I.e. accepting /a when we'd just print "folder2/" and not
e.g. erroring (probably, just asking)?

Fair question.  I think it's fine; by way of comparison:

$ git rm --cached removed-and-no-longer-tracked-file
$ git ls-files --stage -- non-existent-file
removed-and-no-longer-tracked-file untracked-file
$ echo $?
0

So it also shows nothing and displays nothing when asked for file(s)
that are not in the index.

Yes, there is a slight semantic difference in that in your example we
have a "folder2/" entry which *could be* expanded, but I am quite
happy with the literal interpretation of the command that there is no
"folder2/a" in the index.  Said another way, I'm happy with ls-files
showing what is in the index right now, rather than what could be in
it, or listing things that HEAD contains that we don't for whatever
reason.

Sounds good.

quoted

How about:

    $ ls -l sparse-index/x
    ls: cannot access 'sparse-index/x': No such file or directory
    $ 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
    $ git -C sparse-index ls-files --stage --no-empty-directory --sparse 'x/*'
    040000 aaff74984cccd156a469afa7d9ab10e4777beb24 0       x/

The answer is probably "yes that's fine" because I've got no idea how
sparse really works, but just checking..

You should read the docs for this option you are trying: "Do not list
empty directories. Has no effect without --directory."    (Also,
--directory only takes effect with --other, which you are also
missing.)

So yeah, that flag is irrelevant.  Perhaps ls-files should print a
warning when flags are passed but ignored due to other flags not being
passed, but that would belong in an orthogonal series rather than this
one.

...

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.

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?