Re: [PATCH] sparse-checkout: stop blocking empty workdirs

[Elijah Newren <newren@gmail.com>]

On Mon, May 4, 2020 at 11:27 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> Remove the error condition when updating the sparse-checkout leaves
> an empty working directory.
>
> This behavior was added in 9e1afb167 (sparse checkout: inhibit empty
> worktree, 2009-08-20). The comment was added in a7bc906f2 (Add
> explanation why we do not allow to sparse checkout to empty working
> tree, 2011-09-22) in response to a "dubious" comment in 84563a624
> (unpack-trees.c: cosmetic fix, 2010-12-22).
>
> With the recent "cone mode" and "git sparse-checkout init [--cone]"
> command, it is common to set a reasonable sparse-checkout pattern
> set of
>
>         /*
>         !/*/
>
> which matches only files at root. If the repository has no such files,
> then their "git sparse-checkout init" command will fail.
>
> Now that we expect this to be a common pattern, we should not have the
> commands fail on an empty working directory. If it is a confusing
> result, then the user can recover with "git sparse-checkout disable"
> or "git sparse-checkout set". This is especially simple when using cone
> mode.

Yeah, given that setting up a sparse-checkout is now easy (as opposed
to setting both extensions.worktreeConfig and core.sparseCheckout
settings, editing a .git/info/sparse-checkout file that is documented
in some obscure section of the manual, and discovering the
undocumented `git read-tree -mu HEAD` command that you need to run)
and drastically less error-prone, and that recovery is now easy (also
in contrast to before), I think this makes a lot of sense.

I actually hit this error a couple times while testing with the old
style and thought it was annoying (though understandable when the
route for usage was so arcane and easy to mess up), but in my case I
wasn't on a real world repository.  If we've got an example of people
hitting it on real world repos, then by all means let's get rid of
this annoying check.

