Audit and document Scalar config #2010
+218
−47
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ | |
| #include "help.h" | ||
|
There was a problem hiding this comment. On the Git mailing list, Junio C Hamano wrote (reply to this): "Derrick Stolee via GitGitGadget" <[email protected]> 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. |
||
| #include "setup.h" | ||
| #include "trace2.h" | ||
| #include "path.h" | ||
|
|
||
| static void setup_enlistment_directory(int argc, const char **argv, | ||
| const char * const *usagestr, | ||
|
|
@@ -99,16 +100,20 @@ 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"); | ||
|
|
||
| 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_multivar_in_file_gently(the_repository, file, config->key, | ||
| config->value, NULL, | ||
| " # set by scalar", 0); | ||
| } else { | ||
| trace2_data_string("scalar", the_repository, config->key, "exists"); | ||
| res = 0; | ||
| } | ||
|
|
||
| free(file); | ||
| free(value); | ||
| return res; | ||
| } | ||
|
|
@@ -122,13 +127,33 @@ static int have_fsmonitor_support(void) | |
| static int set_recommended_config(int reconfigure) | ||
| { | ||
| struct scalar_config config[] = { | ||
|
There was a problem hiding this comment. On the Git mailing list, Junio C Hamano wrote (reply to this): "Derrick Stolee via GitGitGadget" <[email protected]> writes:
> From: Derrick Stolee <[email protected]>
>
> 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.
|
||
| { "am.keepCR", "true" }, | ||
| { "commitGraph.changedPaths", "true" }, | ||
| { "commitGraph.generationVersion", "1" }, | ||
| { "core.autoCRLF", "false" }, | ||
| { "core.logAllRefUpdates", "true" }, | ||
| { "core.safeCRLF", "false" }, | ||
| { "credential.https://dev.azure.com.useHttpPath", "true" }, | ||
| { "feature.experimental", "false" }, | ||
| { "feature.manyFiles", "false" }, | ||
| { "fetch.showForcedUpdates", "false" }, | ||
| { "fetch.unpackLimit", "1" }, | ||
| { "fetch.writeCommitGraph", "false" }, | ||
| { "gc.auto", "0" }, | ||
| { "gui.GCWarning", "false" }, | ||
| { "index.skipHash", "true", 1 /* Fix previous setting. */ }, | ||
| { "index.threads", "true"}, | ||
| { "index.version", "4" }, | ||
| { "merge.renames", "true" }, | ||
| { "merge.stat", "false" }, | ||
| { "pack.useBitmaps", "false" }, | ||
| { "pack.usePathWalk", "true" }, | ||
| { "receive.autoGC", "false" }, | ||
| { "status.aheadBehind", "false" }, | ||
|
|
||
| /* platform-specific */ | ||
| #ifndef WIN32 | ||
| { "core.untrackedCache", "true" }, | ||
| #else | ||
| /* | ||
| * Unfortunately, Scalar's Functional Tests demonstrated | ||
|
|
@@ -142,36 +167,11 @@ static int set_recommended_config(int reconfigure) | |
| * Therefore, with a sad heart, we disable this very useful | ||
| * feature on Windows. | ||
| */ | ||
| { "core.untrackedCache", "false" }, | ||
|
|
||
| /* Other Windows-specific required settings: */ | ||
| { "http.sslBackend", "schannel" }, | ||
| #endif | ||
| { NULL, NULL }, | ||
| }; | ||
| int i; | ||
|
|
@@ -195,13 +195,18 @@ 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_in_file_gently(the_repository, file, | ||
| "log.excludeDecoration", | ||
| "refs/prefetch/*", | ||
| CONFIG_REGEX_NONE, | ||
| " # set by scalar", | ||
| 0)) | ||
| return error(_("could not configure " | ||
| "log.excludeDecoration")); | ||
| free(file); | ||
| } else { | ||
| trace2_data_string("scalar", the_repository, | ||
| "log.excludeDecoration", "exists"); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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):