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

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.

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.

>                  */
>                 show_stage = 1;
>         if (show_tag || show_stage)
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index ff1ad570a2..c823df423c 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -208,12 +208,13 @@ test_expect_success 'sparse-checkout disable' '
>  test_expect_success 'sparse-index enabled and disabled' '
>         git -C repo sparse-checkout init --cone --sparse-index &&
>         test_cmp_config -C repo true extensions.sparseIndex &&
> -       test-tool -C repo read-cache --table >cache &&
> -       grep " tree " cache &&
> +       git -C repo ls-files --sparse >cache &&
> +       grep "^040000 " cache >lines &&
> +       test_line_count = 3 lines &&
>
>         git -C repo sparse-checkout disable &&
> -       test-tool -C repo read-cache --table >cache &&
> -       ! grep " tree " cache &&
> +       git -C repo ls-files --sparse >cache &&
> +       ! grep "^040000 " cache &&
>         git -C repo config --list >config &&
>         ! grep extensions.sparseindex config
>  '
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index d97bf9b645..48d3920490 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -136,48 +136,67 @@ test_sparse_match () {
>         test_cmp sparse-checkout-err sparse-index-err
>  }
>
> +test_index_entry_like () {
> +       dir=$1
> +       shift
> +       fmt=$1
> +       shift
> +       rev=$1
> +       shift
> +       entry=$1
> +       shift
> +       file=$1
> +       shift
> +       hash=$(git -C "$dir" rev-parse "$rev") &&
> +       printf "$fmt\n" "$hash" "$entry" >expected &&
> +       if grep "$entry" "$file" >line
> +       then
> +               test_cmp expected line
> +       else
> +               cat cache &&
> +               false
> +       fi
> +}
> +
>  test_expect_success 'sparse-index contents' '
>         init_repos &&
>
> -       test-tool -C sparse-index read-cache --table >cache &&
> +       git -C sparse-index ls-files --sparse >cache &&
>         for dir in folder1 folder2 x
>         do
> -               TREE=$(git -C sparse-index rev-parse HEAD:$dir) &&
> -               grep "040000 tree $TREE $dir/" cache \
> -                       || return 1
> +               test_index_entry_like sparse-index "040000 %s 0\t%s" "HEAD:$dir" "$dir/" cache || return 1
>         done &&
>
>         git -C sparse-index sparse-checkout set folder1 &&
>
> -       test-tool -C sparse-index read-cache --table >cache &&
> +       git -C sparse-index ls-files --sparse >cache &&
>         for dir in deep folder2 x
>         do
> -               TREE=$(git -C sparse-index rev-parse HEAD:$dir) &&
> -               grep "040000 tree $TREE $dir/" cache \
> -                       || return 1
> +               test_index_entry_like sparse-index "040000 %s 0\t%s" "HEAD:$dir" "$dir/" cache || return 1
>         done &&
>
>         git -C sparse-index sparse-checkout set deep/deeper1 &&
>
> -       test-tool -C sparse-index read-cache --table >cache &&
> +       git -C sparse-index ls-files --sparse >cache &&
>         for dir in deep/deeper2 folder1 folder2 x
>         do
> -               TREE=$(git -C sparse-index rev-parse HEAD:$dir) &&
> -               grep "040000 tree $TREE $dir/" cache \
> -                       || return 1
> +               test_index_entry_like sparse-index "040000 %s 0\t%s" "HEAD:$dir" "$dir/" cache || return 1
>         done &&
>
> +       grep 040000 cache >lines &&
> +       test_line_count = 4 lines &&
> +
>         # Disabling the sparse-index removes tree entries with full ones
>         git -C sparse-index sparse-checkout init --no-sparse-index &&
>
> -       test-tool -C sparse-index read-cache --table >cache &&
> -       ! grep "040000 tree" cache &&
> -       test_sparse_match test-tool read-cache --table
> +       git -C sparse-index ls-files --sparse >cache &&
> +       ! grep "^040000 " cache >lines &&
> +       test_sparse_match git ls-tree -r HEAD
>  '
>
>  test_expect_success 'expanded in-memory index matches full index' '
>         init_repos &&
> -       test_sparse_match test-tool read-cache --expand --table
> +       test_sparse_match git ls-tree -r HEAD
>  '
>
>  test_expect_success 'status with options' '
> @@ -394,9 +413,9 @@ test_expect_success 'submodule handling' '
>         test_all_match git commit -m "add submodule" &&
>
>         # having a submodule prevents "modules" from collapse
> -       test-tool -C sparse-index read-cache --table >cache &&
> -       grep "100644 blob .*    modules/a" cache &&
> -       grep "160000 commit $(git -C initial-repo rev-parse HEAD)       modules/sub" cache
> +       git -C sparse-index ls-files --sparse >cache &&
> +       test_index_entry_like sparse-index "100644 %s 0\t%s" "HEAD:modules/a" "modules/a" cache &&
> +       test_index_entry_like sparse-index "160000 %s 0\t%s" "HEAD:modules/sub" "modules/sub" cache
>  '
>
>  test_expect_success 'sparse-index is expanded and converted back' '
> --
> 2.31.0.260.g719c683c1d

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.