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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Dec 10 2021, Elijah Newren wrote:

> On Fri, Dec 10, 2021 at 8:31 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
> [...]
>> 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.

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

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

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

...

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