On 10/11/2019 6:14 PM, Elijah Newren wrote:
> On Mon, Oct 7, 2019 at 1:08 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@xxxxxxxxx> 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.
> 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?
>> +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
