[PATCH v1 0/3] environment: migrate more global variables, pt.2

[PATCH v1 0/3] environment: migrate more global variables, pt.2

[Tian Yuchen]

Hi all,

This series moves:

 - (1/3) minimum_abbrev and default_abbrev 
 - (2/3) pack_size_limit_cfg
 - (3/3) assume_unchanged

into repo_config_values to continue the libification effort. 

Note: in commit 1/3, we need (repo != the_repository) checks in the
getters, because some subsystems where the readers of _abbrev
configurations live forbid the use of 'the_repository' and only accept
'repo' [1]. We have to explicitly intercept those intances that are
not 'the_repository'.

Thanks! yuchen

 [1] [PATCH v3 5/6] merge-ort: prevent the_repository from coming back
 https://lore.kernel.org/git/42a25768784492a4e8187bad0b070ccb27e980f7.1771718393.git.gitgitgadget@gmail.com/

Tian Yuchen (3):
  environment: migrate minimum_abbrev and default_abbrev
  environment: migrate pack_size_limit_cfg into repo_config_values
  environment: migrate assume_unchanged into repo_config_values

 builtin/pack-objects.c |  2 +-
 builtin/update-index.c |  3 ++-
 environment.c          | 39 ++++++++++++++++++++++++++++++---------
 environment.h          | 11 ++++++++---
 merge-ort.c            |  7 ++++---
 object-file.c          |  5 +++--
 object-name.h          |  4 ++--
 read-cache.c           |  9 ++++++---
 replay.c               |  2 +-
 sequencer.c            |  5 +++--
 10 files changed, 60 insertions(+), 27 deletions(-)

-- 
2.43.0

[PATCH v1 1/3] environment: migrate minimum_abbrev and default_abbrev

[Tian Yuchen]

Move the global 'minimum_abbrev' and 'default_abbrev' configurations
into the repository-specific 'repo_config_values'
struct.

To ensure code readability, the getter functions
'repo_minimum_abbrev()' and 'repo_default_abbrev()' have been introduced.
The existing MINIMUM_ABBREV and DEFAULT_ABBREV macros are redefined to
the corresponding getters.

Additionally, some subsystems forbid the direct use of 'the_repository'
and must pass their own local 'repo'. Let the getters explicitly intercept
these instances.

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>
---
 environment.c | 31 ++++++++++++++++++++++++++-----
 environment.h |  6 +++++-
 merge-ort.c   |  7 ++++---
 object-name.h |  4 ++--
 replay.c      |  2 +-
 sequencer.c   |  5 +++--
 6 files changed, 41 insertions(+), 14 deletions(-)

diff --git a/environment.c b/environment.c
index c663113e8a..2b44778b50 100644
--- a/environment.c
+++ b/environment.c
@@ -43,7 +43,6 @@ static int zlib_compression_seen;
 
 int trust_executable_bit = 1;
 int has_symlinks = 1;
-int minimum_abbrev = 4, default_abbrev = -1;
 int assume_unchanged;
 char *git_commit_encoding;
 char *git_log_output_encoding;
@@ -148,6 +147,26 @@ int repo_ignore_case(struct repository *repo)
 		0;
 }
 
