Skip to content

Audit and document Scalar config #2010

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
+218 −48
Open

Audit and document Scalar config #2010

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
639ff98
scalar: annotate config file with "set by scalar"
derrickstolee Nov 26, 2025
10e9548
scalar: use index.skipHash=true for performance
derrickstolee Nov 26, 2025
8783db6
scalar: remove stale config values
derrickstolee Nov 26, 2025
edc0254
scalar: alphabetize and simplify config
derrickstolee Nov 26, 2025
ac1627d
scalar: document config settings
derrickstolee Nov 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions scalar.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "help.h"
Copy link

@gitgitgadget gitgitgadget bot Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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.

The implementation is quite straight-forward, inlining expansion of
repo_config_set_gently() in the places that we want to add comment to.

If we had (a lot) more than two callsites, I would have suggested to
add a simple helper function, something like

    static int scalar_config_set(struct repository *r, const char *key, const char *value)
    {
	char *file = repo_git_path(r, "config");
        int res = repo_config_set_multivar_in_file_gently(r, file,
		key, value, NULL, " # set by scalar", 0);
	free(file);
	return res;
    }

and then the updates to the callers would have been absolute minimum.

Well, even with only two callsites, perhaps such a refactoring may
still have value in reducing the risk of typo in the comment.

> diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh
> index bd6f0c40d2..43c210a23d 100755
> --- a/t/t9210-scalar.sh
> +++ b/t/t9210-scalar.sh
> @@ -210,6 +210,9 @@ test_expect_success 'scalar reconfigure' '
>  	GIT_TRACE2_EVENT="$(pwd)/reconfigure" scalar reconfigure -a &&
>  	test_path_is_file one/src/cron.txt &&
>  	test true = "$(git -C one/src config core.preloadIndex)" &&
> +	test_grep "preloadIndex = true # set by scalar" one/src/.git/config &&
> +	test_grep "excludeDecoration = refs/prefetch/\* # set by scalar" one/src/.git/config &&
> +
>  	test_subcommand git maintenance start <reconfigure &&
>  	test_subcommand ! git maintenance unregister --force <reconfigure &&

Looks good.

Copy link

@gitgitgadget gitgitgadget bot Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Wed, Nov 26, 2025 at 03:55:10PM -0800, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > 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.
> 
> The implementation is quite straight-forward, inlining expansion of
> repo_config_set_gently() in the places that we want to add comment to.
> 
> If we had (a lot) more than two callsites, I would have suggested to
> add a simple helper function, something like
> 
>     static int scalar_config_set(struct repository *r, const char *key, const char *value)
>     {
> 	char *file = repo_git_path(r, "config");
>         int res = repo_config_set_multivar_in_file_gently(r, file,
> 		key, value, NULL, " # set by scalar", 0);
> 	free(file);
> 	return res;
>     }
> 
> and then the updates to the callers would have been absolute minimum.
> 
> Well, even with only two callsites, perhaps such a refactoring may
> still have value in reducing the risk of typo in the comment.

Agreed, I think it's a good idea to provide such a function. The calls
to `repo_config_set_multivar_in_file_gently()` are quite verbose.

Patrick

Copy link

@gitgitgadget gitgitgadget bot Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Mon, Dec 01, 2025 at 04:50:43PM +0000, Derrick Stolee via GitGitGadget wrote:
> diff --git a/t/t9210-scalar.sh b/t/t9210-scalar.sh
> index bd6f0c40d2..43c210a23d 100755
> --- a/t/t9210-scalar.sh
> +++ b/t/t9210-scalar.sh
> @@ -210,6 +210,9 @@ test_expect_success 'scalar reconfigure' '
>  	GIT_TRACE2_EVENT="$(pwd)/reconfigure" scalar reconfigure -a &&
>  	test_path_is_file one/src/cron.txt &&
>  	test true = "$(git -C one/src config core.preloadIndex)" &&
> +	test_grep "preloadIndex = true # set by scalar" one/src/.git/config &&
> +	test_grep "excludeDecoration = refs/prefetch/\* # set by scalar" one/src/.git/config &&
> +
>  	test_subcommand git maintenance start <reconfigure &&
>  	test_subcommand ! git maintenance unregister --force <reconfigure &&

We _could_ make this a bit more solid by adding a test that:

  1. Initializes a new repository.

  2. Saves the configuration.

  3. Performs `scalar reconfigure`.

  4. Asserts that all new non-section-header lines in the configuration
     have a trailing "#set by scalar" comment.

This would ensure that there is no callsite we forgot to add the new
annotation to, and that there are new future callsites where somebody
isn't aware of the comments.

I don't insist on such a test though, so please feel free to ignore this
suggestion.

Patrick

#include "setup.h"
#include "trace2.h"
#include "path.h"

static void setup_enlistment_directory(int argc, const char **argv,
const char * const *usagestr,
Expand Down Expand Up @@ -95,6 +96,16 @@ 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;
Expand All @@ -103,7 +114,7 @@ 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 = set_config_with_comment(config->key, config->value);
} else {
trace2_data_string("scalar", the_repository, config->key, "exists");
res = 0;
Expand Down Expand Up @@ -197,9 +208,8 @@ static int set_recommended_config(int reconfigure)
if (repo_config_get_string(the_repository, "log.excludeDecoration", &value)) {
trace2_data_string("scalar", the_repository,
"log.excludeDecoration", "created");
if (repo_config_set_multivar_gently(the_repository, "log.excludeDecoration",
"refs/prefetch/*",
CONFIG_REGEX_NONE, 0))
if (set_config_with_comment("log.excludeDecoration",
"refs/prefetch/*"))
return error(_("could not configure "
"log.excludeDecoration"));
} else {
Expand Down
3 changes: 3 additions & 0 deletions t/t9210-scalar.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,9 @@ test_expect_success 'scalar reconfigure' '
GIT_TRACE2_EVENT="$(pwd)/reconfigure" scalar reconfigure -a &&
test_path_is_file one/src/cron.txt &&
test true = "$(git -C one/src config core.preloadIndex)" &&
test_grep "preloadIndex = true # set by scalar" one/src/.git/config &&
test_grep "excludeDecoration = refs/prefetch/\* # set by scalar" one/src/.git/config &&

test_subcommand git maintenance start <reconfigure &&
test_subcommand ! git maintenance unregister --force <reconfigure &&

Expand Down