Re: [PATCH v10 7/9] environment: move push_default into repo_config_values

["Pablo Sabater" <pabloosabaterr@gmail.com>]

On Sun Jul 12, 2026 at 1:17 PM CEST, Tian Yuchen wrote:
> The global variable 'push_default' specifies the default behavior of
> 'git push' when no explicit refspec is provided. Move 'push_default'
> into 'struct repo_config_values' to continue the libification effort.
>
> While 'enum push_default_type' ideally belongs in 'remote.h', moving it
> there introduces a circular dependency chain:
>
>   remote.h -> hash.h -> repository.h -> environment.h.
>
> Therefore, the enum definition is kept in 'environment.h' just above
> 'struct repo_config_values' with a NEEDSWORK comment for future cleanup.
>
> Modify the configuration parsing in environment.c to update the
> per-repository structure directly, and update caller across the
> codebase to access the value via 'repo_config_values()'.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com>
> Mentored-by: Olamide Caleb Bello <belkid98@gmail.com>
> Signed-off-by: Tian Yuchen <cat@malon.dev>
> ---
>  builtin/push.c |  8 ++++----
>  environment.c  | 16 +++++++++-------
>  environment.h  | 26 ++++++++++++++++----------
>  remote.c       |  2 +-
>  4 files changed, 30 insertions(+), 22 deletions(-)
>
> diff --git a/builtin/push.c b/builtin/push.c
> index 6021b71d66..6dc3224b60 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -88,7 +88,7 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref,
>  		}
>  	}
>
> -	if (push_default == PUSH_DEFAULT_UPSTREAM &&
> +	if (repo_config_values(the_repository)->push_default == PUSH_DEFAULT_UPSTREAM &&

Can we extract cfg from repo_config_values() to shorten this line?
If we look at the hunk below, we can see this pattern. let's do the
same.

>  	    skip_prefix(matched->name, "refs/heads/", &branch_name)) {
>  		struct branch *branch = branch_get(branch_name);
>  		if (branch->merge_nr == 1 && branch->merge[0]->src) {
> @@ -160,7 +160,7 @@ static NORETURN void die_push_simple(struct branch *branch,
>  	 * Don't show advice for people who explicitly set
>  	 * push.default.
>  	 */
> -	if (push_default == PUSH_DEFAULT_UNSPECIFIED)
> +	if (cfg->push_default == PUSH_DEFAULT_UNSPECIFIED)
>  		advice_pushdefault_maybe = _("\n"
>  				 "To choose either option permanently, "
>  				 "see push.default in 'git help config'.\n");
> @@ -232,7 +232,7 @@ static void setup_default_push_refspecs(int *flags, struct remote *remote)
>  	const char *dst;
>  	int same_remote;
>
> -	switch (push_default) {
> +	switch (repo_config_values(the_repository)->push_default) {
>  	case PUSH_DEFAULT_MATCHING:
>  		refspec_append(&rs, ":");
>  		return;
> @@ -252,7 +252,7 @@ static void setup_default_push_refspecs(int *flags, struct remote *remote)
>  	dst = branch->refname;
>  	same_remote = !strcmp(remote->name, remote_for_branch(branch, NULL));
>
> -	switch (push_default) {
> +	switch (repo_config_values(the_repository)->push_default) {
>  	default:
>  	case PUSH_DEFAULT_UNSPECIFIED:
>  	case PUSH_DEFAULT_SIMPLE:
> diff --git a/environment.c b/environment.c
> index 41ba013c86..0080012f31 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -59,7 +59,6 @@ enum eol core_eol = EOL_UNSET;
>  int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
>  char *check_roundtrip_encoding;
>  enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
> -enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
>  #ifndef OBJECT_CREATION_MODE
>  #define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
>  #endif
> @@ -621,21 +620,23 @@ static int git_default_branch_config(const char *var, const char *value)
>
>  static int git_default_push_config(const char *var, const char *value)
>  {
> +	struct repo_config_values *cfg = repo_config_values(the_repository);
> +
>  	if (!strcmp(var, "push.default")) {
>  		if (!value)
>  			return config_error_nonbool(var);
>  		else if (!strcmp(value, "nothing"))
> -			push_default = PUSH_DEFAULT_NOTHING;
> +			cfg->push_default = PUSH_DEFAULT_NOTHING;
>  		else if (!strcmp(value, "matching"))
> -			push_default = PUSH_DEFAULT_MATCHING;
> +			cfg->push_default = PUSH_DEFAULT_MATCHING;
>  		else if (!strcmp(value, "simple"))
> -			push_default = PUSH_DEFAULT_SIMPLE;
> +			cfg->push_default = PUSH_DEFAULT_SIMPLE;
>  		else if (!strcmp(value, "upstream"))
> -			push_default = PUSH_DEFAULT_UPSTREAM;
> +			cfg->push_default = PUSH_DEFAULT_UPSTREAM;
>  		else if (!strcmp(value, "tracking")) /* deprecated */
> -			push_default = PUSH_DEFAULT_UPSTREAM;
> +			cfg->push_default = PUSH_DEFAULT_UPSTREAM;
>  		else if (!strcmp(value, "current"))
> -			push_default = PUSH_DEFAULT_CURRENT;
> +			cfg->push_default = PUSH_DEFAULT_CURRENT;
>  		else {
>  			error(_("malformed value for %s: %s"), var, value);
>  			return error(_("must be one of nothing, matching, simple, "
> @@ -727,6 +728,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
>  	cfg->askpass_program = NULL;
>  	cfg->apply_default_whitespace = NULL;
>  	cfg->apply_default_ignorewhitespace = NULL;
> +	cfg->push_default = PUSH_DEFAULT_UNSPECIFIED;
>  	cfg->apply_sparse_checkout = 0;
>  	cfg->branch_track = BRANCH_TRACK_REMOTE;
>  	cfg->trust_ctime = 1;
> diff --git a/environment.h b/environment.h
> index 553f87adee..6a5c8bd06f 100644
> --- a/environment.h
> +++ b/environment.h
> @@ -87,6 +87,21 @@ extern const char * const local_repo_env[];
>  struct strvec;
>
>  struct repository;
> +
> +/*
> + * NEEDSWORK: It would be better if these definitions could be moved to
> + * other more specific files, but care is needed to avoid circular
> + * inclusion issues.
> + */
> +enum push_default_type {
> +	PUSH_DEFAULT_NOTHING = 0,
> +	PUSH_DEFAULT_MATCHING,
> +	PUSH_DEFAULT_SIMPLE,
> +	PUSH_DEFAULT_UPSTREAM,
> +	PUSH_DEFAULT_CURRENT,
> +	PUSH_DEFAULT_UNSPECIFIED
> +};
> +
>  struct repo_config_values {
>  	/* section "core" config values */
>  	char *attributes_file;
> @@ -96,6 +111,7 @@ struct repo_config_values {
>  	char *askpass_program;
>  	char *apply_default_whitespace;
>  	char *apply_default_ignorewhitespace;
> +	enum push_default_type push_default;
>  	int apply_sparse_checkout;
>  	int trust_ctime;
>  	int check_stat;
> @@ -197,16 +213,6 @@ enum rebase_setup_type {
>  };
>  extern enum rebase_setup_type autorebase;
>
> -enum push_default_type {
> -	PUSH_DEFAULT_NOTHING = 0,
> -	PUSH_DEFAULT_MATCHING,
> -	PUSH_DEFAULT_SIMPLE,
> -	PUSH_DEFAULT_UPSTREAM,
> -	PUSH_DEFAULT_CURRENT,
> -	PUSH_DEFAULT_UNSPECIFIED
> -};
> -extern enum push_default_type push_default;
> -
>  enum object_creation_mode {
>  	OBJECT_CREATION_USES_HARDLINKS = 0,
>  	OBJECT_CREATION_USES_RENAMES = 1
> diff --git a/remote.c b/remote.c
> index 00723b385e..d48c01d375 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1933,7 +1933,7 @@ static char *branch_get_push_1(struct repository *repo,
>  	if (remote->mirror)
>  		return tracking_for_push_dest(remote, branch->refname, err);
>
> -	switch (push_default) {
> +	switch (repo_config_values(repo)->push_default) {
>  	case PUSH_DEFAULT_NOTHING:
>  		return error_buf(err, _("push has no destination (push.default is 'nothing')"));
>

I haven't checked how doable fixing the dependency cycle of the
NEEDSWORK is.

Similar to other patches where we add repo_config_values() to functions
that didn't have it before. We need to check that there's no caller that
could pass a repository different from the_repository. I haven't checked
it this time.

Regards,
Pablo