Re: [RFC/PATCH 3/5] ls-files: add and use a new --sparse option

[Elijah Newren <newren@gmail.com>]

On Wed, Mar 17, 2021 at 11:27 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Wed, Mar 17 2021, Elijah Newren wrote:
>
> > On Wed, Mar 17, 2021 at 6:28 AM Ævar Arnfjörð Bjarmason
> > <avarab@gmail.com> wrote:
> >>
> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> >> ---
> >>  Documentation/git-ls-files.txt           |  4 ++
> >>  builtin/ls-files.c                       | 10 ++++-
> >>  t/t1091-sparse-checkout-builtin.sh       |  9 ++--
> >>  t/t1092-sparse-checkout-compatibility.sh | 57 ++++++++++++++++--------
> >>  4 files changed, 56 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
> >> index 6d11ab506b..1145e960a4 100644
> >> --- a/Documentation/git-ls-files.txt
> >> +++ b/Documentation/git-ls-files.txt
> >> @@ -71,6 +71,10 @@ OPTIONS
> >>  --unmerged::
> >>         Show unmerged files in the output (forces --stage)
> >>
> >> +--sparse::
> >> +       Show sparse directories in the output instead of expanding
> >> +       them (forces --stage)
> >> +
> >>  -k::
> >>  --killed::
> >>         Show files on the filesystem that need to be removed due
> >> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> >> index 4db75351f2..1ebbb63c10 100644
> >> --- a/builtin/ls-files.c
> >> +++ b/builtin/ls-files.c
> >> @@ -26,6 +26,7 @@ static int show_deleted;
> >>  static int show_cached;
> >>  static int show_others;
> >>  static int show_stage;
> >> +static int show_sparse;
> >>  static int show_unmerged;
> >>  static int show_resolve_undo;
> >>  static int show_modified;
> >> @@ -639,6 +640,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
> >>                         DIR_SHOW_IGNORED),
> >>                 OPT_BOOL('s', "stage", &show_stage,
> >>                         N_("show staged contents' object name in the output")),
> >> +               OPT_BOOL(0, "sparse", &show_sparse,
> >> +                       N_("show unexpanded sparse directories in the output")),
> >>                 OPT_BOOL('k', "killed", &show_killed,
> >>                         N_("show files on the filesystem that need to be removed")),
> >>                 OPT_BIT(0, "directory", &dir.flags,
> >> @@ -705,12 +708,17 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
> >>                 tag_skip_worktree = "S ";
> >>                 tag_resolve_undo = "U ";
> >>         }
> >> +       if (show_sparse) {
> >> +               prepare_repo_settings(the_repository);
> >> +               the_repository->settings.command_requires_full_index = 0;
> >> +       }
> >>         if (show_modified || show_others || show_deleted || (dir.flags & DIR_SHOW_IGNORED) || show_killed)
> >>                 require_work_tree = 1;
> >> -       if (show_unmerged)
> >> +       if (show_unmerged || show_sparse)
> >>                 /*
> >>                  * There's no point in showing unmerged unless
> >>                  * you also show the stage information.
> >> +                * The same goes for the --sparse option.
> >
> > Yuck, haven't you just made --sparse an alias for --stage?  Why does
> > it need an alias?
>
> It doesn't, but --unmerged, the one other option which purely modifies
> --stage output implies --stage.

--unmerged modifies --stage output.  --sparse won't.  (Maybe it does
_now_ because the command doesn't yet support sparse-indexes, but
that's a temporary artifact.  Long term, there should be no difference
in the output.)

> So it's in line with existing UI convention in the command, it's
> probably better to keep following that than have new options behave
> differently.
>
> But yeah, we could spell out --stage --sparse in the tests.

There should not be a --sparse option.  The index is _already_ sparse
and users had to take multiple steps to make it so; users shouldn't
have to repeat themselves with each and every command they ever type
when they've created a sparse index that they want sparse behavior.

You should just spell it "--stage".

> > Was the goal just to get a quick way to make the command run under
> > repo->settings.command_requires_full_index = 0 without auditing the
> > codepaths?  It seems to rely on them having been audited anyway, since
> > it just falls back to the code used for --stage, so I don't see how it
> > helps.  It also suggests the command might do unexpected or weird
> > things if run without the --sparse option?  If people manually
> > configure a sparse-checkout and cone mode AND a sparse-index (it's
> > annoying how they have to specify all three instead of having to just
> > pass one flag somewhere), then now we also need to force them to
> > remember to pass extra flags to random various commands for them to
> > operate in a sane manner in their environment??
> >
> > I think this is a bad path to go down.
>
> Those are probably good points, I don't have enough overview of the
> whole sparse thing yet to say.
>
> I just thought it didn't make sense to have a series changing the nature
> of the index without corresponding tooling changes to interrogate the
> state of the index.

That makes sense to me; I agree with you on that point.

> > However, if you want to write the necessary tests to make it so that
> > ls-files can operate with command_requires_full_index = 0, then I
> > think that's useful.  If you want to add a special flag so that folks
> > in a sparse-checkout-with-cone-mode-with-sparse-index setup want to
> > operate densely (i.e. to show what files would be in the index if it
> > were fully populated), then I think that's useful.  But having
> > sparse-yes-with-cone-yes-very-sparse folks need to specify an extra
> > flag to commands to get sparse behavior just seems wrong to me.
>
> Maybe, but what else do you suggest for getting this information out of
> the index?

Use git ls-files without new options...as I stated here:

...
> > I do like the tests and your idea that we can use ls-files to list
> > whatever entries are in the index, I just think the tests should use
> > --stage to do that.

In other words, I think making "git ls-files" the first, or at least
one of the first, commands to be modified to behave properly in a
sparse-index world is what you should be aiming for, not some
new-option-shortcut that'll make no sense long term and persist
indefinitely.

List the entries in the index: `git ls-files`
List the entries in the index with their hash, mode, and stage: `git
ls-files --stage`

List all the entries that would be in the index if it weren't sparse:
`git ls-files --$SOME_NEW_OPTION_NAME`

You don't need to implement the --$SOME_NEW_OPTION_NAME yet, of
course.  We can just note that it's the plan to add it later.