[PATCH v2 0/2] setup: warn if extensions exist on old format

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

This is based on xl/upgrade-repo-format.

Thanks, Taylor and Junio for jumping in with helpful review.

Updates in v2:

 1. My initial patch is essentially dropped in its entirety, with Junio's
    patch here instead. I added an extra test and kept some of my commit
    message.
    
    
 2. A second patch has joined the fray, hopefully answering the concerned
    raise by Johannes [1].
    
    

[1] 
https://lore.kernel.org/git/pull.675.git.1594677321039.gitgitgadget@gmail.com/

Thanks, -Stolee

Derrick Stolee (2):
  setup: tweak upgrade policy to grandfather worktreeconfig
  config: provide extra detail about worktree config

 builtin/config.c                   |  5 +++--
 cache.h                            |  2 +-
 setup.c                            | 12 ++++++++----
 t/t1091-sparse-checkout-builtin.sh | 12 ++++++++++++
 t/t2404-worktree-config.sh         | 12 ++++++++++++
 5 files changed, 36 insertions(+), 7 deletions(-)


base-commit: 14c7fa269e42df4133edd9ae7763b678ed6594cd
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-674%2Fderrickstolee%2Fsparse-checkout-warning-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-674/derrickstolee/sparse-checkout-warning-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/674

Range-diff vs v1:

 1:  113cb7cda2 ! 1:  1b26d9710a setup: warn if extensions exist on old format
     @@ Metadata
      Author: Derrick Stolee <dstolee@microsoft.com>
      
       ## Commit message ##
     -    setup: warn if extensions exist on old format
     +    setup: tweak upgrade policy to grandfather worktreeconfig
      
          Prior to 14c7fa269e4 (check_repository_format_gently(): refuse extensions
          for old repositories, 2020-06-05), Git was honoring configured
     @@ Commit message
      
          The issue now is that users who relied on that previously bad behavior
          will upgrade to the next version of Git and suddently be in a bad
     -    situation. In particular, users of the 'git sparse-checkout' builting
     +    situation. In particular, users of the 'git sparse-checkout' command
          will rely on the extensions.worktreeConfig for the core.sparseCheckout
          and core.sparseCheckoutCone config options. Without that extension,
          these users will suddenly have repositories stop acting like a sparse
     @@ Commit message
          This is because the logic in 14c7fa269e4 refuses to upgrae repos when
          the version is unset but extensions exist.
      
     -    While it is important to correct this improper behavior, create a
     -    warning so users in this situation can correct themselves without too
     -    much guesswork. By creating a warning in
     -    check_repository_format_gently(), we can alert the user if they have a
     -    ocnfigured extension but not a configured repository version.
     +    One possible way to alert a user of this issue is to warn them when Git
     +    notices an extension exists, but core.repositoryFormatVersion is not a
     +    correct value. However,
      
     +     - it requires the end-user to read, understand and execute the
     +       manual upgrade
     +
     +     - it encourages to follow the same procedure blindly, making the
     +       protection even less useful
     +
     +    Let's instead keep failing hard without teaching how to bypass the
     +    repository protection, but allow upgrading even when only the
     +    worktreeconfig extension exists in an old repository, which is
     +    likely to be set by a broke version of Git that did not update the
     +    repository version when setting the extension.
     +
     +    This change of behavior is made visible by testing how 'git
     +    sparse-checkout init' behaves to upgrade the repository format version
     +    even if the extension.worktreeConfig is already set. This would
     +    previously fail without a clear way forward.
     +
     +    Signed-off-by: Junio C Hamano <gitster@pobox.com>
          Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
      
     + ## cache.h ##
     +@@ cache.h: struct repository_format {
     + 	int worktree_config;
     + 	int is_bare;
     + 	int hash_algo;
     +-	int has_extensions;
     + 	char *work_tree;
     ++	int has_unallowed_extensions;
     + 	struct string_list unknown_extensions;
     + };
     + 
     +
       ## setup.c ##
     -@@ setup.c: static int check_repository_format_gently(const char *gitdir, struct repository_
     - 		}
     +@@ setup.c: static int check_repo_format(const char *var, const char *value, void *vdata)
     + 	if (strcmp(var, "core.repositoryformatversion") == 0)
     + 		data->version = git_config_int(var, value);
     + 	else if (skip_prefix(var, "extensions.", &ext)) {
     +-		data->has_extensions = 1;
     + 		/*
     + 		 * record any known extensions here; otherwise,
     + 		 * we fall through to recording it as unknown, and
     + 		 * check_repository_format will complain
     + 		 */
     ++		int is_unallowed_extension = 1;
     ++
     + 		if (!strcmp(ext, "noop"))
     + 			;
     + 		else if (!strcmp(ext, "preciousobjects"))
     +@@ setup.c: static int check_repo_format(const char *var, const char *value, void *vdata)
     + 			if (!value)
     + 				return config_error_nonbool(var);
     + 			data->partial_clone = xstrdup(value);
     +-		} else if (!strcmp(ext, "worktreeconfig"))
     ++		} else if (!strcmp(ext, "worktreeconfig")) {
     + 			data->worktree_config = git_config_bool(var, value);
     +-		else
     ++			is_unallowed_extension = 0;
     ++		} else
     + 			string_list_append(&data->unknown_extensions, ext);
     ++
     ++		data->has_unallowed_extensions |= is_unallowed_extension;
       	}
       
     -+	if (candidate->version == 0 && candidate->has_extensions) {
     -+		warning(_("some extensions are enabled, but core.repositoryFormatVersion=0"));
     -+		warning(_("if you intended to use extensions, run 'git config core.repositoryFormatVersion 1'"));
     -+	}
     -+
     - 	return 0;
     - }
     + 	return read_worktree_config(var, value, vdata);
     +@@ setup.c: int upgrade_repository_format(int target_version)
     + 		return 0;
       
     + 	if (verify_repository_format(&repo_fmt, &err) < 0 ||
     +-	    (!repo_fmt.version && repo_fmt.has_extensions)) {
     ++	    (!repo_fmt.version && repo_fmt.has_unallowed_extensions)) {
     + 		warning("unable to upgrade repository format from %d to %d: %s",
     + 			repo_fmt.version, target_version, err.buf);
     + 		strbuf_release(&err);
      
       ## t/t1091-sparse-checkout-builtin.sh ##
      @@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'git sparse-checkout init' '
       	check_files repo a
       '
       
     -+test_expect_success 'warning about core.repositoryFormatVersion' '
     ++test_expect_success 'git sparse-checkout works if repository format is wrong' '
      +	test_when_finished git -C repo config core.repositoryFormatVersion 1 &&
     -+	git -C repo status 2>err &&
     -+	test_must_be_empty err &&
     -+	git -C repo config --local core.repositoryFormatVersion 0 &&
     -+	git -C repo status 2>err &&
     -+	test_i18ngrep "some extensions are enabled, but core.repositoryFormatVersion=0" err
     ++	git -C repo config --unset core.repositoryFormatVersion &&
     ++	git -C repo sparse-checkout init &&
     ++	git -C repo config core.repositoryFormatVersion >actual &&
     ++	echo 1 >expect &&
     ++	git -C repo config core.repositoryFormatVersion 0 &&
     ++	git -C repo sparse-checkout init &&
     ++	git -C repo config core.repositoryFormatVersion >actual &&
     ++	test_cmp expect actual
      +'
      +
       test_expect_success 'git sparse-checkout list after init' '
 -:  ---------- > 2:  e11e973c6f config: provide extra detail about worktree config

-- 
gitgitgadget