config: move pack size limit into repo config
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
@@ -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
@@ -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
@@ -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
@@ -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
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
Signed-off-by Joy Gundu <[email protected]>[...]
Sent from mobile
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_valuesrather 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@@ -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
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_valuesrather 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@@ -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 firstmemset(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
v2 cover config: move pack size limit into repo config
In reply to Joy via GitGitGadgetThis 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
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
@@ -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
PATCH v2 2/2 config: move pack size limit into repo config
In reply to Joy via GitGitGadgetFrom: 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
@@ -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
@@ -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
@@ -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
@@ -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
Re: [PATCH v2 1/2] repo: zero-initialize configuration values
In reply to Joy Gundu via GitGitGadgetQuoted 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
Signed-off-by Joy Gundu <[email protected]>--- environment.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)environment.c +2
-9@@ -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
-3cfg->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
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_valuesand 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
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
Re: [PATCH v2 1/2] repo: zero-initialize configuration values
In reply to Patrick SteinhardtHi 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_valueswe 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
Signed-off-by Joy Gundu <[email protected]>--- environment.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)environment.c +2
-9@@ -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
-3cfg->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
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
v3 cover config: move pack size limit into repo config
In reply to Joy via GitGitGadgetThis 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
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
@@ -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
PATCH v3 2/2 config: move pack size limit into repo config
In reply to Joy via GitGitGadgetFrom: 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
@@ -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
@@ -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
@@ -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
@@ -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
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