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
derrickstolee wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Audit and document Scalar config #2010

Show file tree
Hide file tree
Changes from 3 commits
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
25 changes: 15 additions & 10 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 All @@ -124,9 +135,6 @@ static int set_recommended_config(int reconfigure)
struct scalar_config config[] = {
Copy link

@gitgitgadget gitgitgadget bot Nov 27, 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:

> From: Derrick Stolee <stolee@gmail.com>
>
> These config values were added in the original Scalar contribution,
> d0feac4e8c (scalar: 'register' sets recommended config and starts
> maintenance, 2021-12-03), but were never fully checked for validity in
> the upstream Git project. At the time, Scalar was only intended for the
> contrib/ directory so did not have as rigorous of an investigation.
>
> Each config option has its own justification for removal:
>
> * core.preloadIndex: This value is true by default, now. Removing this
>   causes some changes required to the tests that checked this config
>   value. Use gui.gcwarning=false instead.
>
> * core.fscache: This config does not exist in the core Git project, but
>   is instead a config option for a Git for Windows feature.
>
> * core.multiPackIndex: This config value is now enabled by default, so
>   does not need to be called out specifically. It was originally
>   included to make sure the background maintenance that created
>   multi-pack-indexes would result in the expected performance
>   improvements.
>
> * credential.validate: This option is not something specific to Git but
>   instead an older version of Git Credential Manager for Windows. That
>   software was replaced several years ago by the cross-platform Git
>   Credential Manger so this option is no longer needed to help users who
>   were on that older software.
>
> * pack.useSparse=true: This value is now Git's default as of de3a864114
>   (config: set pack.useSparse=true by default, 2020-03-20) so we don't
>   need it set by Scalar.

Thanks for a conprehensive list.  Very well described.

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, Matthew Hughes wrote (reply to this):

On Mon, Dec 01, 2025 at 04:50:45PM +0000, Derrick Stolee via GitGitGadget wrote:
> * core.preloadIndex: This value is true by default, now. Removing this
>   causes some changes required to the tests that checked this config
>   value. Use gui.gcwarning=false instead.

I was going to ask about if we could also rely on the default value of
index.threads like we do here, but then went and did some reading and realised
some config values, like index.recordOffsetTable, have their value set
according to whether index.threads was explicitly set, so I guess there's an
implicit reliance on that behaviour that we want to keep?

> * core.fscache: This config does not exist in the core Git project, but
>   is instead a config option for a Git for Windows feature.
> 
> * core.multiPackIndex: This config value is now enabled by default, so
>   does not need to be called out specifically. It was originally
>   included to make sure the background maintenance that created
>   multi-pack-indexes would result in the expected performance
>   improvements.
> 
> * credential.validate: This option is not something specific to Git but
>   instead an older version of Git Credential Manager for Windows. That
>   software was replaced several years ago by the cross-platform Git
>   Credential Manger so this option is no longer needed to help users who
>   were on that older software.
> 
> * pack.useSparse=true: This value is now Git's default as of de3a864114
>   (config: set pack.useSparse=true by default, 2020-03-20) so we don't
>   need it set by Scalar.

Thanks for the detail on all of these, very helpful

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 05:46:46PM +0000, Matthew Hughes wrote:
> On Mon, Dec 01, 2025 at 04:50:45PM +0000, Derrick Stolee via GitGitGadget wrote:
> > * core.preloadIndex: This value is true by default, now. Removing this
> >   causes some changes required to the tests that checked this config
> >   value. Use gui.gcwarning=false instead.
> 
> I was going to ask about if we could also rely on the default value of
> index.threads like we do here, but then went and did some reading and realised
> some config values, like index.recordOffsetTable, have their value set
> according to whether index.threads was explicitly set, so I guess there's an
> implicit reliance on that behaviour that we want to keep?

Wait. Are you saying that "index.recordOffsetTable" behaves differently
based on whether "index.threads" is implicitly enabled due to the
default value or explicitly enabled via the configuration? If so, that
smells like a plain bug to me.

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, Matthew Hughes wrote (reply to this):

On Tue, Dec 02, 2025 at 08:53:45AM +0100, Patrick Steinhardt wrote:
> Wait. Are you saying that "index.recordOffsetTable" behaves differently
> based on whether "index.threads" is implicitly enabled due to the
> default value or explicitly enabled via the configuration?

That was my understanding from a cursory read of the results of searching for
'index.threads' in git-config:

> index.recordEndOfIndexEntries
> ...
> Defaults to true if index.threads has been explicitly enabled, false
> otherwise

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 Tue, Dec 02, 2025 at 07:04:24PM +0000, Matthew Hughes wrote:
> On Tue, Dec 02, 2025 at 08:53:45AM +0100, Patrick Steinhardt wrote:
> > Wait. Are you saying that "index.recordOffsetTable" behaves differently
> > based on whether "index.threads" is implicitly enabled due to the
> > default value or explicitly enabled via the configuration?
> 
> That was my understanding from a cursory read of the results of searching for
> 'index.threads' in git-config:
> 
> > index.recordEndOfIndexEntries
> > ...
> > Defaults to true if index.threads has been explicitly enabled, false
> > otherwise

Hm, true. At least that's a concious decision then.

The logic around this was introduced in 2a9dedef2e (index: make
index.threads=true enable ieot and eoie, 2018-11-19), and the ultimate
reason for it seems to be backwards compatibility:

