[v3,00/21] builtin/config: remove global state

Message

Patrick Steinhardt May 15, 2024, 6:41 a.m. UTC

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