On Wed, Mar 17, 2021 at 11:27 AM Ævar Arnfjörð Bjarmason
<avarab@xxxxxxxxx> wrote:
>
> On Wed, Mar 17 2021, Elijah Newren wrote:
>
> > On Wed, Mar 17, 2021 at 6:28 AM Ævar Arnfjörð Bjarmason
> > <avarab@xxxxxxxxx> wrote:
> >>
> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
> >> ---
> >> 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.