    index.threads and index.recordOffsetTable unspecified: do not write
    the offset table yet (to avoid alarming the user with "ignoring IEOT
    extension" messages when an older version of Git accesses the
    repository) but do make use of multiple threads to read the index if
    the supporting offset table is present.

Older versions of Git complained when they see unknown extensions, and
we didn't want to expose users to such warnings. That makes me wonder
whether it's time now to revisit that decision -- it's been 7 years
since then, I guess that many clients nowadays would understand the
extension. 

The only (documented) downside should thus not be that important
anymore, but the upside is that reading the index would be faster if we
default-enable writing the extension.

Patrick

Copy link

@gitgitgadget gitgitgadget bot Dec 7, 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):

Patrick Steinhardt <ps@pks.im> writes:

> The logic around this was introduced in 2a9dedef2e (index: make
> index.threads=true enable ieot and eoie, 2018-11-19), and the ultimate
> reason for it seems to be backwards compatibility:
>
>     index.threads and index.recordOffsetTable unspecified: do not write
>     the offset table yet (to avoid alarming the user with "ignoring IEOT
>     extension" messages when an older version of Git accesses the
>     repository) but do make use of multiple threads to read the index if
>     the supporting offset table is present.
>
> Older versions of Git complained when they see unknown extensions, and
> we didn't want to expose users to such warnings.

Not "Older versions".

Any version of Git should complain mandatory index extension that it
does not understand.  And any version of Git, even today's Git,
gives a note when it ignores an optional index extension it does not
understand.

> That makes me wonder
> whether it's time now to revisit that decision -- it's been 7 years
> since then, I guess that many clients nowadays would understand the
> extension. 

I do not think 7 years matters.  The only reason you might see the
"ignoring" message is after using a newer version of Git that is
aware of that index extension, and then reverting back to an older
version.  As the index file is a purely local matter, it is not very
likely situation to begin with, and when it happens, the user should
be made aware of it.  Not understanding an optional index extension
is not a breaking sin; but being in such a situation, i.e., the user
is using older version of Git than they once used to use, is a note
worthy vent.

So yes, even this logic was introduced last week, if the only reason
is to avoid showing the note, that design decision should be
revisited.

> The only (documented) downside should thus not be that important
> anymore, but the upside is that reading the index would be faster if we
> default-enable writing the extension.
>
> Patrick

/* Required */
{ "am.keepCR", "true", 1 },
{ "core.FSCache", "true", 1 },
{ "core.multiPackIndex", "true", 1 },
{ "core.preloadIndex", "true", 1 },
#ifndef WIN32
{ "core.untrackedCache", "true", 1 },
#else
Expand All @@ -146,16 +154,14 @@ static int set_recommended_config(int reconfigure)
#endif
{ "core.logAllRefUpdates", "true", 1 },
{ "credential.https://dev.azure.com.useHttpPath", "true", 1 },
{ "credential.validate", "false", 1 }, /* GCM4W-only */
{ "gc.auto", "0", 1 },
{ "gui.GCWarning", "false", 1 },
{ "index.skipHash", "false", 1 },
{ "index.skipHash", "true", 1 },
{ "index.threads", "true", 1 },
{ "index.version", "4", 1 },
{ "merge.stat", "false", 1 },
{ "merge.renames", "true", 1 },
{ "pack.useBitmaps", "false", 1 },
{ "pack.useSparse", "true", 1 },
{ "receive.autoGC", "false", 1 },
{ "feature.manyFiles", "false", 1 },
{ "feature.experimental", "false", 1 },
Expand Down Expand Up @@ -197,9 +203,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
25 changes: 16 additions & 9 deletions t/t9210-scalar.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,14 +202,17 @@ test_expect_success 'scalar clone --no-... opts' '
test_expect_success 'scalar reconfigure' '
git init one/src &&
scalar register one &&
git -C one/src config core.preloadIndex false &&
git -C one/src config unset gui.gcwarning &&
scalar reconfigure one &&
test true = "$(git -C one/src config core.preloadIndex)" &&
git -C one/src config core.preloadIndex false &&
test false = "$(git -C one/src config gui.gcwarning)" &&
git -C one/src config unset gui.gcwarning &&
rm one/src/cron.txt &&
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 false = "$(git -C one/src config gui.gcwarning)" &&
test_grep "GCWarning = false # 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 All @@ -231,25 +234,29 @@ test_expect_success 'scalar reconfigure --all with includeIf.onbranch' '
git init $num/src &&
scalar register $num/src &&
git -C $num/src config includeif."onbranch:foo".path something &&
git -C $num/src config core.preloadIndex false || return 1
git -C $num/src config unset gui.gcwarning || return 1
done &&

scalar reconfigure --all &&

for num in $repos
do
test true = "$(git -C $num/src config core.preloadIndex)" || return 1
test false = "$(git -C $num/src config gui.gcwarning)" || return 1
done
'

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.
sane_unset GIT_TEST_SPLIT_INDEX &&

repos="two three four" &&
for num in $repos
do
rm -rf $num/src &&
git init $num/src &&
scalar register $num/src &&
git -C $num/src config core.preloadIndex false &&
git -C $num/src config unset gui.gcwarning &&
test_commit -C $num/src initial &&
git -C $num/src switch --detach HEAD || return 1
done &&
Expand All @@ -258,7 +265,7 @@ test_expect_success 'scalar reconfigure --all with detached HEADs' '

for num in $repos
do
test true = "$(git -C $num/src config core.preloadIndex)" || return 1
test false = "$(git -C $num/src config gui.gcwarning)" || return 1
done
'

Expand Down Expand Up @@ -290,7 +297,7 @@ test_expect_success 'scalar supports -c/-C' '
git init sub &&
scalar -C sub -c status.aheadBehind=bogus register &&
test -z "$(git -C sub config --local status.aheadBehind)" &&
test true = "$(git -C sub config core.preloadIndex)"
test false = "$(git -C sub config gui.gcwarning)"
'

test_expect_success '`scalar [...] <dir>` errors out when dir is missing' '
Expand Down