[PATCH v3 00/21] builtin/config: remove global state
- From
Patrick Steinhardt <ps@pks.im>
- Date
- May 15, 2024, 06:41 UTC
- Message-ID
- <cover.1715755055.git.ps@pks.im>
- In-Reply-To
- <cover.1715339393.git.ps@pks.im>
Hi,
this is the third version of my patch series that removes global state from "builtin/config.c".
There is only a single change compared to v2, namely another set of memory leak fixes for patch 9. I have double and triple checked now that this does address all the leaks, and all the CI leak jobs (well, all jobs in fact [1]) do pass now. Sorry for the noise and not doing this properly in v2!
[1]: https://gitlab.com/gitlab-org/git/-/pipelines/1291267388
Patrick
Patrick Steinhardt (21): builtin/config: stop printing full usage on misuse builtin/config: move legacy mode into its own function builtin/config: move subcommand options into `cmd_config()` builtin/config: move legacy options into `cmd_config()` builtin/config: move actions into `cmd_config_actions()` builtin/config: check for writeability after source is set up config: make the config source const builtin/config: refactor functions to have common exit paths builtin/config: move location options into local variables builtin/config: move display options into local variables builtin/config: move type options into display options builtin/config: move default value into display options builtin/config: move `respect_includes_opt` into location options builtin/config: convert `do_not_match` to a local variable builtin/config: convert `value_pattern` to a local variable builtin/config: convert `regexp` to a local variable builtin/config: convert `key_regexp` to a local variable builtin/config: convert `key` to a local variable builtin/config: track "fixed value" option via flags only builtin/config: convert flags to a local variable builtin/config: pass data between callbacks via local variables
builtin/config.c | 970 ++++++++++++++++++++++++++-------------------- config.c | 4 +- config.h | 2 +- t/t1300-config.sh | 9 +- 4 files changed, 552 insertions(+), 433 deletions(-)
Range-diff against v2:
1: 0ba7628126 = 1: 32380a12fd builtin/config: stop printing full usage on misuse
2: 663e1f74f8 = 2: 8b07b280a9 builtin/config: move legacy mode into its own function
3: 1239c151d0 = 3: b1de0ff74d builtin/config: move subcommand options into `cmd_config()`
4: 82964510c5 = 4: 1a0c6a8384 builtin/config: move legacy options into `cmd_config()`
5: 0a6ecae2cc = 5: 4950ddcecd builtin/config: move actions into `cmd_config_actions()`
6: 7ab99a27c1 = 6: 6de9097fb2 builtin/config: check for writeability after source is set up
7: 1460d3a36c = 7: 24053f8f4f config: make the config source const
8: 018ed0226b = 8: 9dc1d00f71 builtin/config: refactor functions to have common exit paths
9: b5d43b6f85 ! 9: 2ede4a204b builtin/config: move location options into local variables
@@ builtin/config.c: static const char *const builtin_config_edit_usage[] = {
+struct config_location_options {
+ struct git_config_source source;
+ struct config_options options;
++ char *file_to_free;
+ int use_global_config;
+ int use_system_config;
+ int use_local_config;
@@ builtin/config.c: static char *default_user_config(void)
- use_worktree_config +
- !!given_config_source.file + !!given_config_source.blob > 1) {
+ if (!opts->source.file)
-+ opts->source.file = xstrdup_or_null(getenv(CONFIG_ENVIRONMENT));
-+ else
-+ opts->source.file = xstrdup(opts->source.file);
++ opts->source.file = opts->file_to_free =
++ xstrdup_or_null(getenv(CONFIG_ENVIRONMENT));
+
+ if (opts->use_global_config + opts->use_system_config +
+ opts->use_local_config + opts->use_worktree_config +
@@ builtin/config.c: static char *default_user_config(void)
- given_config_source.file = git_global_config();
- if (!given_config_source.file)
+ if (opts->use_global_config) {
-+ opts->source.file = git_global_config();
++ opts->source.file = opts->file_to_free = git_global_config();
+ if (!opts->source.file)
/*
* It is unknown if HOME/.gitconfig exists, so
@@ builtin/config.c: static void handle_config_location(const char *prefix)
- } else if (use_worktree_config) {
+ opts->source.scope = CONFIG_SCOPE_GLOBAL;
+ } else if (opts->use_system_config) {
-+ opts->source.file = xstrdup_or_null(git_system_config());
++ opts->source.file = opts->file_to_free = git_system_config();
+ opts->source.scope = CONFIG_SCOPE_SYSTEM;
+ } else if (opts->use_local_config) {
-+ opts->source.file = xstrdup_or_null(git_pathdup("config"));
++ opts->source.file = opts->file_to_free = git_pathdup("config");
+ opts->source.scope = CONFIG_SCOPE_LOCAL;
+ } else if (opts->use_worktree_config) {
struct worktree **worktrees = get_worktrees();
if (the_repository->repository_format_worktree_config)
- given_config_source.file = git_pathdup("config.worktree");
-+ opts->source.file = git_pathdup("config.worktree");
++ opts->source.file = opts->file_to_free =
++ git_pathdup("config.worktree");
else if (worktrees[0] && worktrees[1])
die(_("--worktree cannot be used with multiple "
"working trees unless the config\n"
@@ builtin/config.c: static void handle_config_location(const char *prefix)
else
- given_config_source.file = git_pathdup("config");
- given_config_source.scope = CONFIG_SCOPE_LOCAL;
-+ opts->source.file = git_pathdup("config");
++ opts->source.file = opts->file_to_free =
++ git_pathdup("config");
+ opts->source.scope = CONFIG_SCOPE_LOCAL;
free_worktrees(worktrees);
- } else if (given_config_source.file) {
@@ builtin/config.c: static void handle_config_location(const char *prefix)
- given_config_source.scope = CONFIG_SCOPE_COMMAND;
+ } else if (opts->source.file) {
+ if (!is_absolute_path(opts->source.file) && prefix)
-+ opts->source.file =
++ opts->source.file = opts->file_to_free =
+ prefix_filename(prefix, opts->source.file);
+ opts->source.scope = CONFIG_SCOPE_COMMAND;
+ } else if (opts->source.blob) {
@@ builtin/config.c: static void handle_config_location(const char *prefix)
+static void location_options_release(struct config_location_options *opts)
+{
-+ free((char *) opts->source.file);
++ free(opts->file_to_free);
+}
+
static void handle_nul(void) {
10: d66e14af30 ! 10: 4d157942e6 builtin/config: move display options into local variables
@@ builtin/config.c: static int get_urlmatch(const struct config_location_options *
&matched->kvi);
fwrite(buf.buf, 1, buf.len, stdout);
@@ builtin/config.c: static void location_options_release(struct config_location_options *opts)
- free((char *) opts->source.file);
+ free(opts->file_to_free);
}
-static void handle_nul(void) {
11: 63436c3416 = 11: 5579371ad1 builtin/config: move type options into display options
12: 106b8ac8a2 = 12: 05a072d5d1 builtin/config: move default value into display options
13: 8a6b555b58 = 13: 15d45ef7d4 builtin/config: move `respect_includes_opt` into location options
14: 0dd22bf51a = 14: a729286cc5 builtin/config: convert `do_not_match` to a local variable
15: b656951f0c = 15: 821bc68212 builtin/config: convert `value_pattern` to a local variable
16: b56a07bda0 = 16: bac242caf0 builtin/config: convert `regexp` to a local variable
17: 323cb05120 = 17: 746bdf8733 builtin/config: convert `key_regexp` to a local variable
18: e972e63be8 = 18: f1f390f499 builtin/config: convert `key` to a local variable
19: d83c3d085e = 19: e4dbb4707e builtin/config: track "fixed value" option via flags only
20: 294bcd96a4 = 20: abe33015b7 builtin/config: convert flags to a local variable
21: 0496b958e2 = 21: a5cb075fcd builtin/config: pass data between callbacks via local variables-- 2.45.GIT