threads / patch / 56185

patch, 6 partsglobals: clean up some global usages

Subject: [PATCH v1 0/6] globals: clean up some global usages

## tl;dr

7 messages between Jul 31, 2021 and Jul 31, 2021. Diffs are folded; open one to read it.

replies: 6people: 1as markdown or json

Ben Boeckel· Jul 31, 2021, 02:42 UTC · lore
Hi,

This patchset moves a few of the configuration parsing functions around so that they live in the source file which uses the variables that are initialized through those functions. This allows the variables involved to be `static` rather than global variables.

Additionally, a global which is only written to is removed (`core_compression_level`), a file-local variable declared `extern` is now `static`, and explicit `0` initialization of globals is removed.

In the course of making this patch, I found a number of other global variables which could be moved into being static if more configuration parsing functions were created and moved to the relevant file instead of being parsed as part of, say, "all `core.*` settings" in a single function. If wanted, I can prepare a patchset for these as well (or provide the list for discussion of how best to proceed, but it is currently just stored in a WIP commit I have locally as comments on each variable).

Thanks,
--Ben
---
Ben Boeckel (6):
  branch: move `git_default_branch_config` to `branch.c`
  mailmap: move `git_default_mailmap_config` to `mailmap.c`
  apply: move `apply_default_*whitespace` to `apply.c`
  config: remove `core_compression_level`
  refs/debug: declare file-local variable to be static
  globals: remove explicit `0` initialization from globals
 apply.c       |  3 +++
 branch.c      | 32 ++++++++++++++++++++++++++++++++
 branch.h      |  5 +++++
 cache.h       |  6 ------
 config.c      | 43 +------------------------------------------
 environment.c |  4 ----
 mailmap.c     | 16 ++++++++++++++--
 mailmap.h     |  2 ++
 object-file.c |  2 +-
 progress.c    |  2 +-
 refs/debug.c  |  4 ++--
 11 files changed, 61 insertions(+), 58 deletions(-)
base-commit: eb27b338a3e71c7c4079fbac8aeae3f8fbb5c687
-- 
2.31.1
Ben Boeckel· Jul 31, 2021, 02:42 UTC · re: Ben Boeckel · lore

[PATCH v1 1/6] branch: move `git_default_branch_config` to `branch.c`

This allows branch-specific variables to be tracked locally in `branch.c` instead of globally in `environment.c`.

Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
---
 branch.c      | 32 ++++++++++++++++++++++++++++++++
 branch.h      |  5 +++++
 cache.h       |  1 -
 config.c      | 30 ------------------------------
 environment.c |  1 -
 5 files changed, 37 insertions(+), 32 deletions(-)
Show changes to 5 files +37 −32

branch.c, branch.h, cache.h, config.c, environment.c

diff --git a/branch.c b/branch.c
index 7a88a4861e..1ef52db89d 100644
--- a/branch.c
+++ b/branch.c
@@ -9,6 +9,38 @@
 #include "commit.h"
 #include "worktree.h"
 
