Skip to contentSkip to conversation

config: move pack size limit into repo config

PATCH Superseded Joy via GitGitGadget 5 participants 16 messages Oct 20 – Nov 5

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 config: move pack size limit into repo config

From: Joy Gundu <[email protected]>

Store the pack.packsizelimit value in repository configuration rather than keeping it in a standalone global variable for the repository.

Update the configuration parser and pack-writing code to read the value from repo_config_values rather than a global variable.

This change removes one piece of global configuration state from Git and helps make Git code easier to reuse as a library component.

---
config: move pack size limit into repo config

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2271%2FJoiejoie1%2Foutreachy%2Fglobal-state-mini-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2271/Joiejoie1/outreachy/global-state-mini-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2271

 builtin/pack-objects.c | 2 +-
 environment.c          | 4 ++--
 environment.h          | 3 +--
 object-file.c          | 6 ++++--
 4 files changed, 8 insertions(+), 7 deletions(-)
builtin/pack-objects.c +1 -1
index af9390a46b..1c9044060d 100644
@@ -5380,7 +5380,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;
 	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) {
environment.c +2 -2
index c83cf44839..e350d6949e 100644
@@ -57,7 +57,6 @@ char *check_roundtrip_encoding;
 #define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
 #endif
 int grafts_keep_true_parents;
-unsigned long pack_size_limit_cfg;
 
 #ifndef PROTECT_HFS_DEFAULT
 #define PROTECT_HFS_DEFAULT 0
@@ -728,7 +727,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 = git_config_ulong(var, value, ctx->kvi);
 		return 0;
 	}
 
@@ -768,6 +767,7 @@ 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->pack_size_limit = 0;
 	cfg->precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
 	cfg->core_sparse_checkout_cone = 0;
 	cfg->warn_on_object_refname_ambiguity = 1;
environment.h +1 -2
index b336459e90..070314f7ef 100644
@@ -132,6 +132,7 @@ struct repo_config_values {
 	int check_stat;
 	int zlib_compression_level;
 	int pack_compression_level;
+	unsigned long pack_size_limit;
 	int precomposed_unicode;
 	int core_sparse_checkout_cone;
 	int warn_on_object_refname_ambiguity;
@@ -235,8 +236,6 @@ int have_git_dir(void);
 /* Environment bits from configuration mechanism */
 extern int minimum_abbrev, default_abbrev;
 extern int assume_unchanged;
-extern unsigned long pack_size_limit_cfg;
-
 extern int grafts_keep_true_parents;
 
 const char *get_log_output_encoding(void);
object-file.c +4 -2
index 210984f825..3178bd6e65 100644
@@ -885,6 +885,8 @@ static int odb_transaction_files_write_object_stream(struct odb_transaction *bas
 								 struct odb_transaction_files,
 								 base);
 	struct transaction_packfile *state = &transaction->packfile;
+	struct repo_config_values *cfg =
+		repo_config_values(the_repository);
 	struct git_hash_ctx ctx;
 	unsigned char obuf[16384];
 	unsigned header_len;
@@ -906,8 +908,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 + stream->size) {
+	if (state->nr_written && cfg->pack_size_limit &&
+	    cfg->pack_size_limit < state->offset + stream->size) {
 		flush_packfile_transaction(transaction);
 		odb_reprepare(transaction->base.source->odb);
 	}

base-commit: c51408889c3b372814bc20b9431c3b74d0d268eb

Signature · 1 line
gitgitgadget
#1 lore raw
Quoted 6 lines · Joy via GitGitGadget

From: Joy Gundu <[email protected]>

[...]

This change removes one piece of global configuration state from Git and helps make Git code easier to reuse as a library component.

It's nice to see a paragraph explaining the
overall project in plain terms. Not just
"we are getting rid of globals" or "this is
libification".

Quoted 3 lines

Sent from mobile

#2 lore raw
In reply to Joy via GitGitGadget
Quoted 10 lines · Joy via GitGitGadget

From: Joy Gundu <[email protected]>

Store the pack.packsizelimit value in repository configuration rather than keeping it in a standalone global variable for the repository.

Update the configuration parser and pack-writing code to read the value from repo_config_values rather than a global variable.

This change removes one piece of global configuration state from Git and helps make Git code easier to reuse as a library component.

The commit message typically starts with an observation, followed by an explanation what's bad about the observed status quo and then finally explains how that state is improved. The order you have here does not match that typical flow.

Quoted 14 lines
environment.c +1 -1
index c83cf44839..e350d6949e 100644
@@ -728,7 +727,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 = git_config_ulong(var, value, ctx->kvi);
 		return 0;
 	}
 

