[PATCH v6 0/6] Sparse checkout: fix bug with worktree of bare repo

["Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>]

This series is now based on v2.35.0 since that contains all of the necessary
topics.

This patch series includes a fix to the bug reported by Sean Allred [1] and
diagnosed by Eric Sunshine [2].

The root cause is that 'git sparse-checkout init' writes to the worktree
config without checking that core.bare or core.worktree are set in the
common config file. This series fixes this, but also puts in place some
helpers to prevent this from happening in the future.

ATTENTION: I have significantly redesigned the series since previous
versions, so most of this cover letter is new.

 * Patch 1 updates documentation around extensions.worktreeConfig in a few
   places to improve discoverability. Several cross links are added to make
   it easy to find the related areas. (The documentation for the changes to
   'git sparse-checkout' are delayed to patch 4.)

 * Patch 2 introduces the init_worktree_config() helper which follows the
   documented instructions to enable extensions.worktreeConfig as well as
   move the core.bare and core.worktree config values. This update does not
   modify core.repositoryFormatVersion, since this is not needed
   specifically for extensions.worktreeConfig.

 * Patch 3 adds a new repo_config_set_worktree_gently() helper method so we
   can internally adjust a config value within a worktree, at least if
   extensions.worktreeConfig is enabled. (It will write to the common config
   file if the extension is not enabled.)

 * Patch 4 modifies the sparse-checkout builtin to use
   init_worktree_config() and repo_config_set_worktree_gently() in ways that
   fix the reported bug. The behavior change here is that it will no longer
   upgrade the repository format version, since that is not needed for
   extensions.worktreeConfig.

 * Patch 5 updates 'git worktree add' to copy the worktree config from the
   current worktree to the new one (while unsetting core.bare=true and
   core.worktree=*) along with copying the sparse-checkout patterns file.

[1]
https://lore.kernel.org/git/CABceR4bZmtC4rCwgxZ1BBYZP69VOUca1f_moJoP989vTUZWu9Q@mail.gmail.com/
[2]
https://lore.kernel.org/git/CAPig+cQ6U_yFw-X2OWrizB1rbCvc4bNxuSzKFzmoLNnm0GH8Eg@mail.gmail.com/


Updates in v6
=============

 * Updated documentation to use "working tree" over "worktree" and "" over
   ""
 * Delay some allocations to avoid leaking memory in error conditions.
 * Use "main worktree" over "base worktree" in comments.
 * 


Updates in v5
=============

Responded to most of Eric's suggestions. I pushed back on one about a
comment including information from the commit message, but everything else
should be as Eric suggested.

 * Cleaned up documentation as per Elijah's suggestions.
 * Removed unnecessary conflicting change in git-sparse-checkout.txt
 * Fixed an ambiguous comment about moving config values.
 * Removed use of git_configset_get_string_tmp() and added a patch that
   removes its public declaration.
 * Fragile variables are replaced with better ones.
 * Variable names and code style improved.
 * Several test cleanups in patch 5.


Updates in v4
=============

 * Rebased to v2.35.0
 * Fixed memory leak (was leaking repo_git_path() result)
 * Added additional documentation updates so curious users can discover the
   intricacies of extensions.worktreeConfig from multiple entry points.
 * Significantly reduced the amount of changes to config.c.
 * 'git sparse-checkout' no longer upgrades the repository format.
 * Dropped the update to upgrade_repository_format(), since it is not
   needed.
 * Dropped the 'git worktree init-worktree-config' subcommand in favor of a
   helper method called by 'git sparse-checkout'
 * Many others because of the significant changes required by the above
   items.

Thanks, -Stolee

Derrick Stolee (6):
  Documentation: add extensions.worktreeConfig details
  worktree: create init_worktree_config()
  config: add repo_config_set_worktree_gently()
  sparse-checkout: set worktree-config correctly
  worktree: copy sparse-checkout patterns and config on add
  config: make git_configset_get_string_tmp() private

 Documentation/config/extensions.txt   | 31 ++++++++++++
 Documentation/git-config.txt          |  8 ++-
 Documentation/git-sparse-checkout.txt | 16 ++++--
 Documentation/git-worktree.txt        | 11 ++--
 builtin/sparse-checkout.c             | 28 +++++-----
 builtin/worktree.c                    | 63 +++++++++++++++++++++++
 config.c                              | 39 ++++++++++++--
 config.h                              |  9 +++-
 sparse-index.c                        | 10 ++--
 t/t1091-sparse-checkout-builtin.sh    | 35 +++++++++----
 t/t2400-worktree-add.sh               | 58 ++++++++++++++++++++-
 worktree.c                            | 73 +++++++++++++++++++++++++++
 worktree.h                            | 21 ++++++++
 13 files changed, 353 insertions(+), 49 deletions(-)


base-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1101%2Fderrickstolee%2Fsparse-checkout%2Fbare-worktree-bug-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1101/derrickstolee/sparse-checkout/bare-worktree-bug-v6
Pull-Request: https://github.com/gitgitgadget/git/pull/1101

Range-diff vs v5:

 1:  1bd5f26271c ! 1:  0260ff6cac0 Documentation: add extensions.worktreeConfig details
     @@ Documentation/config/extensions.txt: extensions.objectFormat::
      +	If enabled, then worktrees will load config settings from the
      +	`$GIT_DIR/config.worktree` file in addition to the
      +	`$GIT_COMMON_DIR/config` file. Note that `$GIT_COMMON_DIR` and
     -+	`$GIT_DIR` are the same for the main worktree, while other
     -+	worktrees have `$GIT_DIR` equal to
     -+	`$GIT_COMMON_DIR/worktrees/<worktree-name>/`. The settings in the
     ++	`$GIT_DIR` are the same for the main working tree, while other
     ++	working trees have `$GIT_DIR` equal to
     ++	`$GIT_COMMON_DIR/worktrees/<id>/`. The settings in the
      +	`config.worktree` file will override settings from any other
      +	config files.
      ++
      +When enabling `extensions.worktreeConfig`, you must be careful to move
     -+certain values from the common config file to the main worktree's
     ++certain values from the common config file to the main working tree's
      +`config.worktree` file, if present:
      ++
      +* `core.worktree` must be moved from `$GIT_COMMON_DIR/config` to
     @@ Documentation/git-config.txt: from all available files.
       	read from or written to if `extensions.worktreeConfig` is
      -	present. If not it's the same as `--local`.
      +	enabled. If not it's the same as `--local`. Note that `$GIT_DIR`
     -+	is equal to `$GIT_COMMON_DIR` for the main worktree, but is of the
     -+	form `$GIT_DIR/worktrees/<worktree-name>/` for other worktrees. See
     ++	is equal to `$GIT_COMMON_DIR` for the main working tree, but is of
     ++	the form `$GIT_DIR/worktrees/<id>/` for other working trees. See
      +	linkgit:git-worktree[1] to learn how to enable
      +	`extensions.worktreeConfig`.
       
 2:  2a2c350112e ! 2:  5d0cc242d92 worktree: create init_worktree_config()
     @@ worktree.c: int should_prune_worktree(const char *id, struct strbuf *reason, cha
      +	int bare = 0;
      +	struct config_set cs = { { 0 } };
      +	const char *core_worktree;
     -+	char *common_config_file = xstrfmt("%s/config", r->commondir);
     -+	char *main_worktree_file = xstrfmt("%s/config.worktree", r->commondir);
     ++	char *common_config_file;
     ++	char *main_worktree_file;
      +
      +	/*
      +	 * If the extension is already enabled, then we can skip the
     @@ worktree.c: int should_prune_worktree(const char *id, struct strbuf *reason, cha
      +	if ((res = git_config_set_gently("extensions.worktreeConfig", "true")))
      +		return error(_("failed to set extensions.worktreeConfig setting"));
      +
     ++	common_config_file = xstrfmt("%s/config", r->commondir);
     ++	main_worktree_file = xstrfmt("%s/config.worktree", r->commondir);
     ++
      +	git_configset_init(&cs);
      +	git_configset_add_file(&cs, common_config_file);
      +
      +	/*
      +	 * If core.bare is true in the common config file, then we need to
     -+	 * move it to the base worktree's config file or it will break all
     ++	 * move it to the main worktree's config file or it will break all
      +	 * worktrees. If it is false, then leave it in place because it
      +	 * _could_ be negating a global core.bare=true.
      +	 */
     @@ worktree.c: int should_prune_worktree(const char *id, struct strbuf *reason, cha
      +			goto cleanup;
      +	}
      +	/*
     -+	 * If core.worktree is set, then the base worktree is located
     ++	 * If core.worktree is set, then the main worktree is located
      +	 * somewhere different than the parent of the common Git dir.
      +	 * Relocate that value to avoid breaking all worktrees with this
      +	 * upgrade to worktree config.
      +	 */
     -+	if (!git_configset_get_string_tmp(&cs, "core.worktree", &core_worktree)) {
     ++	if (!git_configset_get_value(&cs, "core.worktree", &core_worktree)) {
      +		if ((res = move_config_setting("core.worktree", core_worktree,
      +					       common_config_file,
      +					       main_worktree_file)))
 3:  802b28a9510 = 3:  cf9e86fe3a4 config: add repo_config_set_worktree_gently()
 4:  08b89d17ccf ! 4:  5b5924eab49 sparse-checkout: set worktree-config correctly
     @@ Metadata
       ## Commit message ##
          sparse-checkout: set worktree-config correctly
      
     -    The previous change added repo_config_set_worktree_gently() to assist
     -    writing config values into the config.worktree file, if enabled. An
     -    earlier change added init_worktree_config() as a helper to initialize
     -    extensions.worktreeConfig if not already enabled.
     +    `git sparse-checkout set/init` enables worktree-specific
     +    configuration[*] by setting extensions.worktreeConfig=true, but neglects
     +    to perform the additional necessary bookkeeping of relocating
     +    `core.bare=true` and `core.worktree` from $GIT_COMMON_DIR/config to
     +    $GIT_COMMON_DIR/config.worktree, as documented in git-worktree.txt. As a
     +    result of this oversight, these settings, which are nonsensical for
     +    secondary worktrees, can cause Git commands to incorrectly consider a
     +    worktree bare (in the case of `core.bare`) or operate on the wrong
     +    worktree (in the case of `core.worktree`). Fix this problem by taking
     +    advantage of the recently-added init_worktree_config() which enables
     +    `extensions.worktreeConfig` and takes care of necessary bookkeeping.
      
     -    Let the sparse-checkout builtin use these helpers instead of attempting to
     -    initialize the worktree config on its own. This changes behavior of 'git
     -    sparse-checkout set' in a few important ways:
     +    While at it, for backward-compatibility reasons, also stop upgrading the
     +    repository format to "1" since doing so is (unintentionally) not
     +    required to take advantage of `extensions.worktreeConfig`, as explained
     +    by 11664196ac ("Revert "check_repository_format_gently(): refuse
     +    extensions for old repositories"", 2020-07-15).
      
     -     1. Git will no longer upgrade the repository format, since this is not
     -        a requirement for understanding extensions.worktreeConfig.
     -
     -     2. If the main worktree is bare, then this command will not put the
     -        worktree in a broken state.
     -
     -    The main reason to use worktree-specific config for the sparse-checkout
     -    builtin was to avoid enabling sparse-checkout patterns in one and
     -    causing a loss of files in another. If a worktree does not have a
     -    sparse-checkout patterns file, then the sparse-checkout logic will not
     -    kick in on that worktree.
     +    [*] The main reason to use worktree-specific config for the
     +    sparse-checkout builtin was to avoid enabling sparse-checkout patterns
     +    in one and causing a loss of files in another. If a worktree does not
     +    have a sparse-checkout patterns file, then the sparse-checkout logic
     +    will not kick in on that worktree.
      
          Reported-by: Sean Allred <allred.sean@gmail.com>
          Helped-by: Eric Sunshine <sunshine@sunshineco.com>
 5:  85779dfaed3 ! 5:  c51cb3714e7 worktree: copy sparse-checkout patterns and config on add
     @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam
      +	 */
      +	if (core_apply_sparse_checkout) {
      +		char *from_file = git_pathdup("info/sparse-checkout");
     -+		char *to_file = xstrfmt("%s/worktrees/%s/info/sparse-checkout",
     -+					realpath.buf, name);
     ++		char *to_file = xstrfmt("%s/info/sparse-checkout",
     ++					sb_repo.buf);
      +
      +		if (file_exists(from_file)) {
      +			if (safe_create_leading_directories(to_file) ||
     @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam
      +	 */
      +	if (repository_format_worktree_config) {
      +		char *from_file = git_pathdup("config.worktree");
     -+		char *to_file = xstrfmt("%s/worktrees/%s/config.worktree",
     -+					realpath.buf, name);
     ++		char *to_file = xstrfmt("%s/config.worktree",
     ++					sb_repo.buf);
      +
      +		if (file_exists(from_file)) {
     -+			struct config_set cs = { { 0 }};
     -+			const char *str_value;
     -+			int bool_value;
     ++			struct config_set cs = { { 0 } };
     ++			const char *core_worktree;
     ++			int bare;
      +
      +			if (safe_create_leading_directories(to_file) ||
     -+			    copy_file(to_file, from_file, 0666))
     -+				die(_("failed to copy worktree config from '%s' to '%s'"),
     -+				    from_file, to_file);
     ++			    copy_file(to_file, from_file, 0666)) {
     ++				error(_("failed to copy worktree config from '%s' to '%s'"),
     ++				      from_file, to_file);
     ++				goto worktree_copy_cleanup;
     ++			}
      +
      +			git_configset_init(&cs);
      +			git_configset_add_file(&cs, from_file);
      +
     -+			if (!git_configset_get_bool(&cs, "core.bare", &bool_value) &&
     -+			    bool_value &&
     ++			if (!git_configset_get_bool(&cs, "core.bare", &bare) &&
     ++			    bare &&
      +			    git_config_set_multivar_in_file_gently(
      +					to_file, "core.bare", NULL, "true", 0))
      +				error(_("failed to unset 'core.bare' in '%s'"), to_file);
     -+			if (!git_configset_get_value(&cs, "core.worktree", &str_value) &&
     ++			if (!git_configset_get_value(&cs, "core.worktree", &core_worktree) &&
      +			    git_config_set_in_file_gently(to_file,
      +							  "core.worktree", NULL))
      +				error(_("failed to unset 'core.worktree' in '%s'"), to_file);
     @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam
      +			git_configset_clear(&cs);
      +		}
      +
     ++worktree_copy_cleanup:
      +		free(from_file);
      +		free(to_file);
      +	}
     @@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'add to sparse-checkout'
      +	git -C repo sparse-checkout set --no-cone "/*" &&
      +	git -C repo worktree add --quiet ../worktree 2>err &&
      +	test_must_be_empty err &&
     -+	new=repo/.git/worktrees/worktree/info/sparse-checkout &&
     -+	test_path_is_file $new &&
     -+	test_cmp repo/.git/info/sparse-checkout $new &&
     ++	new="$(git -C worktree rev-parse --git-path info/sparse-checkout)" &&
     ++	test_path_is_file "$new" &&
     ++	test_cmp repo/.git/info/sparse-checkout "$new" &&
      +	git -C worktree sparse-checkout set --cone &&
      +	test_cmp_config -C worktree true core.sparseCheckoutCone &&
      +	test_must_fail git -C repo core.sparseCheckoutCone
     @@ t/t2400-worktree-add.sh: test_expect_success '"add" default branch of a bare rep
      -	)
      +		git worktree add ../there3 main &&
      +		cd ../there3 &&
     ++		# Simple check that a Git command does not
     ++		# immediately fail with the current setup
      +		git status
      +	) &&
      +	cat >expect <<-EOF &&
     @@ t/t2400-worktree-add.sh: test_expect_success '"add" default branch of a bare rep
      +		git clone --bare . bare3 &&
      +		cd bare3 &&
      +		git config extensions.worktreeconfig true &&
     ++
     ++		# Add config values that are erroneous to have in
     ++		# a config.worktree file outside of the main
     ++		# working tree, to check that Git filters them out
     ++		# when copying config during "git worktree add".
      +		git config --worktree core.bare true &&
      +		git config --worktree core.worktree "$(pwd)" &&
     ++
     ++		# We want to check that bogus.key is copied
      +		git config --worktree bogus.key value &&
      +		git config --unset core.bare &&
      +		git worktree add ../there4 main &&
      +		cd ../there4 &&
     ++
     ++		# Simple check that a Git command does not
     ++		# immediately fail with the current setup
      +		git status &&
      +		git worktree add --detach ../there5 &&
      +		cd ../there5 &&
 -:  ----------- > 6:  f687a0bfa16 config: make git_configset_get_string_tmp() private

-- 
gitgitgadget