+static enum rebase_setup_type autorebase = AUTOREBASE_NEVER;
+
+int git_default_branch_config(const char *var, const char *value)
+{
+	if (!strcmp(var, "branch.autosetupmerge")) {
+		if (value && !strcasecmp(value, "always")) {
+			git_branch_track = BRANCH_TRACK_ALWAYS;
+			return 0;
+		}
+		git_branch_track = git_config_bool(var, value);
+		return 0;
+	}
+	if (!strcmp(var, "branch.autosetuprebase")) {
+		if (!value)
+			return config_error_nonbool(var);
+		else if (!strcmp(value, "never"))
+			autorebase = AUTOREBASE_NEVER;
+		else if (!strcmp(value, "local"))
+			autorebase = AUTOREBASE_LOCAL;
+		else if (!strcmp(value, "remote"))
+			autorebase = AUTOREBASE_REMOTE;
+		else if (!strcmp(value, "always"))
+			autorebase = AUTOREBASE_ALWAYS;
+		else
+			return error(_("malformed value for %s"), var);
+		return 0;
+	}
+
+	/* Add other config variables here and to Documentation/config.txt. */
+	return 0;
+}
+
 struct tracking {
 	struct refspec_item spec;
 	char *src;
diff --git a/branch.h b/branch.h
index df0be61506..801ff25f92 100644
--- a/branch.h
+++ b/branch.h
@@ -17,6 +17,11 @@ extern enum branch_track git_branch_track;
 
 /* Functions for acting on the information about branches. */
 
+/*
+ * Parses branch-related configuration options.
+ */
+int git_default_branch_config(const char *var, const char *value);
+
 /*
  * Creates a new branch, where:
  *
diff --git a/cache.h b/cache.h
index ba04ff8bd3..6ea1ea5854 100644
--- a/cache.h
+++ b/cache.h
@@ -1030,7 +1030,6 @@ enum push_default_type {
 	PUSH_DEFAULT_UNSPECIFIED
 };
 
-extern enum rebase_setup_type autorebase;
 extern enum push_default_type push_default;
 
 enum object_creation_mode {
diff --git a/config.c b/config.c
index f33abeab85..840be51710 100644
--- a/config.c
+++ b/config.c
@@ -1574,36 +1574,6 @@ static int git_default_i18n_config(const char *var, const char *value)
 	return 0;
 }
 
-static int git_default_branch_config(const char *var, const char *value)
-{
-	if (!strcmp(var, "branch.autosetupmerge")) {
-		if (value && !strcasecmp(value, "always")) {
-			git_branch_track = BRANCH_TRACK_ALWAYS;
-			return 0;
-		}
-		git_branch_track = git_config_bool(var, value);
-		return 0;
-	}
-	if (!strcmp(var, "branch.autosetuprebase")) {
-		if (!value)
-			return config_error_nonbool(var);
-		else if (!strcmp(value, "never"))
-			autorebase = AUTOREBASE_NEVER;
-		else if (!strcmp(value, "local"))
-			autorebase = AUTOREBASE_LOCAL;
-		else if (!strcmp(value, "remote"))
-			autorebase = AUTOREBASE_REMOTE;
-		else if (!strcmp(value, "always"))
-			autorebase = AUTOREBASE_ALWAYS;
-		else
-			return error(_("malformed value for %s"), var);
-		return 0;
-	}
-
-	/* Add other config variables here and to Documentation/config.txt. */
-	return 0;
-}
-
 static int git_default_push_config(const char *var, const char *value)
 {
 	if (!strcmp(var, "push.default")) {
diff --git a/environment.c b/environment.c
index 2f27008424..5d45152731 100644
--- a/environment.c
+++ b/environment.c
@@ -60,7 +60,6 @@ int global_conv_flags_eol = CONV_EOL_RNDTRP_WARN;
 char *check_roundtrip_encoding = "SHIFT-JIS";
 unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
 enum branch_track git_branch_track = BRANCH_TRACK_REMOTE;
-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
-- 
2.31.1
Ben Boeckel· Jul 31, 2021, 02:42 UTC · re: Ben Boeckel · lore

[PATCH v1 3/6] apply: move `apply_default_*whitespace` to `apply.c`

These variables are only set and used in this file, so they can become `static`.

Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
---
 apply.c       | 3 +++
 cache.h       | 2 --
 environment.c | 2 --
 3 files changed, 3 insertions(+), 4 deletions(-)
Show changes to 3 files +3 −4

apply.c, cache.h, environment.c

diff --git a/apply.c b/apply.c
index 44bc31d6eb..15dcd8b7d7 100644
--- a/apply.c
+++ b/apply.c
@@ -23,6 +23,9 @@
 #include "apply.h"
 #include "entry.h"
 
+static char *apply_default_whitespace;
+static char *apply_default_ignorewhitespace;
+
 struct gitdiff_data {
 	struct strbuf *root;
 	int linenr;
diff --git a/cache.h b/cache.h
index 551a6cb5cf..dc6c4172cb 100644
--- a/cache.h
+++ b/cache.h
@@ -944,8 +944,6 @@ extern int assume_unchanged;
 extern int prefer_symlink_refs;
 extern int warn_ambiguous_refs;
 extern int warn_on_object_refname_ambiguity;
-extern char *apply_default_whitespace;
-extern char *apply_default_ignorewhitespace;
 extern const char *git_attributes_file;
 extern const char *git_hooks_path;
 extern int zlib_compression_level;
diff --git a/environment.c b/environment.c
index 5d45152731..0cc086d847 100644
--- a/environment.c
+++ b/environment.c
@@ -36,8 +36,6 @@ int repository_format_precious_objects;
 int repository_format_worktree_config;
 const char *git_commit_encoding;
 const char *git_log_output_encoding;
-char *apply_default_whitespace;
-char *apply_default_ignorewhitespace;
 const char *git_attributes_file;
 const char *git_hooks_path;
 int zlib_compression_level = Z_BEST_SPEED;
-- 
2.31.1
Ben Boeckel· Jul 31, 2021, 02:42 UTC · re: Ben Boeckel · lore

[PATCH v1 2/6] mailmap: move `git_default_mailmap_config` to `mailmap.c`

This allows mailmap-specific variables to be tracked locally in `mailmap.c` instead of globally.

Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
---
 cache.h   |  2 --
 config.c  | 12 +-----------
 mailmap.c | 16 ++++++++++++++--
 mailmap.h |  2 ++
 4 files changed, 17 insertions(+), 15 deletions(-)
Show changes to 4 files +17 −15

cache.h, config.c, mailmap.c, mailmap.h

diff --git a/cache.h b/cache.h
index 6ea1ea5854..551a6cb5cf 100644
--- a/cache.h
+++ b/cache.h
@@ -1718,8 +1718,6 @@ int author_ident_sufficiently_given(void);
 
 extern const char *git_commit_encoding;
 extern const char *git_log_output_encoding;
-extern const char *git_mailmap_file;
-extern const char *git_mailmap_blob;
 
 /* IO helper functions */
 void maybe_flush_or_die(FILE *, const char *);
diff --git a/config.c b/config.c
index 840be51710..c45001adbe 100644
--- a/config.c
+++ b/config.c
@@ -15,6 +15,7 @@
 #include "strbuf.h"
 #include "quote.h"
 #include "hashmap.h"
+#include "mailmap.h"
 #include "string-list.h"
 #include "object-store.h"
 #include "utf8.h"
@@ -1603,17 +1604,6 @@ static int git_default_push_config(const char *var, const char *value)
 	return 0;
 }
 
-static int git_default_mailmap_config(const char *var, const char *value)
-{
-	if (!strcmp(var, "mailmap.file"))
-		return git_config_pathname(&git_mailmap_file, var, value);
-	if (!strcmp(var, "mailmap.blob"))
-		return git_config_string(&git_mailmap_blob, var, value);
-
-	/* Add other config variables here and to Documentation/config.txt. */
-	return 0;
-}
-
 int git_default_config(const char *var, const char *value, void *cb)
 {
 	if (starts_with(var, "core."))
diff --git a/mailmap.c b/mailmap.c
index d1f7c0d272..e1d9a2bbdb 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "config.h"
 #include "string-list.h"
 #include "mailmap.h"
 #include "object-store.h"
@@ -12,8 +13,19 @@ static inline void debug_mm(const char *format, ...) {}
 static inline const char *debug_str(const char *s) { return s; }
 #endif
 
-const char *git_mailmap_file;
-const char *git_mailmap_blob;
+static const char *git_mailmap_file;
+static const char *git_mailmap_blob;
+
+int git_default_mailmap_config(const char *var, const char *value)
+{
+	if (!strcmp(var, "mailmap.file"))
+		return git_config_pathname(&git_mailmap_file, var, value);
+	if (!strcmp(var, "mailmap.blob"))
+		return git_config_string(&git_mailmap_blob, var, value);
+
+	/* Add other config variables here and to Documentation/config.txt. */
+	return 0;
+}
 
 struct mailmap_info {
 	char *name;
diff --git a/mailmap.h b/mailmap.h
index 7e99fccb46..48bfe9fac7 100644
--- a/mailmap.h
+++ b/mailmap.h
@@ -3,6 +3,8 @@
 
 struct string_list;
 
+int git_default_mailmap_config(const char *var, const char *value);
+
 int read_mailmap(struct string_list *map);
 void clear_mailmap(struct string_list *map);
 
-- 
2.31.1
Ben Boeckel· Jul 31, 2021, 02:42 UTC · re: Ben Boeckel · lore

[PATCH v1 4/6] config: remove `core_compression_level`

This variable is just stored to after parsing `core.compression` and then ignored, so it can just be completely removed.

Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
---
 cache.h       | 1 -
 config.c      | 1 -
 environment.c | 1 -
 3 files changed, 3 deletions(-)
Show changes to 3 files +0 −3

cache.h, config.c, environment.c

diff --git a/cache.h b/cache.h
index dc6c4172cb..4fd134863c 100644
--- a/cache.h
+++ b/cache.h
@@ -947,7 +947,6 @@ extern int warn_on_object_refname_ambiguity;
 extern const char *git_attributes_file;
 extern const char *git_hooks_path;
 extern int zlib_compression_level;
-extern int core_compression_level;
 extern int pack_compression_level;
 extern size_t packed_git_window_size;
 extern size_t packed_git_limit;
diff --git a/config.c b/config.c
index c45001adbe..b241a56cfb 100644
--- a/config.c
+++ b/config.c
@@ -1401,7 +1401,6 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
 			level = Z_DEFAULT_COMPRESSION;
 		else if (level < 0 || level > Z_BEST_COMPRESSION)
 			die(_("bad zlib compression level %d"), level);
-		core_compression_level = level;
 		core_compression_seen = 1;
 		if (!zlib_compression_seen)
 			zlib_compression_level = level;
diff --git a/environment.c b/environment.c
index 0cc086d847..3b995434fc 100644
--- a/environment.c
+++ b/environment.c
@@ -39,7 +39,6 @@ const char *git_log_output_encoding;
 const char *git_attributes_file;
 const char *git_hooks_path;
 int zlib_compression_level = Z_BEST_SPEED;
-int core_compression_level;
 int pack_compression_level = Z_DEFAULT_COMPRESSION;
 int fsync_object_files;
 size_t packed_git_window_size = DEFAULT_PACKED_GIT_WINDOW_SIZE;
-- 
2.31.1
Ben Boeckel· Jul 31, 2021, 02:42 UTC · re: Ben Boeckel · lore

[PATCH v1 5/6] refs/debug: declare file-local variable to be static

Declaring a variable in a source file as `extern` and then defining it later is not very useful. It can instead be declared as `static`.

Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
---
 refs/debug.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to refs/debug.c +2 −2
diff --git a/refs/debug.c b/refs/debug.c
index 7db4abccc3..f5609408f3 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -9,7 +9,7 @@ struct debug_ref_store {
 	struct ref_store *refs;
 };
 
-extern struct ref_storage_be refs_be_debug;
+static struct ref_storage_be refs_be_debug;
 
 struct ref_store *maybe_debug_wrap_ref_store(const char *gitdir, struct ref_store *store)
 {
@@ -413,7 +413,7 @@ static int debug_reflog_expire(struct ref_store *ref_store, const char *refname,
 	return res;
 }
 
-struct ref_storage_be refs_be_debug = {
+static struct ref_storage_be refs_be_debug = {
 	NULL,
 	"debug",
 	NULL,
-- 
2.31.1
Ben Boeckel· Jul 31, 2021, 02:42 UTC · re: Ben Boeckel · lore

[PATCH v1 6/6] globals: remove explicit `0` initialization from globals

Git relies on implicit zero-initialization of global variables.
Signed-off-by: Ben Boeckel <mathstuf@gmail.com>
---
 object-file.c | 2 +-
 progress.c    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
Show changes to 2 files +2 −2

object-file.c, progress.c

diff --git a/object-file.c b/object-file.c
index ecca5a8da0..6a04013342 100644
--- a/object-file.c
+++ b/object-file.c
@@ -1503,7 +1503,7 @@ static int loose_object_info(struct repository *r,
 	return (status < 0) ? status : 0;
 }
 
-int obj_read_use_lock = 0;
+int obj_read_use_lock;
 pthread_mutex_t obj_read_mutex;
 
 void enable_obj_read_lock(void)
diff --git a/progress.c b/progress.c
index 680c6a8bf9..4fb4233b67 100644
--- a/progress.c
+++ b/progress.c
@@ -52,7 +52,7 @@ static volatile sig_atomic_t progress_update;
  * for 'test-tool progress'.
  */
 int progress_testing;
-uint64_t progress_test_ns = 0;
+uint64_t progress_test_ns;
 void progress_test_force_update(void)
 {
 	progress_update = 1;
-- 
2.31.1

← back to recent threads