environment.c void repo_config_values_init(struct repo_config_values *cfg) · line 773

Quoted 7 lines
environment.c +1 -0
@@ -768,6 +767,7 @@ 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->pack_size_limit = 0;
 	cfg->precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
 	cfg->core_sparse_checkout_cone = 0;
 	cfg->warn_on_object_refname_ambiguity = 1;

Hmm, quite annoying that we need to explicitly zero out all these fields. Maybe we should adapt repo_config_values_init() to first memset(cfg, 0, sizeof(*cfg))? That would allow us to remove a lot of assignments. If you want to do that, it would be great to do it as a preparatory patch in the same series.

Other than that the diff looks exactly the same as a local patch I had already written myself, so thanks for allowing me to worry about one less thing :)

Patrick

#3 lore raw

Hi Patrick,

Thanks for the feedback! I’ll take a closer look at zero-initializing repo_config_values and see if it makes sense to include it as a preparatory patch.

I’ll also keep your advice on commit message structure in mind for future patches.

Thanks again for taking the time to review my work!

Best,
Joy

Quoted 51 lines · Patrick Steinhardt
Quoted 10 lines · Joy via GitGitGadget

From: Joy Gundu <[email protected]>

Store the pack.packsizelimit value in repository configuration rather than keeping it in a standalone global variable for the repository.

Update the configuration parser and pack-writing code to read the value from repo_config_values rather than a global variable.

This change removes one piece of global configuration state from Git and helps make Git code easier to reuse as a library component.

The commit message typically starts with an observation, followed by an explanation what's bad about the observed status quo and then finally explains how that state is improved. The order you have here does not match that typical flow.

Quoted 14 lines
environment.c +1 -1
index c83cf44839..e350d6949e 100644
@@ -728,7 +727,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 = git_config_ulong(var, value, ctx->kvi);
              return 0;
      }
 

environment.c void repo_config_values_init(struct repo_config_values *cfg) · line 773

Quoted 7 lines
environment.c +1 -0
@@ -768,6 +767,7 @@ 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->pack_size_limit = 0;
      cfg->precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
      cfg->core_sparse_checkout_cone = 0;
      cfg->warn_on_object_refname_ambiguity = 1;

Hmm, quite annoying that we need to explicitly zero out all these fields. Maybe we should adapt repo_config_values_init() to first memset(cfg, 0, sizeof(*cfg))? That would allow us to remove a lot of assignments. If you want to do that, it would be great to do it as a preparatory patch in the same series.

Other than that the diff looks exactly the same as a local patch I had already written myself, so thanks for allowing me to worry about one less thing :)

Patrick

#4 lore raw

v2 cover config: move pack size limit into repo config

In reply to Joy via GitGitGadget

This series moves pack.packsizelimit from a standalone global variable into struct repo_config_values, as part of the ongoing effort to reduce Git's reliance on global state and make its code easier to reuse as a library.

The series includes two patches:

1. Update repo_config_values_init() to use memset() for zero-initialization, while keeping the defaults that require explicit values.
2. Move the pack size limit into repo_config_values and update the relevant code to read it from there.

Changes since v1
================

Thanks to Patrick Steinhardt for reviewing the first version and suggesting the initialization cleanup.

Following his feedback, I've:

* Added a preparatory patch to simplify how repository configuration values are initialized.
* Reworked the main commit message to explain the existing problem before describing the change.
* Removed the need to explicitly initialize the new pack size limit field to zero.

Testing
=======