> Reported-by: Lars Schneider <larsxschneider@gmail.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>     sparse-checkout: stop blocking empty workdirs
>
>     This is based on en/sparse-checkout.
>
>     This is something that Lars Schneider discovered working with a repo
>     that had no files at root.
>
>     Thanks, -Stolee
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-624%2Fderrickstolee%2Fsparse-checkout-allow-empty-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-624/derrickstolee/sparse-checkout-allow-empty-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/624
>
>  t/t1011-read-tree-sparse-checkout.sh | 12 +++++++---
>  t/t1091-sparse-checkout-builtin.sh   |  8 +++----
>  unpack-trees.c                       | 34 +---------------------------
>  3 files changed, 13 insertions(+), 41 deletions(-)
>
> diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
> index 63223e13bd1..140f4599773 100755
> --- a/t/t1011-read-tree-sparse-checkout.sh
> +++ b/t/t1011-read-tree-sparse-checkout.sh
> @@ -74,13 +74,19 @@ test_expect_success 'read-tree --no-sparse-checkout with empty .git/info/sparse-
>  test_expect_success 'read-tree with empty .git/info/sparse-checkout' '
>         git config core.sparsecheckout true &&
>         echo >.git/info/sparse-checkout &&
> -       read_tree_u_must_fail -m -u HEAD &&
> +       read_tree_u_must_succeed -m -u HEAD &&
>         git ls-files --stage >result &&
>         test_cmp expected result &&
>         git ls-files -t >result &&
> +       cat >expected.swt <<-\EOF &&
> +       S init.t
> +       S sub/added
> +       S sub/addedtoo
> +       S subsub/added
> +       EOF
>         test_cmp expected.swt result &&
> -       test -f init.t &&
> -       test -f sub/added
> +       ! test -f init.t &&
> +       ! test -f sub/added
>  '
>
>  test_expect_success 'match directories with trailing slash' '
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index dee99eeec30..88cdde255cd 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -106,10 +106,8 @@ test_expect_success 'set enables config' '
>                 cd empty-config &&
>                 test_commit test file &&
>                 test_path_is_missing .git/config.worktree &&
> -               test_must_fail git sparse-checkout set nothing &&
> +               git sparse-checkout set nothing &&
>                 test_path_is_file .git/config.worktree &&
> -               test_must_fail git config core.sparseCheckout &&
> -               git sparse-checkout set "/*" &&
>                 test_cmp_config true core.sparseCheckout
>         )
>  '
> @@ -302,8 +300,8 @@ test_expect_success 'revert to old sparse-checkout on empty update' '
>                 echo >file &&
>                 git add file &&
>                 git commit -m "test" &&
> -               test_must_fail git sparse-checkout set nothing 2>err &&
> -               test_i18ngrep "Sparse checkout leaves no entry on working directory" err &&
> +               git sparse-checkout set nothing 2>err &&
> +               test_i18ngrep ! "Sparse checkout leaves no entry on working directory" err &&
>                 test_i18ngrep ! ".git/index.lock" err &&
>                 git sparse-checkout set file
>         )
> diff --git a/unpack-trees.c b/unpack-trees.c
> index b43f3e775ad..9a3ccd9d083 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -1677,8 +1677,6 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>         }
>
>         if (!o->skip_sparse_checkout) {
> -               int empty_worktree = 1;
> -
>                 /*
>                  * Sparse checkout loop #2: set NEW_SKIP_WORKTREE on entries not in loop #1
>                  * If they will have NEW_SKIP_WORKTREE, also set CE_SKIP_WORKTREE
> @@ -1706,19 +1704,6 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
>
>                         if (apply_sparse_checkout(&o->result, ce, o))
>                                 ret = 1;
> -
> -                       if (!ce_skip_worktree(ce))
> -                               empty_worktree = 0;
> -               }
> -               /*
> -                * Sparse checkout is meant to narrow down checkout area
> -                * but it does not make sense to narrow down to empty working
> -                * tree. This is usually a mistake in sparse checkout rules.
> -                * Do not allow users to do that.
> -                */
> -               if (o->result.cache_nr && empty_worktree) {
> -                       ret = unpack_failed(o, "Sparse checkout leaves no entry on working directory");
> -                       goto done;
>                 }
>                 if (ret == 1) {
>                         /*
> @@ -1779,7 +1764,7 @@ enum update_sparsity_result update_sparsity(struct unpack_trees_options *o)
>  {
>         enum update_sparsity_result ret = UPDATE_SPARSITY_SUCCESS;
>         struct pattern_list pl;
> -       int i, empty_worktree;
> +       int i;
>         unsigned old_show_all_errors;
>         int free_pattern_list = 0;
>
> @@ -1810,7 +1795,6 @@ enum update_sparsity_result update_sparsity(struct unpack_trees_options *o)
>
>         /* Then loop over entries and update/remove as needed */
>         ret = UPDATE_SPARSITY_SUCCESS;
> -       empty_worktree = 1;
>         for (i = 0; i < o->src_index->cache_nr; i++) {
>                 struct cache_entry *ce = o->src_index->cache[i];
>
> @@ -1824,28 +1808,12 @@ enum update_sparsity_result update_sparsity(struct unpack_trees_options *o)
>
>                 if (apply_sparse_checkout(o->src_index, ce, o))
>                         ret = UPDATE_SPARSITY_WARNINGS;
> -
> -               if (!ce_skip_worktree(ce))
> -                       empty_worktree = 0;
> -       }
> -
> -       /*
> -        * Sparse checkout is meant to narrow down checkout area
> -        * but it does not make sense to narrow down to empty working
> -        * tree. This is usually a mistake in sparse checkout rules.
> -        * Do not allow users to do that.
> -        */
> -       if (o->src_index->cache_nr && empty_worktree) {
> -               unpack_failed(o, "Sparse checkout leaves no entry on working directory");
> -               ret = UPDATE_SPARSITY_INDEX_UPDATE_FAILURES;
> -               goto done;
>         }
>
>  skip_sparse_checkout:
>         if (check_updates(o, o->src_index))
>                 ret = UPDATE_SPARSITY_WORKTREE_UPDATE_FAILURES;
>
> -done:
>         display_warning_msgs(o);
>         o->show_all_errors = old_show_all_errors;
>         if (free_pattern_list)
>
> base-commit: 5644ca28cded68972c74614fc06d6e0e1db1a7de

Patch looks good to me.