[PATCH v2 0/5] Audit and document Scalar config

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

In September [1], we discussed that the Scalar config options could use some
documented justification as well as some comments to the config file that
they were set by Scalar. I was then immediately distracted by other work
things and am finally here with a series to do just that.

[1]
https://lore.kernel.org/git/ffa61066-7004-48dd-9096-85b305373bc7@gmail.com/

I have indeed used Patrick's idea to add '# set by scalar' to each line
added by Scalar, it took a little more work for all the kinds of config set.
I made myself a co-author.

While working to justify each config option, I found some stale or incorrect
config options. I also relaxed the override setting in most cases which gave
me an opportunity to alphabetize the settings.

There was at least one case (I'm thinking of core.fscache here) where the
config doesn't even exist in core Git, but instead in Git for Windows. We'll
need to adjust in that fork to reinclude it in the right place.


Updates in V2
=============

 * The config-setting code is simplified somewhat.
 * Use 'sane_unset' instead of 'export' in test.
 * Documentation is improved for typos, grammar, and clarity.

Thanks, -Stolee

Derrick Stolee (5):
  scalar: annotate config file with "set by scalar"
  scalar: use index.skipHash=true for performance
  scalar: remove stale config values
  scalar: alphabetize and simplify config
  scalar: document config settings

 Documentation/scalar.adoc | 158 ++++++++++++++++++++++++++++++++++++++
 scalar.c                  |  83 ++++++++++----------
 t/t9210-scalar.sh         |  25 +++---
 3 files changed, 218 insertions(+), 48 deletions(-)


base-commit: 6ab38b7e9cc7adafc304f3204616a4debd49c6e9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2010%2Fderrickstolee%2Fscalar-config-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2010/derrickstolee/scalar-config-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2010

Range-diff vs v1:

 1:  a4ad8f80d0 ! 1:  639ff98c44 scalar: annotate config file with "set by scalar"
     @@ Commit message
          recommendations.
      
          Add "# set by scalar" to the end of each config option to assist users
     -    in identifying why these config options were set in their repo.
     +    in identifying why these config options were set in their repo. Use a new
     +    helper method to simplify the two callsites.
      
          Co-authored-by: Patrick Steinhardt <ps@pks.im>
          Signed-off-by: Patrick Steinhardt <ps@pks.im>
     @@ scalar.c
       
       static void setup_enlistment_directory(int argc, const char **argv,
       				       const char * const *usagestr,
     -@@ scalar.c: static int set_scalar_config(const struct scalar_config *config, int reconfigure
     +@@ scalar.c: struct scalar_config {
     + 	int overwrite_on_reconfigure;
     + };
     + 
     ++static int set_config_with_comment(const char *key, const char *value)
     ++{
     ++	char *file = repo_git_path(the_repository, "config");
     ++	int res = repo_config_set_multivar_in_file_gently(the_repository, file,
     ++							  key, value, NULL,
     ++							  " # set by scalar", 0);
     ++	free(file);
     ++	return res;
     ++}
     ++
     + static int set_scalar_config(const struct scalar_config *config, int reconfigure)
       {
       	char *value = NULL;
     - 	int res;
     -+	char *file = repo_git_path(the_repository, "config");
     - 
     +@@ scalar.c: static int set_scalar_config(const struct scalar_config *config, int reconfigure
       	if ((reconfigure && config->overwrite_on_reconfigure) ||
       	    repo_config_get_string(the_repository, config->key, &value)) {
       		trace2_data_string("scalar", the_repository, config->key, "created");
      -		res = repo_config_set_gently(the_repository, config->key, config->value);
     -+		res = repo_config_set_multivar_in_file_gently(the_repository, file, config->key,
     -+							      config->value, NULL,
     -+							      " # set by scalar", 0);
     ++		res = set_config_with_comment(config->key, config->value);
       	} else {
       		trace2_data_string("scalar", the_repository, config->key, "exists");
       		res = 0;
     - 	}
     - 
     -+	free(file);
     - 	free(value);
     - 	return res;
     - }
      @@ scalar.c: static int set_recommended_config(int reconfigure)
     - 	 * for multiple values.
     - 	 */
       	if (repo_config_get_string(the_repository, "log.excludeDecoration", &value)) {
     -+		char *file = repo_git_path(the_repository, "config");
       		trace2_data_string("scalar", the_repository,
       				   "log.excludeDecoration", "created");
      -		if (repo_config_set_multivar_gently(the_repository, "log.excludeDecoration",
     -+		if (repo_config_set_multivar_in_file_gently(the_repository, file,
     -+						    "log.excludeDecoration",
     - 						    "refs/prefetch/*",
     +-						    "refs/prefetch/*",
      -						    CONFIG_REGEX_NONE, 0))
     -+						    CONFIG_REGEX_NONE,
     -+						    " # set by scalar",
     -+						    0))
     ++		if (set_config_with_comment("log.excludeDecoration",
     ++					    "refs/prefetch/*"))
       			return error(_("could not configure "
       				       "log.excludeDecoration"));
     -+		free(file);
       	} else {
     - 		trace2_data_string("scalar", the_repository,
     - 				   "log.excludeDecoration", "exists");
      
       ## t/t9210-scalar.sh ##
      @@ t/t9210-scalar.sh: test_expect_success 'scalar reconfigure' '
 2:  1c51dbb814 ! 2:  10e9548955 scalar: use index.skipHash=true for performance
     @@ t/t9210-scalar.sh: test_expect_success 'scalar reconfigure --all with includeIf.
       test_expect_success 'scalar reconfigure --all with detached HEADs' '
      +	# This test demonstrates an issue with index.skipHash=true and
      +	# this test variable for the split index. Disable the test variable.
     -+	GIT_TEST_SPLIT_INDEX= &&
     -+	export GIT_TEST_SPLIT_INDEX &&
     ++	sane_unset GIT_TEST_SPLIT_INDEX &&
      +
       	repos="two three four" &&
       	for num in $repos
 3:  156be69a79 = 3:  8783db6153 scalar: remove stale config values
 4:  9b8ce6ba2b = 4:  edc0254770 scalar: alphabetize and simplify config
 5:  18580f020d ! 5:  ac1627dbd9 scalar: document config settings
     @@ Commit message
          Add user-facing documentation that justifies the values being set by
          'scalar clone', 'scalar register', and 'scalar reconfigure'.
      
     +    Helped-by: Junio C Hamano <gitster@pobox.com>
     +    Helped-by: Patrick Steinhardt <ps@pks.im>
          Signed-off-by: Derrick Stolee <stolee@gmail.com>
      
       ## Documentation/scalar.adoc ##
     @@ Documentation/scalar.adoc: delete <enlistment>::
      +	While the preferred version is 2 for performance reasons, existing users
      +	that had version 1 by default will need special care in upgrading to
      +	version 2. This is likely to change in the future as the upgrade story
     -+	is solidifies.
     ++	solidifies.
      +
      +core.autoCRLF=false::
      +	This removes the transformation of worktree files to add CRLF line
     @@ Documentation/scalar.adoc: delete <enlistment>::
      +
      +fetch.unpackLimit=1::
      +	This setting prevents Git from unpacking packfiles into loose objects
     -+	as they are downloaded from the server. This feature was intended as a
     -+	way to prevent performance issues from too many packfiles, but Scalar
     -+	uses background maintenance to group packfiles and cover them with a
     -+	multi-pack-index, removing this issue.
     ++	as they are downloaded from the server. The default limit of 100 was
     ++	intended as a way to prevent performance issues from too many packfiles,
     ++	but Scalar uses background maintenance to group packfiles and cover them
     ++	with a multi-pack-index, removing this issue.
      +
      +fetch.writeCommitGraph=false::
     -+	This config setting was created to help users automatically udpate their
     ++	This config setting was created to help users automatically update their
      +	commit-graph files as they perform fetches. However, this takes time
      +	from foreground fetches and pulls and Scalar uses background maintenance
      +	for this function instead.
     @@ Documentation/scalar.adoc: delete <enlistment>::
      +
      +index.threads=true::
      +	This tells Git to automatically detect how many threads it should use
     -+	when reading the index in parallel due to the `core.preloadIndex=true`
     -+	setting.
     ++	when reading the index due to the default value of `core.preloadIndex`,
     ++	which enables parallel index reads.
      +
      +index.version=4::
      +	This index version adds compression to the path names, reducing the size

-- 
gitgitgadget