Re: [PATCH 2/9] sparse-checkout: create 'init' subcommand
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Sep 11, 2019, 20:28 UTC
- Message-ID
- <ab98c292-a1b1-ff47-2e8e-f74b7ade2b94@gmail.com>
- In-Reply-To
- <CABPp-BEmwSwg4tgJg6nVG8a3Hpn_g-=ZjApZF4EiJO+qVgu4uw@mail.gmail.com>
On 8/23/2019 7:02 PM, Elijah Newren wrote:
Show 20 quoted lines
> 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.
Show 12 quoted lines
>
>> +
>> + 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.
Show 5 quoted lines
> > 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.
Show 29 quoted lines
>
>> +
>> + 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.
Show 28 quoted lines
>
>> + 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).
Show 36 quoted lines
>> +
>> + /* 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!)
Show 7 quoted lines
>> + >> +reset_dir: >> + return sc_read_tree(); >> +} >> + > > The rest looks fine.
Thanks, -Stolee