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

On 12/29/2021 1:37 AM, Eric Sunshine wrote:

On Tue, Dec 28, 2021 at 4:32 PM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

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...

quoted

Signed-off-by: Derrick Stolee <redacted>
---

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/

quoted

+       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