Re: [PATCH v4 4/5] sparse-checkout: set worktree-config correctly

[Derrick Stolee <stolee@gmail.com>]

On 1/27/2022 2:15 AM, Elijah Newren wrote:
> On Tue, Jan 25, 2022 at 10:42 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>  'set'::
>> -       Enable the necessary config settings
>> -       (extensions.worktreeConfig, core.sparseCheckout,
>> -       core.sparseCheckoutCone) if they are not already enabled, and
>> -       write a set of patterns to the sparse-checkout file from the
>> -       list of arguments following the 'set' subcommand. Update the
>> -       working directory to match the new patterns.
>> +       Enable the necessary sparse-checkout config settings
>> +       (`core.sparseCheckout` and possibly `core.sparseCheckoutCone`) if
> 
> and possibly index.sparse as well, right?

Good catch.

>> +       they are not already enabled, and write a set of patterns to the
>> +       sparse-checkout file from the list of arguments following the
>> +       'set' subcommand. Update the working directory to match the new
>> +       patterns.
>> ++
>> +To ensure that adjusting the sparse-checkout settings within a worktree
>> +does not alter the sparse-checkout settings in other worktrees, the 'set'
>> +subcommand will upgrade your repository config to use worktree-specific
>> +config if not already present. The sparsity defined by the arguments to
>> +the 'set' subcommand are stored in the worktree-specific sparse-checkout
>> +file. See linkgit:git-worktree[1] and the documentation of
>> +`extensions.worktreeConfig` in linkgit:git-config[1] for more details.
>>  +
>>  When the `--stdin` option is provided, the patterns are read from
>>  standard in as a newline-delimited list instead of from the arguments.
>> @@ -73,7 +81,9 @@ interact with your repository until it is disabled.
>>         By default, these patterns are read from the command-line arguments,
>>         but they can be read from stdin using the `--stdin` option. When
>>         `core.sparseCheckoutCone` is enabled, the given patterns are interpreted
>> -       as directory names as in the 'set' subcommand.
>> +       as directory names as in the 'set' subcommand. The sparsity defined
>> +       by the arguments to the 'add' subcommand are added to the patterns
>> +       in the worktree-specific sparse-checkout file.
> 
> This sentence addition makes your series conflict with patch 4 of my
> en/present-despite-skipped series.
> 
> The sentence also seems somewhat redundant with the first sentence of
> the paragraph (not quoted here).  Perhaps consider just dropping it to
> make it easier for Junio to integrate?

I'll just drop it. Thanks.

>>  'reapply'::
>>         Reapply the sparsity pattern rules to paths in the working tree.
>> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
>> index 679c1070368..314c8d61f80 100644
>> --- a/builtin/sparse-checkout.c
>> +++ b/builtin/sparse-checkout.c
>> @@ -15,6 +15,7 @@
>>  #include "wt-status.h"
>>  #include "quote.h"
>>  #include "sparse-index.h"
>> +#include "worktree.h"
>>
>>  static const char *empty_base = "";
>>
>> @@ -359,26 +360,23 @@ enum sparse_checkout_mode {
>>
>>  static int set_config(enum sparse_checkout_mode mode)
>>  {
>> -       const char *config_path;
>> -
>> -       if (upgrade_repository_format(1) < 0)
>> -               die(_("unable to upgrade repository format to enable worktreeConfig"));
> 
> Wait, this got added in mid-2020, since v2.28.0 -- and I missed it but
> it hasn't bit us yet??
> 
> I'm so sorry.  Earlier when I objected to setting this, I was worried
> it was a new change and would introduce some new failures, but clearly
> it was already introduced...and it turns out we've been fine.  So, I
> was wrong to worry about this.
> 
> (Which is to say, if you want to add it back, I'm fine with it given
> this info.  If you'd rather still take it out, I'm fine with that
> too.)

Yes, this has been around for a while. However, based on our discussion on
this topic, this was never necessary for the worktreeConfig extension.

Removing it seems to increase our compatibility, but it would be interesting
if we catch new compatibility problems. I'm thinking specifically about
third-party tools that should complain about extensions.worktreeConfig and
specifically reading sparse-checkout settings from worktree config files.

> Patch looks good; I only had a few very minor comments.

Thanks,
-Stolee