Re: [PATCH v3 02/17] sparse-checkout: create 'init' subcommand
- From
Elijah Newren <newren@gmail.com>
- Date
- Oct 11, 2019, 22:14 UTC
- Message-ID
- <CABPp-BHd9sJ=HcM-VQaGpd5TV-A0JL5NWG6zdDReUJrMnE+FwA@mail.gmail.com>
- In-Reply-To
- <08bb6fb7f31b92395df4db10bf47e9a9c01257f4.1570478905.git.gitgitgadget@gmail.com>
On Mon, Oct 7, 2019 at 1:08 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:
Show 5 quoted lines
> ++ > +The init subcommand also enables the 'extensions.worktreeConfig' setting > +and sets the `core.sparseCheckout` setting in the worktree-specific config > +file. This prevents the sparse-checkout feature from interfering with other > +worktrees.
I'm afraid that might be mis-parsed by future readers. Perhaps something like:
The init subcommand also enables the `core.sparseCheckout` setting. To avoid interfering with other worktrees, it first enables the `extensions.worktreeConfig` setting and makes sure to set the `core.sparseCheckout` setting in the worktree-specific config file.
> +enum sparse_checkout_mode {
> + MODE_NONE = 0,
> + MODE_FULL = 1,
> +};So MODE_FULL is "true" and MODE_NONE is "false". MODE_NONE seems confusing to me, but let's keep reading...
Show 16 quoted lines
> +
> +static int sc_set_config(enum sparse_checkout_mode mode)
> +{
> + struct argv_array argv = ARGV_ARRAY_INIT;
> +
> + if (git_config_set_gently("extensions.worktreeConfig", "true")) {
> + error(_("failed to set extensions.worktreeConfig setting"));
> + return 1;
> + }
> +
> + argv_array_pushl(&argv, "config", "--worktree", "core.sparseCheckout", NULL);
> +
> + if (mode)
> + argv_array_pushl(&argv, "true", NULL);
> + else
> + argv_array_pushl(&argv, "false", NULL);Wait, what? MODE_FULL is used to specify that you want a sparse checkout, and MODE_NONE is used to denote that you want a full (i.e. non-sparse) checkout? These are *very* confusing names.
Show 9 quoted lines
> +static int sparse_checkout_init(int argc, const char **argv)
> +{
> + struct pattern_list pl;
> + char *sparse_filename;
> + FILE *fp;
> + int res;
> +
> + if (sc_set_config(MODE_FULL))
> + return 1;Seems confusing here too.
Everything else in the patch looks good, though.