Re: [PATCH 2/9] sparse-checkout: create 'init' subcommand

[Elijah Newren <newren@gmail.com>]

On Tue, Aug 20, 2019 at 8:13 AM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Derrick Stolee <dstolee@microsoft.com>
>
> Getting started with a sparse-checkout file can be daunting. Help
> users start their sparse enlistment using 'git sparse-checkout init'.
> This will set 'core.sparseCheckout=true' in their config, write
> an initial set of patterns to the sparse-checkout file, and update
> their working directory.
>
> Using 'git read-tree' to clear directories does not work cleanly
> on Windows, so manually delete directories that are tracked by Git
> before running read-tree.

Is that a bug in read-tree that needs to be fixed?

> The use of running another process for 'git read-tree' is likely
> suboptimal, but that can be improved in a later change, if valuable.

I think it's valuable.  The bigger problem may be that "git read-tree
-mu HEAD" may turn out to be insufficient for our needs; see below....

>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  Documentation/git-sparse-checkout.txt |   7 ++
>  builtin/sparse-checkout.c             | 106 +++++++++++++++++++++++++-
>  t/t1091-sparse-checkout-builtin.sh    |  40 ++++++++++
>  3 files changed, 152 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
> index ca0ca6a12f..50c53ee60a 100644
> --- a/Documentation/git-sparse-checkout.txt
> +++ b/Documentation/git-sparse-checkout.txt
> @@ -26,6 +26,13 @@ COMMANDS
>  'list'::
>         Provide a list of the contents in the sparse-checkout file.
>
> +'init'::
> +       Enable the `core.sparseCheckout` setting. If the
> +       sparse-checkout file does not exist, then populate it with
> +       patterns that match every file in the root directory and
> +       no other directories, then will remove all directories tracked
> +       by Git. Add patterns to the sparse-checkout file to
> +       repopulate the working directory.
>
>  SPARSE CHECKOUT
>  ----------------
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index 6477a6ed9c..86d24e6295 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -8,7 +8,7 @@
>  #include "strbuf.h"
>
>  static char const * const builtin_sparse_checkout_usage[] = {
> -       N_("git sparse-checkout [list]"),
> +       N_("git sparse-checkout [init|list]"),
>         NULL
>  };
>
> @@ -64,6 +64,108 @@ static int sparse_checkout_list(int argc, const char **argv)
>         return 0;
>  }
>
> +static int sc_read_tree(void)
> +{
> +       struct argv_array argv = ARGV_ARRAY_INIT;
> +       int result = 0;
> +       argv_array_pushl(&argv, "read-tree", "-m", "-u", "HEAD", NULL);
> +
> +       if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
> +               error(_("failed to update index with new sparse-checkout paths"));
> +               result = 1;
> +       }

`git read-tree -m -u HEAD` will fail if the index has any higher stage
entries in it, even if those higher stage entries correspond to files
which are included in the sparseness patterns and thus would not need
an update.  It might be nice if we can find a way to provide a better
error message, and/or implement the read-tree -m -u HEAD internally in
a way that will allow us to not fail if the conflicted files are
included in the sparse set.

> +
> +       argv_array_clear(&argv);
> +       return result;
> +}
> +
> +static int sc_enable_config(void)
> +{
> +       struct argv_array argv = ARGV_ARRAY_INIT;
> +       int result = 0;
> +       argv_array_pushl(&argv, "config", "--add", "core.sparseCheckout", "true", NULL);

Why --add?  That seems really odd to me.

This should also have "--worktree".  And this function should either
set extensions.worktreeConfig to true or die if it isn't already set;
not sure which.  There's some UI and documentation stuff to figure out
here...

> +
> +       if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
> +               error(_("failed to enable core.sparseCheckout"));
> +               result = 1;
> +       }
> +
> +       argv_array_clear(&argv);
> +       return result;
> +}
> +
> +static int delete_directory(const struct object_id *oid, struct strbuf *base,
> +               const char *pathname, unsigned mode, int stage, void *context)
> +{
> +       struct strbuf dirname = STRBUF_INIT;
> +       struct stat sb;
> +
> +       strbuf_addstr(&dirname, the_repository->worktree);
> +       strbuf_addch(&dirname, '/');
> +       strbuf_addstr(&dirname, pathname);
> +
> +       if (stat(dirname.buf, &sb) || !(sb.st_mode & S_IFDIR))
> +               return 0;
> +
> +       if (remove_dir_recursively(&dirname, 0))

flags = 0 implies not REMOVE_DIR_EMPTY_ONLY.  I'm not familiar with
remove_dir_recursively(), but won't this delete everything...including
untracked files?  If so, that sounds like a bug.

> +               warning(_("failed to remove directory '%s'"),
> +                       dirname.buf);
> +
> +       strbuf_release(&dirname);
> +       return 0;
> +}
> +
> +static int sparse_checkout_init(int argc, const char **argv)
> +{
> +       struct tree *t;
> +       struct object_id oid;
> +       struct exclude_list el;
> +       static struct pathspec pathspec;
> +       char *sparse_filename;
> +       FILE *fp;
> +       int res;
> +
> +       if (sc_enable_config())
> +               return 1;
> +
> +       memset(&el, 0, sizeof(el));
> +
> +       sparse_filename = get_sparse_checkout_filename();
> +       res = add_excludes_from_file_to_list(sparse_filename, "", 0, &el, NULL);

But 'el' isn't used again?  Why are we getting the list of files from
sparse_filename then?

> +
> +       /* If we already have a sparse-checkout file, use it. */
> +       if (res >= 0) {
> +               free(sparse_filename);
> +               goto reset_dir;
> +       }
> +
> +       /* initial mode: all blobs at root */
> +       fp = fopen(sparse_filename, "w");
> +       free(sparse_filename);
> +       fprintf(fp, "/*\n!/*/*\n");
> +       fclose(fp);

Makes sense.

> +
> +       /* remove all directories in the root, if tracked by Git */
> +       if (get_oid("HEAD", &oid)) {
> +               /* assume we are in a fresh repo */
> +               return 0;
> +       }
> +
> +       t = parse_tree_indirect(&oid);
> +
> +       parse_pathspec(&pathspec, PATHSPEC_ALL_MAGIC &
> +                                 ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
> +                      PATHSPEC_PREFER_CWD,
> +                      "", NULL);
> +
> +       if (read_tree_recursive(the_repository, t, "", 0, 0, &pathspec,
> +                               delete_directory, NULL))
> +               return 1;

Since this is only needed on Windows, as per your commit message,
should it be #ifdef'd?  Or is this actually a bug that should be fixed
in "git read-tree -mu HEAD"?

> +
> +reset_dir:
> +       return sc_read_tree();
> +}
> +

The rest looks fine.