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

From: Elijah Newren <newren@gmail.com>
Date: Dec 11, 2021, 04:45 UTC
Message-ID: <CABPp-BFEayJvGytbsi76Nqe=tWj6zX5Aa+ErAxG=akC-yp1JFw@mail.gmail.com>
In-Reply-To: <211211.86ee6jc1t2.gmgdl@evledraar.gmail.com>

On Fri, Dec 10, 2021 at 6:28 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 20 quoted lines

>
> 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:
> > [...]
> >> 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:

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

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

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

Show 6 quoted lines

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