I built Git and ran the relevant configuration and pack-object tests:

* make -j2 — passed.
* t1300-config.sh — 518 successful tests.
* t5300-pack-object.sh — 63 passed, 2 skipped, 0 failed.

Both test runs reported permission errors during cleanup after the tests had completed.

Joy Gundu (2):
repo: zero-initialize configuration values
config: move pack size limit into repo config

 builtin/pack-objects.c |  2 +-
 environment.c          | 17 +++--------------
 environment.h          |  3 +--
 object-file.c          |  6 ++++--
 4 files changed, 9 insertions(+), 19 deletions(-)

base-commit: 47fdf2208c5b94c8cc01c106b8f7c85efb91c926
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2271%2FJoiejoie1%2Foutreachy%2Fglobal-state-mini-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2271/Joiejoie1/outreachy/global-state-mini-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2271

Range-diff vs v1:

 -:  ---------- > 1:  df93a2a86c repo: zero-initialize configuration values
 1:  e3e36c3459 ! 2:  05ae50b5da config: move pack size limit into repo config

@@ Metadata
## Commit message ##
config: move pack size limit into repo config

- Store the pack.packsizelimit value in repository configuration
- rather than keeping it in a standalone global variable for the repository.
+ The pack size limit is currently stored in a standalone global
+ variable, even though it is repository configuration.

- Update the configuration parser and pack-writing code to read
- the value from repo_config_values rather than a global variable.
+ Move the value into repo_config_values and update the configuration
+ parser and pack-writing code to read it from the repository's
+ configuration values.

- This change removes one piece of global configuration state from Git
- and helps make Git code easier to reuse as a library component.
+ This removes one piece of global configuration state and helps
+ make Git code easier to reuse as a library component.

@@ environment.c: int git_default_config(const char *var, const char *value,
return 0;
}

-@@ environment.c: 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->pack_size_limit = 0;
- cfg->precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
- cfg->core_sparse_checkout_cone = 0;
- cfg->warn_on_object_refname_ambiguity = 1;

## environment.h ##
@@ environment.h: struct repo_config_values {

Signature · 1 line
gitgitgadget
#5 lore raw

PATCH v2 1/2 repo: zero-initialize configuration values

From: Joy Gundu <[email protected]>

---
 environment.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)
