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

From: Derrick Stolee <stolee@gmail.com>
To: Elijah Newren <newren@gmail.com>,
	Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Junio C Hamano <gitster@pobox.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 2/9] sparse-checkout: create 'init' subcommand
Date: Wed, 11 Sep 2019 16:28:45 -0400	[thread overview]
Message-ID: <ab98c292-a1b1-ff47-2e8e-f74b7ade2b94@gmail.com> (raw)
In-Reply-To: <CABPp-BEmwSwg4tgJg6nVG8a3Hpn_g-=ZjApZF4EiJO+qVgu4uw@mail.gmail.com>

On 8/23/2019 7:02 PM, Elijah Newren wrote:
> On Tue, Aug 20, 2019 at 8:13 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> +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.

I agree that this is not the _best_ thing to do, but it does mimic the
current recommendation for a user interacting with sparse-checkout.

I'll rename this helper to something like "update_working_directory()"
so it can be swapped with a different implementation later, after we
work out those usability kinks.

The other thing that is needed here: allow reverting the sparse-checkout
settings if this fails. I'll isolate that to a new commit so we can
examine that behavior carefully.

> 
>> +
>> +       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.

Yeah, that's a mistake. Good find.

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

I was planning to switch my `git config` subcommand to use in-process
methods, but I'm struggling to find a way to ensure we follow the
`--worktree` option. It likely would work if extensions.worktreeConfig
was enabled when the process starts, but adding it in-process likely
causes a problem.

> 
>> +
>> +       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.
This whole thing isn't needed any more, since read-tree does the right
thing.

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

This is the only way I could think to check that the sparse-checkout file parses well without just doing the file open myself. Maybe we only need to check if the file exists (and is not empty).

>> +
>> +       /* 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"?

(this will not be needed, but thanks!)

>> +
>> +reset_dir:
>> +       return sc_read_tree();
>> +}
>> +
> 
> The rest looks fine.

Thanks,
-Stolee