+int repo_minimum_abbrev(struct repository *repo)
+{
+	if (repo != the_repository)
+		repo = the_repository;
+
+	return repo->initialized
+		? repo_config_values(repo)->minimum_abbrev
+		: 4;
+}
+
+int repo_default_abbrev(struct repository *repo)
+{
+	if (repo != the_repository)
+		repo = the_repository;
+
+	return repo->initialized
+		? repo_config_values(repo)->default_abbrev
+		: -1;
+}
+
 int have_git_dir(void)
 {
 	return startup_info->have_repository
@@ -364,14 +383,14 @@ int git_default_core_config(const char *var, const char *value,
 		if (!value)
 			return config_error_nonbool(var);
 		if (!strcasecmp(value, "auto"))
-			default_abbrev = -1;
+			cfg->default_abbrev = -1;
 		else if (!git_parse_maybe_bool_text(value))
-			default_abbrev = GIT_MAX_HEXSZ;
+			cfg->default_abbrev = GIT_MAX_HEXSZ;
 		else {
 			int abbrev = git_config_int(var, value, ctx->kvi);
-			if (abbrev < minimum_abbrev)
+			if (abbrev < cfg->minimum_abbrev)
 				return error(_("abbrev length out of range: %d"), abbrev);
-			default_abbrev = abbrev;
+			cfg->default_abbrev = abbrev;
 		}
 		return 0;
 	}
@@ -738,6 +757,8 @@ void repo_config_values_init(struct repo_config_values *cfg)
 	cfg->check_stat = 1;
 	cfg->zlib_compression_level = Z_BEST_SPEED;
 	cfg->pack_compression_level = Z_DEFAULT_COMPRESSION;
+	cfg->minimum_abbrev = 4;
+	cfg->default_abbrev = -1;
 	cfg->precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
 	cfg->core_sparse_checkout_cone = 0;
 	cfg->sparse_expect_files_outside_of_patterns = 0;
diff --git a/environment.h b/environment.h
index acfb670be1..c5905d8b01 100644
--- a/environment.h
+++ b/environment.h
@@ -95,6 +95,8 @@ struct repo_config_values {
 	int check_stat;
 	int zlib_compression_level;
 	int pack_compression_level;
+	int minimum_abbrev;
+	int default_abbrev;
 	int precomposed_unicode;
 	int core_sparse_checkout_cone;
 	int warn_on_object_refname_ambiguity;
@@ -151,6 +153,9 @@ int repo_protect_ntfs(struct repository *repo);
  */
 int repo_ignore_case(struct repository *repo);
 
+int repo_minimum_abbrev(struct repository *repo);
+int repo_default_abbrev(struct repository *repo);
+
 void repo_config_values_init(struct repo_config_values *cfg);
 
 int is_bare_repository(struct repository *repo);
@@ -180,7 +185,6 @@ int have_git_dir(void);
 /* Environment bits from configuration mechanism */
 extern int trust_executable_bit;
 extern int has_symlinks;
-extern int minimum_abbrev, default_abbrev;
 extern int assume_unchanged;
 extern char *apply_default_whitespace;
 extern char *apply_default_ignorewhitespace;
diff --git a/merge-ort.c b/merge-ort.c
index c410a5d353..b94ebcc2de 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -777,7 +777,7 @@ static void format_commit(struct strbuf *sb,
 {
 	struct merge_remote_desc *desc;
 	struct pretty_print_context ctx = {0};
-	ctx.abbrev = DEFAULT_ABBREV;
+	ctx.abbrev = repo_default_abbrev(repo);
 
 	strbuf_addchars(sb, ' ', indent);
 	desc = merge_remote_util(commit);
@@ -2035,7 +2035,8 @@ static int merge_submodule(struct merge_options *opt,
 		util->flag = sub_flag;
 		util->abbrev = NULL;
 		if (!sub_not_initialized) {
-			abbrev = repo_find_unique_abbrev(&subrepo, b, DEFAULT_ABBREV);
+			abbrev = repo_find_unique_abbrev(&subrepo, b,
+							repo_default_abbrev(opt->repo));
 			util->abbrev = xstrdup(abbrev);
 		}
 		string_list_append(csub, path)->util = util;
@@ -5348,7 +5349,7 @@ static void merge_ort_internal(struct merge_options *opt,
 	} else {
 		strbuf_add_unique_abbrev(&merge_base_abbrev,
 					 &merged_merge_bases->object.oid,
-					 DEFAULT_ABBREV);
+					 repo_default_abbrev(opt->repo));
 		ancestor_name = merge_base_abbrev.buf;
 	}
 
diff --git a/object-name.h b/object-name.h
index 167a9154ea..a6d7206ed8 100644
--- a/object-name.h
+++ b/object-name.h
@@ -133,8 +133,8 @@ struct object *repo_peel_to_type(struct repository *r,
 				 struct object *o, enum object_type);
 
 /* Convert to/from hex/sha1 representation */
-#define MINIMUM_ABBREV minimum_abbrev
-#define DEFAULT_ABBREV default_abbrev
+#define MINIMUM_ABBREV repo_minimum_abbrev(the_repository)
+#define DEFAULT_ABBREV repo_default_abbrev(the_repository)
 
 /* used when the code does not know or care what the default abbrev is */
 #define FALLBACK_DEFAULT_ABBREV 7
diff --git a/replay.c b/replay.c
index 463c900d6c..cd41c7f507 100644
--- a/replay.c
+++ b/replay.c
@@ -27,7 +27,7 @@ static const char *short_commit_name(struct repository *repo,
 				     struct commit *commit)
 {
 	return repo_find_unique_abbrev(repo, &commit->object.oid,
-				       DEFAULT_ABBREV);
+				       repo_default_abbrev(repo));
 }
 
 static struct commit *peel_committish(struct repository *repo,
diff --git a/sequencer.c b/sequencer.c
index 1355a99a09..2426c5e422 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5813,7 +5813,7 @@ static const char *label_oid(struct object_id *oid, const char *label,
 		label = p = state->buf.buf;
 
 		repo_find_unique_abbrev_r(the_repository, p, oid,
-					  default_abbrev);
+					  repo_default_abbrev(the_repository));
 
 		/*
 		 * We may need to extend the abbreviated hash so that there is
@@ -5875,7 +5875,8 @@ static const char *label_oid(struct object_id *oid, const char *label,
 				strbuf_addch(buf, '-');
 		if (!buf->len) {
 			strbuf_addstr(buf, "rev-");
-			strbuf_add_unique_abbrev(buf, oid, default_abbrev);
+			strbuf_add_unique_abbrev(buf, oid,
+						 repo_default_abbrev(the_repository));
 		}
 		label = buf->buf;
 
-- 
2.43.0

[PATCH v1 2/3] environment: migrate pack_size_limit_cfg into repo_config_values

[Tian Yuchen]

Move the global 'pack_size_limit_cfg' configuration into the
repository-specific 'repo_config_values' struct.

We do not introduce a getter for it because the readers are
limited and no hardcoded fallback values are needed.

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/pack-objects.c | 2 +-
 environment.c          | 4 ++--
 environment.h          | 3 ++-
 object-file.c          | 5 +++--
 4 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 3673b14b89..4ebcaccb09 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -5328,7 +5328,7 @@ int cmd_pack_objects(int argc,
 	if (!HAVE_THREADS && delta_search_threads != 1)
 		warning(_("no threads support, ignoring --threads"));
 	if (!pack_to_stdout && !pack_size_limit)
-		pack_size_limit = pack_size_limit_cfg;
+		pack_size_limit = cfg->pack_size_limit_cfg;
 	if (pack_to_stdout && pack_size_limit)
 		die(_("--max-pack-size cannot be used to build a pack for transfer"));
 	if (pack_size_limit && pack_size_limit < 1024*1024) {
diff --git a/environment.c b/environment.c
index 2b44778b50..ff731a9611 100644
--- a/environment.c
+++ b/environment.c
@@ -66,7 +66,6 @@ enum push_default_type push_default = PUSH_DEFAULT_UNSPECIFIED;
 #endif
 enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
 int grafts_keep_true_parents;
-unsigned long pack_size_limit_cfg;
 
 #ifndef PROTECT_HFS_DEFAULT
 #define PROTECT_HFS_DEFAULT 0
@@ -723,7 +722,7 @@ int git_default_config(const char *var, const char *value,
 	}
 
 	if (!strcmp(var, "pack.packsizelimit")) {
-		pack_size_limit_cfg = git_config_ulong(var, value, ctx->kvi);
+		cfg->pack_size_limit_cfg = git_config_ulong(var, value, ctx->kvi);
 		return 0;
 	}
 
@@ -763,4 +762,5 @@ void repo_config_values_init(struct repo_config_values *cfg)
 	cfg->core_sparse_checkout_cone = 0;
 	cfg->sparse_expect_files_outside_of_patterns = 0;
 	cfg->warn_on_object_refname_ambiguity = 1;
+	cfg->pack_size_limit_cfg = 0;
 }
diff --git a/environment.h b/environment.h
index c5905d8b01..c1d5bba2f0 100644
--- a/environment.h
+++ b/environment.h
@@ -103,6 +103,8 @@ struct repo_config_values {
 	int protect_hfs;
 	int protect_ntfs;
 	int ignore_case;
+	unsigned long pack_size_limit_cfg;
+
 
 	/* section "sparse" config values */
 	int sparse_expect_files_outside_of_patterns;
@@ -188,7 +190,6 @@ extern int has_symlinks;
 extern int assume_unchanged;
 extern char *apply_default_whitespace;
 extern char *apply_default_ignorewhitespace;
-extern unsigned long pack_size_limit_cfg;
 
 enum rebase_setup_type {
 	AUTOREBASE_NEVER = 0,
diff --git a/object-file.c b/object-file.c
index 7ff2b730ac..be68eead63 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1273,6 +1273,7 @@ static int odb_transaction_files_write_object_stream(struct odb_transaction *bas
 						     size_t size,
 						     struct object_id *result_oid)
 {
+	struct repo_config_values *cfg = repo_config_values(the_repository);
 	struct odb_transaction_files *transaction = container_of(base,
 								 struct odb_transaction_files,
 								 base);
@@ -1298,8 +1299,8 @@ static int odb_transaction_files_write_object_stream(struct odb_transaction *bas
 	 * the difference between the inflated and on-disk size is limited
 	 * to zlib compression and is sufficient for this check.
 	 */
-	if (state->nr_written && pack_size_limit_cfg &&
-	    pack_size_limit_cfg < state->offset + size)
+	if (state->nr_written && cfg->pack_size_limit_cfg &&
+	    cfg->pack_size_limit_cfg < state->offset + size)
 		flush_packfile_transaction(transaction);
 
 	CALLOC_ARRAY(idx, 1);
-- 
2.43.0

[PATCH v1 3/3] environment: migrate assume_unchanged into repo_config_values

[Tian Yuchen]

Move the global 'assume_unchanged' configuration into the
repository-specific 'repo_config_values' struct.

We do not introduce a getter for it because the readers are
limited and no hardcoded fallback values are needed.

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/update-index.c | 3 ++-
 environment.c          | 4 ++--
 environment.h          | 2 +-
 read-cache.c           | 9 ++++++---
 4 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 4c4b39a157..3ef7e9bb90 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -419,6 +419,7 @@ static int add_cacheinfo(unsigned int mode, const struct object_id *oid,
 {
 	int len, option;
 	struct cache_entry *ce;
+	struct repo_config_values *cfg = repo_config_values(the_repository);
 
 	if (!verify_path(path, mode))
 		return error("Invalid path '%s'", path);
@@ -431,7 +432,7 @@ static int add_cacheinfo(unsigned int mode, const struct object_id *oid,
 	ce->ce_flags = create_ce_flags(stage);
 	ce->ce_namelen = len;
 	ce->ce_mode = create_ce_mode(mode);
-	if (assume_unchanged)
+	if (cfg->assume_unchanged)
 		ce->ce_flags |= CE_VALID;
 	option = allow_add ? ADD_CACHE_OK_TO_ADD : 0;
 	option |= allow_replace ? ADD_CACHE_OK_TO_REPLACE : 0;
diff --git a/environment.c b/environment.c
index ff731a9611..8fdb736023 100644
--- a/environment.c
+++ b/environment.c
@@ -43,7 +43,6 @@ static int zlib_compression_seen;
 
 int trust_executable_bit = 1;
 int has_symlinks = 1;
-int assume_unchanged;
 char *git_commit_encoding;
 char *git_log_output_encoding;
 char *apply_default_whitespace;
@@ -374,7 +373,7 @@ int git_default_core_config(const char *var, const char *value,
 	}
 
 	if (!strcmp(var, "core.ignorestat")) {
-		assume_unchanged = git_config_bool(var, value);
+		cfg->assume_unchanged = git_config_bool(var, value);
 		return 0;
 	}
 
@@ -758,6 +757,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
 	cfg->pack_compression_level = Z_DEFAULT_COMPRESSION;
 	cfg->minimum_abbrev = 4;
 	cfg->default_abbrev = -1;
+	cfg->assume_unchanged = 0;
 	cfg->precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
 	cfg->core_sparse_checkout_cone = 0;
 	cfg->sparse_expect_files_outside_of_patterns = 0;
diff --git a/environment.h b/environment.h
index c1d5bba2f0..a7e2bcbe30 100644
--- a/environment.h
+++ b/environment.h
@@ -97,6 +97,7 @@ struct repo_config_values {
 	int pack_compression_level;
 	int minimum_abbrev;
 	int default_abbrev;
+	int assume_unchanged;
 	int precomposed_unicode;
 	int core_sparse_checkout_cone;
 	int warn_on_object_refname_ambiguity;
@@ -187,7 +188,6 @@ int have_git_dir(void);
 /* Environment bits from configuration mechanism */
 extern int trust_executable_bit;
 extern int has_symlinks;
-extern int assume_unchanged;
 extern char *apply_default_whitespace;
 extern char *apply_default_ignorewhitespace;
 
diff --git a/read-cache.c b/read-cache.c
index 38b55323dd..643b13f1fb 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -192,9 +192,11 @@ void rename_index_entry_at(struct index_state *istate, int nr, const char *new_n
  */
 void fill_stat_cache_info(struct index_state *istate, struct cache_entry *ce, struct stat *st)
 {
+	struct repo_config_values *cfg = repo_config_values(the_repository);
+
 	fill_stat_data(&ce->ce_stat_data, st);
 
-	if (assume_unchanged)
+	if (cfg->assume_unchanged)
 		ce->ce_flags |= CE_VALID;
 
 	if (S_ISREG(st->st_mode)) {
@@ -1346,6 +1348,7 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
 {
 	struct stat st;
 	struct cache_entry *updated;
+	struct repo_config_values *cfg = repo_config_values(the_repository);
 	int changed;
 	int refresh = options & CE_MATCH_REFRESH;
 	int ignore_valid = options & CE_MATCH_IGNORE_VALID;
@@ -1405,7 +1408,7 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
 		 * is not marked VALID, this is the place to mark it
 		 * valid again, under "assume unchanged" mode.
 		 */
-		if (ignore_valid && assume_unchanged &&
+		if (ignore_valid && cfg->assume_unchanged &&
 		    !(ce->ce_flags & CE_VALID))
 			; /* mark this one VALID again */
 		else {
@@ -1440,7 +1443,7 @@ static struct cache_entry *refresh_cache_ent(struct index_state *istate,
 	 * (i.e. things to be edited) will reacquire CE_VALID bit
 	 * automatically, which is not really what we want.
 	 */
-	if (!ignore_valid && assume_unchanged &&
+	if (!ignore_valid && cfg->assume_unchanged &&
 	    !(ce->ce_flags & CE_VALID))
 		updated->ce_flags &= ~CE_VALID;
 
-- 
2.43.0

Re: [PATCH v1 0/3] environment: migrate more global variables, pt.2

[Junio C Hamano]

Tian Yuchen <cat@malon.dev> writes:

> Hi all,
>
> This series moves:
>
>  - (1/3) minimum_abbrev and default_abbrev 
>  - (2/3) pack_size_limit_cfg
>  - (3/3) assume_unchanged
>
> into repo_config_values to continue the libification effort. 
>
> Note: in commit 1/3, we need (repo != the_repository) checks in the
> getters, because some subsystems where the readers of _abbrev
> configurations live forbid the use of 'the_repository' and only accept
> 'repo' [1]. We have to explicitly intercept those intances that are
> not 'the_repository'.

Sorry but I am not sure I follow.  If a repository that is not
the_repository is not yet allowed, shouldn't the caller be flagged
for passing a random repository that is not the_repository as not
conforming to the API (yet) with:

        if (repo != the_repository)
                BUG(...);

rather than papering over the issue with an unconditional

        repo = the_repository;

override?

If the API that deals with this 'abbrev' setting needs to call
another API that only superficially takes any 'repo' parameter
without supporting anything other than the_repository, isn't that a
sign that the other API needs to be extended to work with any 'repo'
before the 'abbrev' part of the system can use it, simply because the
former is not ready?  Futzing with the 'abbrev' part of the system in
such a state piles on more unfinished work that will need to be fixed
later without achieving anything, except for the superficial "now
this part too can take a 'repo' parameter, even though it does not
support anything but the_repository", no?

Puzzled...