Re: [PATCH v2 3/8] sparse-checkout: match some 'clean' behavior

>
> From: Derrick Stolee <stolee@gmail.com>
>
> The 'git sparse-checkout clean' subcommand is somewhat similar to 'git
> clean' in that it will delete files that should not be in the worktree.
> The big difference is that it focuses on the directories that should not
> be in the worktree due to cone-mode sparse-checkout. It also does not
> discriminate in the kinds of files and focuses on deleting entire
> directories.
>
> However, there are some restrictions that would be good to bring over
> from 'git clean', specifically how it refuses to do anything without the
> '-f'/'--force' or '-n'/'--dry-run' arguments. The 'clean.requireForce'
> config can be set to 'false' to imply '--force'.
>
> Add this behavior to avoid accidental deletion of files that cannot be
> recovered from Git.

> Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---
>  Documentation/git-sparse-checkout.adoc |  9 ++++
>  builtin/sparse-checkout.c              | 15 +++++-
>  t/t1091-sparse-checkout-builtin.sh     | 66 +++++++++++++++++++++++++-
>  3 files changed, 87 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/git-sparse-checkout.adoc b/Documentation/git-sparse-checkout.adoc
> index 6db88f00781d..823a66c40bc5 100644
> --- a/Documentation/git-sparse-checkout.adoc
> +++ b/Documentation/git-sparse-checkout.adoc
> @@ -119,6 +119,15 @@ all sparsity paths.
>         This command can be used to be sure the sparse index works
>         efficiently, though it does not require enabling the sparse index
>    feature via the `index.sparse=true` configuration.
> ++
> +To prevent accidental deletion of worktree files, the `clean` subcommand
> +will not delete any files without the `-f` or `--force` option, unless
> +the `clean.requireForce` config option is set to `false`.
> ++
> +The `--dry-run` option will list the directories that would be removed
> +without deleting them. Running in this mode can be helpful to predict the
> +behavior of the clean comand or to determine which kinds of files are left
> +in the sparse directories.
>
>  'disable'::
>         Disable the `core.sparseCheckout` config setting, and restore the
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index 6fe6ec718fe3..fe332ff5f941 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -931,6 +931,7 @@ static char const * const builtin_sparse_checkout_clean_usage[] = {
>  };
>
>  static const char *msg_remove = N_("Removing %s\n");
> +static const char *msg_would_remove = N_("Would remove %s\n");
>
>  static int sparse_checkout_clean(int argc, const char **argv,
>                                    const char *prefix,
> @@ -939,8 +940,12 @@ static int sparse_checkout_clean(int argc, const char **argv,
>         struct strbuf full_path = STRBUF_INIT;
>         const char *msg = msg_remove;
>         size_t worktree_len;
> +       int force = 0, dry_run = 0;
> +       int require_force = 1;
>
>         struct option builtin_sparse_checkout_clean_options[] = {
> +               OPT__DRY_RUN(&dry_run, N_("dry run")),
> +               OPT__FORCE(&force, N_("force"), PARSE_OPT_NOCOMPLETE),
>                 OPT_END(),
>         };
>
> @@ -954,6 +959,13 @@ static int sparse_checkout_clean(int argc, const char **argv,
>                              builtin_sparse_checkout_clean_options,
>                              builtin_sparse_checkout_clean_usage, 0);
>
> +       repo_config_get_bool(repo, "clean.requireforce", &require_force);
> +       if (require_force && !force && !dry_run)
> +               die(_("for safety, refusing to clean without one of --force or --dry-run"));
> +
> +       if (dry_run)
> +               msg = msg_would_remove;
> +
>         if (repo_read_index(repo) < 0)
>                 die(_("failed to read index"));
>
> @@ -977,7 +989,8 @@ static int sparse_checkout_clean(int argc, const char **argv,
>
>                 printf(msg, ce->name);
>
> -               if (remove_dir_recursively(&full_path, 0))
> +               if (dry_run <= 0 &&
> +                   remove_dir_recursively(&full_path, 0))
>                         warning_errno(_("failed to remove '%s'"), ce->name);
>         }
>
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index a48eedf766d2..69f5a6dcc689 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -1056,12 +1056,29 @@ test_expect_success 'clean' '
>         touch repo/deep/deeper2/file &&
>         touch repo/folder1/file &&
>
> +       test_must_fail git -C repo sparse-checkout clean 2>err &&
> +       grep "refusing to clean" err &&
> +
> +       git -C repo config clean.requireForce true &&
> +       test_must_fail git -C repo sparse-checkout clean 2>err &&
> +       grep "refusing to clean" err &&
> +
> +       cat >expect <<-\EOF &&
> +       Would remove deep/deeper2/
> +       Would remove folder1/
> +       EOF
> +
> +       git -C repo sparse-checkout clean --dry-run >out &&
> +       test_cmp expect out &&
> +       test_path_exists repo/deep/deeper2 &&
> +       test_path_exists repo/folder1 &&
> +
>         cat >expect <<-\EOF &&
>         Removing deep/deeper2/
>         Removing folder1/
>         EOF
>
> -       git -C repo sparse-checkout clean >out &&
> +       git -C repo sparse-checkout clean -f >out &&
>         test_cmp expect out &&
>
>         test_path_is_missing repo/deep/deeper2 &&
> @@ -1077,16 +1094,61 @@ test_expect_success 'clean with staged sparse change' '
>
>         git -C repo add --sparse folder1/file &&
>
> +       cat >expect <<-\EOF &&
> +       Would remove deep/deeper2/
> +       EOF
> +
> +       git -C repo sparse-checkout clean --dry-run >out &&
> +       test_cmp expect out &&
> +       test_path_exists repo/deep/deeper2 &&
> +       test_path_exists repo/folder1 &&
> +       test_path_exists repo/folder2 &&
> +
>         # deletes deep/deeper2/ but leaves folder1/ and folder2/
>         cat >expect <<-\EOF &&
>         Removing deep/deeper2/
>         EOF
>
> +       # The previous test case checked the -f option, so
> +       # test the config option in this one.
> +       git -C repo config clean.requireForce false &&
>         git -C repo sparse-checkout clean >out &&
>         test_cmp expect out &&
>
>         test_path_is_missing repo/deep/deeper2 &&
> -       test_path_exists repo/folder1
> +       test_path_exists repo/folder1 &&
> +       test_path_exists repo/folder2
> +'
> +
> +test_expect_success 'clean with merge conflict status' '
> +       git clone repo clean-merge &&
> +
> +       echo dirty >clean-merge/deep/deeper2/a &&
> +       touch clean-merge/folder2/extra &&
> +
> +       cat >input <<-EOF &&
> +       0 $ZERO_OID     folder1/a
> +       100644 $(git -C clean-merge rev-parse HEAD:folder1/a) 1 folder1/a
> +       EOF
> +       git -C clean-merge update-index --index-info <input &&
> +
> +       git -C clean-merge sparse-checkout set deep/deeper1 &&
> +
> +       test_must_fail git -C clean-merge sparse-checkout clean -f 2>err &&
> +       grep "failed to convert index to a sparse index" err &&

> +       mkdir -p clean-merge/folder1/ &&
> +       echo merged >clean-merge/folder1/a &&
> +       git -C clean-merge add --sparse folder1/a &&
> +
> +       # deletes folder2/ but leaves staged change in folder1
> +       # and dirty change in deep/deeper2/
> +       cat >expect <<-\EOF &&
> +       Removing folder2/
> +       EOF
> +
> +       git -C clean-merge sparse-checkout clean -f >out &&
> +       test_cmp expect out
>  '
>
>  test_done
> --
> gitgitgadget