Re: [PATCH v3 6/6] worktree: copy sparse-checkout patterns and config on add

[Derrick Stolee <stolee@gmail.com>]

On 12/29/2021 1:37 AM, Eric Sunshine wrote:
> On Tue, Dec 28, 2021 at 4:32 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> When adding a new worktree, it is reasonable to expect that we want to
>> use the current set of sparse-checkout settings for that new worktree.
>> This is particularly important for repositories where the worktree would
>> become too large to be useful. This is even more important when using
>> partial clone as well, since we want to avoid downloading the missing
>> blobs for files that should not be written to the new worktree.
>>
>> The only way to create such a worktree without this intermediate step of
>> expanding the full worktree is to copy the sparse-checkout patterns and
>> config settings during 'git worktree add'. Each worktree has its own
>> sparse-checkout patterns, and the default behavior when the
>> sparse-checkout file is missing is to include all paths at HEAD. Thus,
>> we need to have patterns from somewhere, they might as well be the
>> current worktree's patterns. These are then modified independently in
>> the future.
>>
>> In addition to the sparse-checkout file, copy the worktree config file
>> if worktree config is enabled and the file exists. This will copy over
>> any important settings to ensure the new worktree behaves the same as
>> the current one.
> 
> This is not a proper review. I just happened to very quickly scan my
> eyes over this patch without even having looked at any of the others,
> nor have I read the v3 cover letter closely yet. Nevertheless, while
> skimming this patch, an issue jumped out at me...
> 
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> @@ -336,6 +336,47 @@ static int add_worktree(const char *path, const char *refname,
>> +       /*
>> +        * If we are using worktree config, then copy all currenct config
>> +        * values from the current worktree into the new one, that way the
>> +        * new worktree behaves the same as this one.
>> +        */
> 
> s/currenct/current/
> 
>> +       if (repository_format_worktree_config) {
>> +               char *from_file = git_pathdup("config.worktree");
>> +               char *to_file = xstrfmt("%s/worktrees/%s/config.worktree",
>> +                                       realpath.buf, name);
>> +
>> +               if (file_exists(from_file)) {
>> +                       if (safe_create_leading_directories(to_file) ||
>> +                           copy_file(to_file, from_file, 0666))
>> +                               error(_("failed to copy worktree config from '%s' to '%s'"),
>> +                                     from_file, to_file);
>> +               }
> 
> I presume that you lifted this idea from [1] in which I offhandedly
> mentioned that one possible way to implement Elijah's desire to copy
> sparse-checkout configuration when a new worktree is created would be
> to simply copy the existing worktree-specific configuration to the new
> worktree. Unfortunately, a direct implementation of that idea suffers
> the same problem which started this entire thread. Namely:
> 
>     % git clone --bare <url>/bare.git
>     % cd bare.git/
>     % git worktree init-worktree-config
>     % git worktree add -d ../foo
>     Preparing worktree (detached HEAD a0df8ce)
>     HEAD is now at a0df8ce gobbledygook
>     % cd ../foo/
>     % git sparse-checkout init
>     fatal: this operation must be run in a work tree
>     % git config --get --show-origin --show-scope core.bare
>     worktree file:.../bare.git/worktrees/foo/config.worktree true
> 
> The problem is that for a bare repository, after `git worktree
> init-worktree-config`, "bare.git/config.worktree" contains the
> repo-specific `core.bare=true` setting, so copying
> "bare.git/config.worktree" to
> "bare.git/worktrees/<id>/config.worktree" verbatim has undesired
> consequences.

It is certainly unfortunate that this can happen when core.bare or
core.worktree are set in the config.worktree of the bare repo.

The thing we are doing here is trying to create a worktree that exactly
matches the current worktree (even if it is bare or redirected to a
different working directory), but since we don't actually support a
"bare" worktree this does not work.

> The obvious way to work around this problem is to (again) special-case
> `core.bare` and `core.worktree` to remove them when copying the
> worktree-specific configuration. Whether or not that is the best
> solution or even desirable is a different question. (I haven't thought
> it through enough to have an opinion.)

It makes sense to special case these two settings since we want to
allow creating a working worktree from a bare repo, even if it has
worktree config stating that it is bare.

As far as the implementation goes, we could do the copy and then
unset those two values in the new file. That's an easy enough change.
I'll wait for more feedback on the overall ideas (and names of things
like the init-worktree-config subcommand).

Thanks,
-Stolee