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

[Taylor Blau <me@ttaylorr.com>]

On Mon, May 04, 2020 at 11:41:38AM -0700, Elijah Newren wrote:
> 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.

To me as well. Thanks for sending something so quickly after Lars'
initial report.

  Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks,
Taylor