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

On Tue, Dec 28, 2021 at 4:32 PM Derrick Stolee via GitGitGadget
[off-list ref] 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...

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.

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

[1]: https://lore.kernel.org/git/CAPig+cRYKKGA1af4hV0fz_nZWNG=zMgAziuAimDxWTz6L8u3Tg@mail.gmail.com/ (local)