[PATCH v3] worktree: detect from secondary worktree if main worktree is bare

["Olga Pilipenco via GitGitGadget" <gitgitgadget@gmail.com>]

From: Olga Pilipenco <olga.pilipenco@shopify.com>

When extensions.worktreeConfig is true and the main worktree is
bare -- that is, its config.worktree file contains core.bare=true
-- commands run from secondary worktrees incorrectly see the main
worktree as not bare. As such, those commands incorrectly think
that the repository's default branch (typically "main" or
"master") is checked out in the bare repository even though it's
not. This makes it impossible, for instance, to checkout or delete
the default branch from a secondary worktree, among other
shortcomings.

This problem occurs because, when extensions.worktreeConfig is
true, commands run in secondary worktrees only consult
$commondir/config and $commondir/worktrees/<id>/config.worktree,
thus they never see the main worktree's core.bare=true setting in
$commondir/config.worktree.

Fix this problem by consulting the main worktree's config.worktree
file when checking whether it is bare. (This extra work is
performed only when running from a secondary worktree.)

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Olga Pilipenco <olga.pilipenco@shopify.com>
---
    worktree: detect from secondary worktree if main worktree is bare
    
    Changes since v2, all results of the amazing review:
    
     * updated description & comments;
     * private function is_bare_git_dir is replaced with
       is_main_worktree_bare. The new implementation only checks if main
       worktree's worktree.config contains information if main worktree is
       bare or not. It's assumed that other configs of main worktree are
       already checked for bareness prior this call.
     * notation { { 0 } } is replaced with {0} that is preferred by the
       project.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1829%2Folga-mcbfe%2Ffix-bare-repo-detection-with-worktree-config-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1829/olga-mcbfe/fix-bare-repo-detection-with-worktree-config-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1829