environment.c +2 -12
index c83cf44839..89af09aa40 100644
@@ -752,34 +752,24 @@ int git_default_config(const char *var, const char *value,
 
 void repo_config_values_init(struct repo_config_values *cfg)
 {
+	memset(cfg, 0, sizeof(*cfg));
+
 	/* section "core" config values */
-	cfg->attributes_file = NULL;
-	cfg->excludes_file = NULL;
-	cfg->editor_program = NULL;
-	cfg->pager_program = NULL;
-	cfg->askpass_program = NULL;
-	cfg->apply_default_whitespace = NULL;
-	cfg->apply_default_ignorewhitespace = NULL;
 	cfg->push_default = PUSH_DEFAULT_UNSPECIFIED;
 	cfg->autorebase = AUTOREBASE_NEVER;
 	cfg->object_creation_mode = OBJECT_CREATION_MODE;
-	cfg->apply_sparse_checkout = 0;
 	cfg->trust_ctime = 1;
 	cfg->check_stat = 1;
 	cfg->zlib_compression_level = Z_BEST_SPEED;
 	cfg->pack_compression_level = Z_DEFAULT_COMPRESSION;
 	cfg->precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
-	cfg->core_sparse_checkout_cone = 0;
 	cfg->warn_on_object_refname_ambiguity = 1;
 	cfg->protect_hfs = PROTECT_HFS_DEFAULT;
 	cfg->protect_ntfs = PROTECT_NTFS_DEFAULT;
-	cfg->ignore_case = 0;
 	cfg->trust_executable_bit = 1;
 	cfg->has_symlinks = platform_has_symlinks();
-	cfg->use_nanosec = 0;
 
 	/* section "sparse" config values */
-	cfg->sparse_expect_files_outside_of_patterns = 0;
 
 	/* section "branch" config values */
 	cfg->branch_track = BRANCH_TRACK_REMOTE;
Signature · 1 line
gitgitgadget
#9 lore raw

PATCH v2 2/2 config: move pack size limit into repo config

In reply to Joy via GitGitGadget

From: Joy Gundu <[email protected]>

The pack size limit is currently stored in a standalone global variable, even though it is repository configuration.

Move the value into repo_config_values and update the configuration parser and pack-writing code to read it from the repository's configuration values.

This removes one piece of global configuration state and helps make Git code easier to reuse as a library component.

---
 builtin/pack-objects.c | 2 +-
 environment.c          | 3 +--
 environment.h          | 3 +--
 object-file.c          | 6 ++++--
 4 files changed, 7 insertions(+), 7 deletions(-)
builtin/pack-objects.c +1 -1
index ca3a891dfb..1bd6b2d566 100644
@@ -5370,7 +5370,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;
 	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) {
environment.c +1 -2
index 89af09aa40..77bbbb7649 100644
@@ -57,7 +57,6 @@ char *check_roundtrip_encoding;
 #define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
 #endif
 int grafts_keep_true_parents;
-unsigned long pack_size_limit_cfg;
 
 #ifndef PROTECT_HFS_DEFAULT
 #define PROTECT_HFS_DEFAULT 0
@@ -728,7 +727,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 = git_config_ulong(var, value, ctx->kvi);
 		return 0;
 	}
 
environment.h +1 -2
index b336459e90..070314f7ef 100644
@@ -132,6 +132,7 @@ struct repo_config_values {
 	int check_stat;
 	int zlib_compression_level;
 	int pack_compression_level;
+	unsigned long pack_size_limit;
 	int precomposed_unicode;
 	int core_sparse_checkout_cone;
 	int warn_on_object_refname_ambiguity;
@@ -235,8 +236,6 @@ int have_git_dir(void);
 /* Environment bits from configuration mechanism */
 extern int minimum_abbrev, default_abbrev;
 extern int assume_unchanged;
-extern unsigned long pack_size_limit_cfg;
-
 extern int grafts_keep_true_parents;
 
 const char *get_log_output_encoding(void);
object-file.c +4 -2
index c049bf13f7..7f55e6331f 100644
@@ -882,6 +882,8 @@ static int odb_transaction_files_write_object_stream(struct odb_transaction *bas
 								 struct odb_transaction_files,
 								 base);
 	struct transaction_packfile *state = &transaction->packfile;
+	struct repo_config_values *cfg =
+		repo_config_values(the_repository);
 	struct git_hash_ctx ctx;
 	unsigned char obuf[16384];
 	unsigned header_len;
@@ -903,8 +905,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 + stream->size) {
+	if (state->nr_written && cfg->pack_size_limit &&
+	    cfg->pack_size_limit < state->offset + stream->size) {
 		flush_packfile_transaction(transaction);
 		odb_reprepare(transaction->base.source->odb);
 	}
Signature · 1 line
gitgitgadget
#6 lore raw

Re: [PATCH v2 1/2] repo: zero-initialize configuration values

In reply to Joy Gundu via GitGitGadget
Quoted 1 line · Joy Gundu via GitGitGadget

From: Joy Gundu <[email protected]>

Even though the patch speaks for itself, we typically still like to a sentence or two in the commit message itself. E.g.:

When initializing struct repo_config_values we manually zero out a lot of its members, which is tedious. Reduce the boilerplate by first zeroing out the whole structure so that we only have to set those members that have a non-zero default value.

Quoted 33 lines
---
 environment.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)
environment.c +2 -9
index c83cf44839..89af09aa40 100644
@@ -752,34 +752,24 @@ int git_default_config(const char *var, const char *value,
 
 void repo_config_values_init(struct repo_config_values *cfg)
 {
+	memset(cfg, 0, sizeof(*cfg));
+
 	/* section "core" config values */
-	cfg->attributes_file = NULL;
-	cfg->excludes_file = NULL;
-	cfg->editor_program = NULL;
-	cfg->pager_program = NULL;
-	cfg->askpass_program = NULL;
-	cfg->apply_default_whitespace = NULL;
-	cfg->apply_default_ignorewhitespace = NULL;
 	cfg->push_default = PUSH_DEFAULT_UNSPECIFIED;
 	cfg->autorebase = AUTOREBASE_NEVER;
 	cfg->object_creation_mode = OBJECT_CREATION_MODE;
-	cfg->apply_sparse_checkout = 0;
 	cfg->trust_ctime = 1;
 	cfg->check_stat = 1;
 	cfg->zlib_compression_level = Z_BEST_SPEED;
 	cfg->pack_compression_level = Z_DEFAULT_COMPRESSION;
 	cfg->precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
-	cfg->core_sparse_checkout_cone = 0;

environment.c int git_default_config(const char *var, const char *value, · line 782

Quoted 10 lines
environment.c +0 -3
 	cfg->warn_on_object_refname_ambiguity = 1;
 	cfg->protect_hfs = PROTECT_HFS_DEFAULT;
 	cfg->protect_ntfs = PROTECT_NTFS_DEFAULT;
-	cfg->ignore_case = 0;
 	cfg->trust_executable_bit = 1;
 	cfg->has_symlinks = platform_has_symlinks();
-	cfg->use_nanosec = 0;
 
 	/* section "sparse" config values */
-	cfg->sparse_expect_files_outside_of_patterns = 0;

We should probably drop this comment together with the code.

environment.c int git_default_config(const char *var, const char *value, · line 775

Quoted 2 lines
environment.c +0 -0
 	/* section "branch" config values */
 	cfg->branch_track = BRANCH_TRACK_REMOTE;

Other than that this looks good to me.

Patrick

#10 lore raw
In reply to Joy Gundu via GitGitGadget
Quoted 11 lines · Joy Gundu via GitGitGadget

From: Joy Gundu <[email protected]>

The pack size limit is currently stored in a standalone global variable, even though it is repository configuration.

Move the value into repo_config_values and update the configuration parser and pack-writing code to read it from the repository's configuration values.

This removes one piece of global configuration state and helps make Git code easier to reuse as a library component.

This reads well to me, thanks.

Patrick

#11 lore raw
In reply to Joy via GitGitGadget
Quoted 6 lines · Joy via GitGitGadget

Changes since v1
================

Thanks to Patrick Steinhardt for reviewing the first version and suggesting the initialization cleanup.

Quoted 8 lines

Following his feedback, I've:

* Added a preparatory patch to simplify how repository configuration values are initialized.
* Reworked the main commit message to explain the existing problem before describing the change.
* Removed the need to explicitly initialize the new pack size limit field to zero.

I've got two more small nits on the first patch that you probably want to address, but other than that this looks as expected to me. Thanks!

Patrick

#7 lore raw

Re: [PATCH v2 1/2] repo: zero-initialize configuration values

In reply to Patrick Steinhardt

Hi Patrick,

Thanks for the review and the suggested wording. I'll add an explanation to the commit message and remove the now-unnecessary comment.

I'll include both changes in the next version of the series.

Thanks again,
Joy

Quoted 64 lines · Patrick Steinhardt
Quoted 1 line · Joy Gundu via GitGitGadget

From: Joy Gundu <[email protected]>

Even though the patch speaks for itself, we typically still like to a sentence or two in the commit message itself. E.g.:

When initializing struct repo_config_values we manually zero out a lot of its members, which is tedious. Reduce the boilerplate by first zeroing out the whole structure so that we only have to set those members that have a non-zero default value.

Quoted 33 lines
---
 environment.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)
environment.c +2 -9
index c83cf44839..89af09aa40 100644
@@ -752,34 +752,24 @@ int git_default_config(const char *var, const char *value,
 
 void repo_config_values_init(struct repo_config_values *cfg)
 {
+     memset(cfg, 0, sizeof(*cfg));
+
      /* section "core" config values */
-     cfg->attributes_file = NULL;
-     cfg->excludes_file = NULL;
-     cfg->editor_program = NULL;
-     cfg->pager_program = NULL;
-     cfg->askpass_program = NULL;
-     cfg->apply_default_whitespace = NULL;
-     cfg->apply_default_ignorewhitespace = NULL;
      cfg->push_default = PUSH_DEFAULT_UNSPECIFIED;
      cfg->autorebase = AUTOREBASE_NEVER;
      cfg->object_creation_mode = OBJECT_CREATION_MODE;
-     cfg->apply_sparse_checkout = 0;
      cfg->trust_ctime = 1;
      cfg->check_stat = 1;
      cfg->zlib_compression_level = Z_BEST_SPEED;
      cfg->pack_compression_level = Z_DEFAULT_COMPRESSION;
      cfg->precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
-     cfg->core_sparse_checkout_cone = 0;

environment.c int git_default_config(const char *var, const char *value, · line 782

Quoted 10 lines
environment.c +0 -3
      cfg->warn_on_object_refname_ambiguity = 1;
      cfg->protect_hfs = PROTECT_HFS_DEFAULT;
      cfg->protect_ntfs = PROTECT_NTFS_DEFAULT;
-     cfg->ignore_case = 0;
      cfg->trust_executable_bit = 1;
      cfg->has_symlinks = platform_has_symlinks();
-     cfg->use_nanosec = 0;
 
      /* section "sparse" config values */
-     cfg->sparse_expect_files_outside_of_patterns = 0;

We should probably drop this comment together with the code.

environment.c int git_default_config(const char *var, const char *value, · line 775

Quoted 2 lines
environment.c +0 -0
      /* section "branch" config values */
      cfg->branch_track = BRANCH_TRACK_REMOTE;

Other than that this looks good to me.

Patrick

#8 lore raw

Re: [PATCH v2 1/2] repo: zero-initialize configuration values

Hi Joy,

Quoted 7 lines · Joie Joie

Hi Patrick,

Thanks for the review and the suggested wording. I'll add an explanation to the commit message and remove the now-unnecessary comment.

I'll include both changes in the next version of the series.

Thanks.

One note though: we don't top-post over here, but instead write our replies below the parts that we're replying to (like I do here).

Patrick

#12 lore raw

v3 cover config: move pack size limit into repo config

In reply to Joy via GitGitGadget

This series moves pack.packsizelimit from a standalone global variable into struct repo_config_values, as part of the ongoing effort to reduce Git's reliance on global state and make its code easier to reuse as a library.

The series includes two patches:

1. Update repo_config_values_init() to use memset() for zero-initialization, while keeping the defaults that require explicit values.
2. Move the pack size limit into repo_config_values and update the relevant code to read it from there.

Changes since v1
================

Thanks to Patrick Steinhardt for reviewing the first version and suggesting the initialization cleanup.

Following his feedback, I've:

* Added a preparatory patch to simplify how repository configuration values are initialized.
* Reworked the main commit message to explain the existing problem before describing the change.
* Removed the need to explicitly initialize the new pack size limit field to zero.

Testing
=======

I built Git and ran the relevant configuration and pack-object tests:

* make -j2 — passed.
* t1300-config.sh — 518 successful tests.
* t5300-pack-object.sh — 63 passed, 2 skipped, 0 failed.

Both test runs reported permission errors during cleanup after the tests had completed.

Joy Gundu (2):
repo: zero-initialize configuration values
config: move pack size limit into repo config

 builtin/pack-objects.c |  2 +-
 environment.c          | 19 +++----------------
 environment.h          |  3 +--
 object-file.c          |  6 ++++--
 4 files changed, 9 insertions(+), 21 deletions(-)

base-commit: 47fdf2208c5b94c8cc01c106b8f7c85efb91c926
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2271%2FJoiejoie1%2Foutreachy%2Fglobal-state-mini-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2271/Joiejoie1/outreachy/global-state-mini-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2271

Range-diff vs v2:

1: df93a2a86c ! 1: dc18b66976 repo: zero-initialize configuration values
@@ Metadata
## Commit message ##
repo: zero-initialize configuration values

+ When initializing struct repo_config_values, we explicitly initialize
+ several members to zero or NULL. This adds unnecessary boilerplate as
+ more configuration values are added to the structure.
+
+ Zero-initialize the entire structure with memset() instead, leaving
+ only members with non-zero defaults to be initialized individually.
+

## environment.c ##
@@ environment.c: int git_default_config(const char *var, const char *value,
cfg->has_symlinks = platform_has_symlinks();
- cfg->use_nanosec = 0;

- /* section "sparse" config values */
+- /* section "sparse" config values */
- cfg->sparse_expect_files_outside_of_patterns = 0;

/* section "branch" config values */
2: 05ae50b5da ! 2: ba34e82a0a config: move pack size limit into repo config
@@ environment.c: int git_default_config(const char *var, const char *value,
return 0;
}

+@@ environment.c: void repo_config_values_init(struct repo_config_values *cfg)
+ cfg->trust_executable_bit = 1;
+ cfg->has_symlinks = platform_has_symlinks();
+
+-
+ /* section "branch" config values */
+ cfg->branch_track = BRANCH_TRACK_REMOTE;
+ }

## environment.h ##
@@ environment.h: struct repo_config_values {

Signature · 1 line
gitgitgadget
#13 lore raw

PATCH v3 1/2 repo: zero-initialize configuration values

From: Joy Gundu <[email protected]>

When initializing struct repo_config_values, we explicitly initialize several members to zero or NULL. This adds unnecessary boilerplate as more configuration values are added to the structure.

Zero-initialize the entire structure with memset() instead, leaving only members with non-zero defaults to be initialized individually.

---
 environment.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)
environment.c +2 -13
index c83cf44839..5a2eb8b2e4 100644
@@ -752,34 +752,23 @@ int git_default_config(const char *var, const char *value,
 
 void repo_config_values_init(struct repo_config_values *cfg)
 {
+	memset(cfg, 0, sizeof(*cfg));
+
 	/* section "core" config values */
-	cfg->attributes_file = NULL;
-	cfg->excludes_file = NULL;
-	cfg->editor_program = NULL;
-	cfg->pager_program = NULL;
-	cfg->askpass_program = NULL;
-	cfg->apply_default_whitespace = NULL;
-	cfg->apply_default_ignorewhitespace = NULL;
 	cfg->push_default = PUSH_DEFAULT_UNSPECIFIED;
 	cfg->autorebase = AUTOREBASE_NEVER;
 	cfg->object_creation_mode = OBJECT_CREATION_MODE;
-	cfg->apply_sparse_checkout = 0;
 	cfg->trust_ctime = 1;
 	cfg->check_stat = 1;
 	cfg->zlib_compression_level = Z_BEST_SPEED;
 	cfg->pack_compression_level = Z_DEFAULT_COMPRESSION;
 	cfg->precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
-	cfg->core_sparse_checkout_cone = 0;
 	cfg->warn_on_object_refname_ambiguity = 1;
 	cfg->protect_hfs = PROTECT_HFS_DEFAULT;
 	cfg->protect_ntfs = PROTECT_NTFS_DEFAULT;
-	cfg->ignore_case = 0;
 	cfg->trust_executable_bit = 1;
 	cfg->has_symlinks = platform_has_symlinks();
-	cfg->use_nanosec = 0;
 
-	/* section "sparse" config values */
-	cfg->sparse_expect_files_outside_of_patterns = 0;
 
 	/* section "branch" config values */
 	cfg->branch_track = BRANCH_TRACK_REMOTE;
Signature · 1 line
gitgitgadget
#14 lore raw

PATCH v3 2/2 config: move pack size limit into repo config

In reply to Joy via GitGitGadget

From: Joy Gundu <[email protected]>

The pack size limit is currently stored in a standalone global variable, even though it is repository configuration.

Move the value into repo_config_values and update the configuration parser and pack-writing code to read it from the repository's configuration values.

This removes one piece of global configuration state and helps make Git code easier to reuse as a library component.

---
 builtin/pack-objects.c | 2 +-
 environment.c          | 4 +---
 environment.h          | 3 +--
 object-file.c          | 6 ++++--
 4 files changed, 7 insertions(+), 8 deletions(-)
builtin/pack-objects.c +1 -1
index ca3a891dfb..1bd6b2d566 100644
@@ -5370,7 +5370,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;
 	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) {
environment.c +1 -3
index 5a2eb8b2e4..b004029025 100644
@@ -57,7 +57,6 @@ char *check_roundtrip_encoding;
 #define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
 #endif
 int grafts_keep_true_parents;
-unsigned long pack_size_limit_cfg;
 
 #ifndef PROTECT_HFS_DEFAULT
 #define PROTECT_HFS_DEFAULT 0
@@ -728,7 +727,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 = git_config_ulong(var, value, ctx->kvi);
 		return 0;
 	}
 
@@ -769,7 +768,6 @@ void repo_config_values_init(struct repo_config_values *cfg)
 	cfg->trust_executable_bit = 1;
 	cfg->has_symlinks = platform_has_symlinks();
 
-
 	/* section "branch" config values */
 	cfg->branch_track = BRANCH_TRACK_REMOTE;
 }
environment.h +1 -2
index b336459e90..070314f7ef 100644
@@ -132,6 +132,7 @@ struct repo_config_values {
 	int check_stat;
 	int zlib_compression_level;
 	int pack_compression_level;
+	unsigned long pack_size_limit;
 	int precomposed_unicode;
 	int core_sparse_checkout_cone;
 	int warn_on_object_refname_ambiguity;
@@ -235,8 +236,6 @@ int have_git_dir(void);
 /* Environment bits from configuration mechanism */
 extern int minimum_abbrev, default_abbrev;
 extern int assume_unchanged;
-extern unsigned long pack_size_limit_cfg;
-
 extern int grafts_keep_true_parents;
 
 const char *get_log_output_encoding(void);
object-file.c +4 -2
index c049bf13f7..7f55e6331f 100644
@@ -882,6 +882,8 @@ static int odb_transaction_files_write_object_stream(struct odb_transaction *bas
 								 struct odb_transaction_files,
 								 base);
 	struct transaction_packfile *state = &transaction->packfile;
+	struct repo_config_values *cfg =
+		repo_config_values(the_repository);
 	struct git_hash_ctx ctx;
 	unsigned char obuf[16384];
 	unsigned header_len;
@@ -903,8 +905,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 + stream->size) {
+	if (state->nr_written && cfg->pack_size_limit &&
+	    cfg->pack_size_limit < state->offset + stream->size) {
 		flush_packfile_transaction(transaction);
 		odb_reprepare(transaction->base.source->odb);
 	}
Signature · 1 line
gitgitgadget
#15 lore raw

Hello Joy,

Thanks for the new version.

I fetched v3 with the Fetch-It-Via line and built it. t1300, t5300 and t7700 pass, and pack.packSizeLimit still works: with "-c pack.packSizeLimit=1m", "git repack -ad" on three 700 kB files gives three packs.

One thing you may not have seen: Junio has a patch, "repo_config_values: freeze the structure for now" [1], resent to the list on 3 November. It adds a comment to struct repo_config_values saying not to add any new members until it can work on a repository other than the_repository, and Patrick acked it. It is in 'seen' as jc/freeze-repo-config-values, and 2/2 goes against what that comment asks.

Separately, v3 does not merge cleanly with 'seen' because object-file.c conflicts with Christian's cc/pack-index-v3.

Quoted 1 line · Joy Gundu via GitGitGadget

+ unsigned long pack_size_limit;

Since 2/2 adds a member to that structure, I think it cannot go in like this for now. 1/2 does not add one, so maybe it could be sent on its own? Christian or Usman probably know better what to do next.

Two small things in case you send another version. 1/2 leaves two blank lines in a row in repo_config_values_init(), and 2/2 removes one of them, so that fix belongs in 1/2. And the cover letter still says "Changes since v1" and lists the v1 to v2 changes, so it does not say what changed in v3.

[1] https://lore.kernel.org/git/[email protected]/

Thanks,
Tanga