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

On Wed, Mar 17, 2021 at 11:27 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

On Wed, Mar 17 2021, Elijah Newren wrote:

quoted

On Wed, Mar 17, 2021 at 6:28 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
 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".

quoted

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.

quoted

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:

...

quoted

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.