Re: [PATCH v2 5/5] sparse-checkout: use repo_config_set_worktree_gently()

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Dec 21, 2021 at 2:14 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> The previous change added repo_config_set_worktree_gently() to assist
> writing config values into the worktree.config file, especially when
> that may not have been initialized.
>
> When the base repo is bare, running 'git sparse-checkout init' in a
> worktree will create the config.worktree file for the worktree, but that
> will start causing the worktree to parse the bare repo's core.bare=true
> value and start treating the worktree as bare. This causes more problems
> as other commands are run in that worktree.
>
> The fix is to have this assignment into config.worktree be handled by
> the repo_config_set_worktree_gently() helper.
>
> Reported-by: Sean Allred <allred.sean@gmail.com>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> @@ -71,6 +71,20 @@ test_expect_success 'git sparse-checkout init' '
> +test_expect_success 'init in a worktree of a bare repo' '

Nit: Although `init` doing an incomplete job of enabling per-worktree
config was indeed the source of the problem, it actually manifested
when invoking other commands, such as `set`. Consequently, it may be
slightly misleading to talk about `init` in the test title. A title
such as "worktree of a bare repo" might be good enough. Anyhow, just a
nit.

> +       test_when_finished rm -rf bare worktree &&
> +       git clone --bare repo bare &&
> +       git -C bare worktree add ../worktree &&
> +       (
> +               cd worktree &&
> +               git sparse-checkout init &&
> +               test_must_fail git config core.bare &&

Nit: I'm rather "meh" on explicitly checking `core.bare` here since
it's not particularly relevant to the test: just testing `init + set`
alone is enough to trigger the bug which begat this patch series.
Future readers of this test might even be confused by the presence of
this `core.bare` check.

> +               git sparse-checkout set /*

The `/*` is expanding to all entries in the root of the filesystem,
which probably isn't what you intended. I suspect you want literal
"/*", in which case you need to quote it:

    git sparse-checkout set "/*"

> +       ) &&
> +       git -C bare config --list --show-origin >actual &&
> +       grep "file:config.worktree      core.bare=true" actual

As mentioned above, I'm fairly meh on this part (and perhaps leaning
toward the negative) since it places too much emphasis on a low-level
detail. I _could_ see this as a test of the new function which
upgrades the repo to per-worktree config, but that's not what _this_
test is about.

> +'