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