Re: [PATCH v3 02/17] sparse-checkout: create 'init' subcommand
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Oct 14, 2019, 20:22 UTC
- Message-ID
- <91b9126d-8b50-8648-d5c1-25e4c6c68f8b@gmail.com>
- In-Reply-To
- <CABPp-BHd9sJ=HcM-VQaGpd5TV-A0JL5NWG6zdDReUJrMnE+FwA@mail.gmail.com>
On 10/11/2019 6:14 PM, Elijah Newren wrote:
Show 11 quoted lines
> On Mon, Oct 7, 2019 at 1:08 PM Derrick Stolee via GitGitGadget > <gitgitgadget@gmail.com> wrote: >> ++ >> +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.
I like the paragraph below, but the sentence above is repeated from the earlier paragraph.
Show 32 quoted lines
> 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...
>
>> +
>> +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.I understand they are confusing, hopefully it makes more sense with the cone mode later.
* NONE == "No patterns at all" * FULL == "all patterns allowed" * CONE == "only cone patterns" (appears later)
Since this is just an internal detail, what if I switched it to
* MODE_NO_PATTERNS * MODE_ALL_PATTERNS * MODE_CONE_PATTERNS
Would that make more sense?
Show 14 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.Thanks, -Stolee