Range-diff vs v2:

 1:  17f4b24d1da ! 1:  f9207746b38 worktree: detect from secondary worktree if main worktree is bare
     @@ Metadata
       ## Commit message ##
          worktree: detect from secondary worktree if main worktree is bare
      
     -    Setup:
     -    1. Have a bare repo with core.bare = true in config.worktree
     -    2. Create a new worktree
     +    When extensions.worktreeConfig is true and the main worktree is
     +    bare -- that is, its config.worktree file contains core.bare=true
     +    -- commands run from secondary worktrees incorrectly see the main
     +    worktree as not bare. As such, those commands incorrectly think
     +    that the repository's default branch (typically "main" or
     +    "master") is checked out in the bare repository even though it's
     +    not. This makes it impossible, for instance, to checkout or delete
     +    the default branch from a secondary worktree, among other
     +    shortcomings.
      
     -    Behavior:
     -    From the secondary worktree the main worktree appears as non-bare.
     +    This problem occurs because, when extensions.worktreeConfig is
     +    true, commands run in secondary worktrees only consult
     +    $commondir/config and $commondir/worktrees/<id>/config.worktree,
     +    thus they never see the main worktree's core.bare=true setting in
     +    $commondir/config.worktree.
      
     -    Expected:
     -    From the secondary worktree the main worktree should appear as bare.
     -
     -    Why current behavior is not good?
     -    If the main worktree is detected as not bare it doesn't allow
     -    checking out the branch of the main worktree. There are possibly
     -    other problems associated with that behavior.
     -
     -    Why is it happening?
     -    While we're inside the secondary worktree we don't initialize the main
     -    worktree's repository with its configuration.
     -
     -    How is it fixed?
     -    Load actual configs of the main worktree. Also, skip the config loading
     -    step if we're already inside the current worktree because in that case we
     -    rely on is_bare_repository() to return the correct result.
     -
     -    Other solutions considered:
     -    Alternatively, instead of incorrectly always using
     -    `the_repository` as the main worktree's repository, we can detect
     -    and load the actual repository of the main worktree and then use
     -    that repository's `is_bare` value extracted from correct configs.
     -    However, this approach is a bit riskier and could also affect
     -    performance. Since we had the assignment `worktree->repo =
     -    the_repository` for a long time already, I decided it's safe to
     -    keep it as it is for now; it can be still fixed separately from
     -    this change.
     -
     -    Real life use case:
     -    1. Have a bare repo
     -    2. Create a worktree from the bare repo
     -    3. In the secondary worktree enable sparse-checkout - this enables
     -    extensions.worktreeConfig and keeps core.bare=true setting in
     -    config.worktree of the bare worktree
     -    4. The secondary worktree or any other non-bare worktree created
     -    won't be able to use branch main (not even once), but it should be
     -    able to.
     +    Fix this problem by consulting the main worktree's config.worktree
     +    file when checking whether it is bare. (This extra work is
     +    performed only when running from a secondary worktree.)
      
     +    Helped-by: Eric Sunshine <sunshine@sunshineco.com>
          Signed-off-by: Olga Pilipenco <olga.pilipenco@shopify.com>
      
       ## t/t3200-branch.sh ##
     @@ t/t3200-branch.sh: test_expect_success 'bare main worktree has HEAD at branch de
       	git -C secondary branch -D main
       '
       
     -+test_expect_success 'secondary worktree can switch to main if common dir is bare worktree' '
     ++test_expect_success 'secondary worktrees recognize core.bare=true in main config.worktree' '
      +	test_when_finished "rm -rf bare_repo non_bare_repo secondary_worktree" &&
      +	git init -b main non_bare_repo &&
      +	test_commit -C non_bare_repo x &&
     @@ worktree.c: static int is_current_worktree(struct worktree *wt)
       	return is_current;
       }
       
     -+static int is_bare_git_dir(const char *git_dir)
     ++/*
     ++* When in a secondary worktree, and when extensions.worktreeConfig
     ++* is true, only $commondir/config and $commondir/worktrees/<id>/
     ++* config.worktree are consulted, hence any core.bare=true setting in
     ++* $commondir/config.worktree gets overlooked. Thus, check it manually
     ++* to determine if the repository is bare.
     ++*/
     ++static int is_main_worktree_bare(struct repository *repo)
      +{
      +	int bare = 0;
     -+	struct config_set cs = { { 0 } };
     -+	char *config_file;
     -+	char *worktree_config_file;
     -+
     -+	config_file = xstrfmt("%s/config", git_dir);
     -+	worktree_config_file = xstrfmt("%s/config.worktree",  git_dir);
     ++	struct config_set cs = {0};
     ++	char *worktree_config = xstrfmt("%s/config.worktree", repo_get_common_dir(repo));
      +
      +	git_configset_init(&cs);
     -+	git_configset_add_file(&cs, config_file);
     -+	git_configset_add_file(&cs, worktree_config_file);
     -+
     ++	git_configset_add_file(&cs, worktree_config);
      +	git_configset_get_bool(&cs, "core.bare", &bare);
      +
      +	git_configset_clear(&cs);
     -+	free(config_file);
     -+	free(worktree_config_file);
     ++	free(worktree_config);
      +	return bare;
      +}
      +
     @@ worktree.c: static int is_current_worktree(struct worktree *wt)
        * get the main worktree
        */
      @@ worktree.c: static struct worktree *get_main_worktree(int skip_reading_head)
     - 	strbuf_strip_suffix(&worktree_path, "/.git");
     - 
       	CALLOC_ARRAY(worktree, 1);
     -+	/*
     -+	 * NEEDSWORK: the_repository is not always main worktree's repository
     -+	*/
       	worktree->repo = the_repository;
       	worktree->path = strbuf_detach(&worktree_path, NULL);
      -	/*
     @@ worktree.c: static struct worktree *get_main_worktree(int skip_reading_head)
       	worktree->is_current = is_current_worktree(worktree);
      +	worktree->is_bare = (is_bare_repository_cfg == 1) ||
      +		is_bare_repository() ||
     -+		(!worktree->is_current && is_bare_git_dir(repo_get_common_dir(the_repository)));
     ++		(!worktree->is_current && is_main_worktree_bare(the_repository));
      +
       	if (!skip_reading_head)
       		add_head_info(worktree);


 t/t3200-branch.sh | 14 ++++++++++++++
 worktree.c        | 35 ++++++++++++++++++++++++++---------
 2 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index a3a21c54cf6..f3e720dc10d 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -410,6 +410,20 @@ test_expect_success 'bare main worktree has HEAD at branch deleted by secondary
 	git -C secondary branch -D main
 '
 
+test_expect_success 'secondary worktrees recognize core.bare=true in main config.worktree' '
+	test_when_finished "rm -rf bare_repo non_bare_repo secondary_worktree" &&
+	git init -b main non_bare_repo &&
+	test_commit -C non_bare_repo x &&
+
+	git clone --bare non_bare_repo bare_repo &&
+	git -C bare_repo config extensions.worktreeConfig true &&
+	git -C bare_repo config unset core.bare &&
+	git -C bare_repo config --worktree core.bare true &&
+
+	git -C bare_repo worktree add ../secondary_worktree &&
+	git -C secondary_worktree checkout main
+'
+
 test_expect_success 'git branch --list -v with --abbrev' '
 	test_when_finished "git branch -D t" &&
 	git branch t &&
diff --git a/worktree.c b/worktree.c
index 248bbb39d43..6df4ccf97f7 100644
--- a/worktree.c
+++ b/worktree.c
@@ -65,6 +65,28 @@ static int is_current_worktree(struct worktree *wt)
 	return is_current;
 }
 
+/*
+* When in a secondary worktree, and when extensions.worktreeConfig
+* is true, only $commondir/config and $commondir/worktrees/<id>/
+* config.worktree are consulted, hence any core.bare=true setting in
+* $commondir/config.worktree gets overlooked. Thus, check it manually
+* to determine if the repository is bare.
+*/
+static int is_main_worktree_bare(struct repository *repo)
+{
+	int bare = 0;
+	struct config_set cs = {0};
+	char *worktree_config = xstrfmt("%s/config.worktree", repo_get_common_dir(repo));
+
+	git_configset_init(&cs);
+	git_configset_add_file(&cs, worktree_config);
+	git_configset_get_bool(&cs, "core.bare", &bare);
+
+	git_configset_clear(&cs);
+	free(worktree_config);
+	return bare;
+}
+
 /**
  * get the main worktree
  */
@@ -79,16 +101,11 @@ static struct worktree *get_main_worktree(int skip_reading_head)
 	CALLOC_ARRAY(worktree, 1);
 	worktree->repo = the_repository;
 	worktree->path = strbuf_detach(&worktree_path, NULL);
-	/*
-	 * NEEDSWORK: If this function is called from a secondary worktree and
-	 * config.worktree is present, is_bare_repository_cfg will reflect the
-	 * contents of config.worktree, not the contents of the main worktree.
-	 * This means that worktree->is_bare may be set to 0 even if the main
-	 * worktree is configured to be bare.
-	 */
-	worktree->is_bare = (is_bare_repository_cfg == 1) ||
-		is_bare_repository();
 	worktree->is_current = is_current_worktree(worktree);
+	worktree->is_bare = (is_bare_repository_cfg == 1) ||
+		is_bare_repository() ||
+		(!worktree->is_current && is_main_worktree_bare(the_repository));
+
 	if (!skip_reading_head)
 		add_head_info(worktree);
 	return worktree;

base-commit: f93ff170b93a1782659637824b25923245ac9dd1
-- 
gitgitgadget