repo_config_values: migrate more globals
cover repo_config_values: migrate more globals
Base series
===========
This series builds on top of the merged series:
[PATCH v7 0/3] Move repo-specific globals into repo_config_values
available at:
https://lore.kernel.org/git/<[email protected]>
It should be applied on top of that series.
Overview
========
This series continues the effort to migrate repository-specific global variables into struct repo_config_values, further reducing global state and improving repository isolation.
The previous series introduced the infrastructure for storing repository-dependent configuration in repo_config_values. This series extends that work by moving additional environment-related globals into that struct and updating their call sites accordingly.
This helps avoid cross-repository interference when multiple repositories are used within the same process, and continues preparing Git for improved library usage and better encapsulation of repository state.
Olamide Caleb Bello (8):
environment: move "trust_ctime" into struct repo_config_values
environment: move "check_stat" into struct repo_config_values
environment: move zlib_compression_level into repo_config_values
environment: move "pack_compression_level" into `struct
repo_config_values`
environment: move "precomposed_unicode" into `struct
repo_config_values`
env: move "core_sparse_checkout_cone" into struct repo_config_values
env: put "sparse_expect_files_outside_of_patterns" in
repo_config_values
env: move "warn_on_object_refname_ambiguity" into repo_config_values
builtin/cat-file.c | 7 ++++--- builtin/fast-import.c | 8 +++++--- builtin/index-pack.c | 3 ++- builtin/mv.c | 2 +- builtin/pack-objects.c | 15 ++++++++++----- builtin/sparse-checkout.c | 37 +++++++++++++++++++++--------------- compat/precompose_utf8.c | 20 +++++++++++++------- diff.c | 3 ++- dir.c | 3 ++- entry.c | 3 ++- environment.c | 40 +++++++++++++++++++++------------------ environment.h | 19 ++++++++++--------- http-push.c | 3 ++- object-file.c | 6 ++++-- object-name.c | 3 ++- revision.c | 7 ++++--- sparse-index.c | 4 ++-- statinfo.c | 12 +++++++----- submodule.c | 7 ++++--- upload-pack.c | 3 ++- 20 files changed, 122 insertions(+), 83 deletions(-)
Signature · 1 line
2.53.0.155.g9f36b15afa
PATCH 1/8 environment: move "trust_ctime" into `struct repo_config_values`
The core.trustctime configuration is currently stored in the global variable trust_ctime, which makes it shared across repository instances in a single process.
Store it instead in repo_config_values, so the value is tied to the repository from which it was read. This preserves existing behavior while avoiding cross-repository state leakage and continues the effort to reduce reliance on global configuration state.
Update all references to use repo_config_values().
--- environment.c | 4 ++-- environment.h | 2 +- statinfo.c | 6 ++++-- 3 files changed, 7 insertions(+), 5 deletions(-)
environment.c +2 -2
@@ -42,7 +42,6 @@ static int pack_compression_seen;static int zlib_compression_seen;int trust_executable_bit =1 ; -int trust_ctime =1 ;int check_stat =1 ;int has_symlinks =1 ;int minimum_abbrev =4 , default_abbrev =-1 ;
@@ -309,7 +308,7 @@ int git_default_core_config(const char *var, const char *value,return 0 ; }if (!strcmp (var,"core.trustctime" )) { - trust_ctime =git_config_bool (var, value); + cfg->trust_ctime =git_config_bool (var, value);return 0 ; }if (!strcmp (var,"core.checkstat" )) {
@@ -721,4 +720,5 @@ void repo_config_values_init(struct repo_config_values *cfg) cfg->attributes_file =NULL ; cfg->apply_sparse_checkout =0 ; cfg->branch_track =BRANCH_TRACK_REMOTE ; + cfg->trust_ctime =1 ; }
environment.h +1 -1
@@ -91,6 +91,7 @@ struct repo_config_values {/* section "core" config values */ char *attributes_file;int apply_sparse_checkout; +int trust_ctime;/* section "branch" config values */ enum branch_track branch_track;
@@ -161,7 +162,6 @@ extern char *git_work_tree_cfg;/* Environment bits from configuration mechanism */ extern int trust_executable_bit; -extern int trust_ctime;extern int check_stat;extern int has_symlinks;extern int minimum_abbrev, default_abbrev;
statinfo.c +4 -2
@@ -3,6 +3,7 @@#include "git-compat-util.h" #include "environment.h" #include "statinfo.h" +#include "repository.h" /* * Munge st_size into an unsigned int.
@@ -63,17 +64,18 @@ void fake_lstat_data(const struct stat_data *sd, struct stat *st)int match_stat_data (const struct stat_data *sd,struct stat *st) {int changed =0 ; +struct repo_config_values *cfg =repo_config_values (the_repository);if (sd->sd_mtime.sec != (unsigned int )st->st_mtime) changed |=MTIME_CHANGED ; -if (trust_ctime && check_stat && +if (cfg->trust_ctime && check_stat && sd->sd_ctime.sec != (unsigned int )st->st_ctime) changed |=CTIME_CHANGED ;#ifdef USE_NSEC if (check_stat && sd->sd_mtime.nsec !=ST_MTIME_NSEC (*st)) changed |=MTIME_CHANGED ; -if (trust_ctime && check_stat && +if (cfg->trust_ctime && check_stat && sd->sd_ctime.nsec !=ST_CTIME_NSEC (*st)) changed |=CTIME_CHANGED ;#endif
Signature · 1 line
2.53.0.155.g9f36b15afa
PATCH 2/8 environment: move "check_stat" into `struct repo_config_values`
In reply to Olamide Caleb BelloThe core.checkstat configuration is currently stored in the global variable check_stat, which makes it shared across repository instances within a single process.
Store it instead in repo_config_values so the value is associated with the repository from which it was read. This preserves existing behavior while avoiding cross-repository state leakage and continues the effort to reduce reliance on global configuration state.
Update all references to use repo_config_values().
--- entry.c | 3 ++- environment.c | 6 +++--- environment.h | 2 +- statinfo.c | 10 +++++----- 4 files changed, 11 insertions(+), 10 deletions(-)
entry.c +2 -1
@@ -443,7 +443,8 @@ static int check_path(const char *path, int len, struct stat *st, int skiplen)static void mark_colliding_entries (const struct checkout *state,struct cache_entry *ce,struct stat *st) { -int trust_ino = check_stat; +struct repo_config_values *cfg =repo_config_values (the_repository); +int trust_ino = cfg->check_stat;#if defined(GIT_WINDOWS_NATIVE ) || defined(__CYGWIN__) trust_ino =0 ;
environment.c +3 -3
@@ -42,7 +42,6 @@ static int pack_compression_seen;static int zlib_compression_seen;int trust_executable_bit =1 ; -int check_stat =1 ;int has_symlinks =1 ;int minimum_abbrev =4 , default_abbrev =-1 ;int ignore_case;
@@ -315,9 +314,9 @@ int git_default_core_config(const char *var, const char *value,if (!value)return config_error_nonbool (var);if (!strcasecmp (value,"default" )) - check_stat =1 ; + cfg->check_stat =1 ;else if (!strcasecmp (value,"minimal" )) - check_stat =0 ; + cfg->check_stat =0 ;else return error (_ ("invalid value for '%s': '%s'" ), var, value);
@@ -721,4 +720,5 @@ void repo_config_values_init(struct repo_config_values *cfg) cfg->apply_sparse_checkout =0 ; cfg->branch_track =BRANCH_TRACK_REMOTE ; cfg->trust_ctime =1 ; + cfg->check_stat =1 ; }
environment.h +1 -1
@@ -92,6 +92,7 @@ struct repo_config_values {char *attributes_file;int apply_sparse_checkout;int trust_ctime; +int check_stat;/* section "branch" config values */ enum branch_track branch_track;
@@ -162,7 +163,6 @@ extern char *git_work_tree_cfg;/* Environment bits from configuration mechanism */ extern int trust_executable_bit; -extern int check_stat;extern int has_symlinks;extern int minimum_abbrev, default_abbrev;extern int ignore_case;
statinfo.c +5 -5
@@ -68,19 +68,19 @@ int match_stat_data(const struct stat_data *sd, struct stat *st)if (sd->sd_mtime.sec != (unsigned int )st->st_mtime) changed |=MTIME_CHANGED ; -if (cfg->trust_ctime && check_stat && +if (cfg->trust_ctime && cfg->check_stat && sd->sd_ctime.sec != (unsigned int )st->st_ctime) changed |=CTIME_CHANGED ;#ifdef USE_NSEC -if (check_stat && sd->sd_mtime.nsec !=ST_MTIME_NSEC (*st)) +if (cfg->check_stat && sd->sd_mtime.nsec !=ST_MTIME_NSEC (*st)) changed |=MTIME_CHANGED ; -if (cfg->trust_ctime && check_stat && +if (cfg->trust_ctime && cfg->check_stat && sd->sd_ctime.nsec !=ST_CTIME_NSEC (*st)) changed |=CTIME_CHANGED ;#endif -if (check_stat) { +if (cfg->check_stat) {if (sd->sd_uid != (unsigned int ) st->st_uid || sd->sd_gid != (unsigned int ) st->st_gid) changed |=OWNER_CHANGED ;
@@ -94,7 +94,7 @@ int match_stat_data(const struct stat_data *sd, struct stat *st)* clients will have different views of what "device" * the filesystem is on */ -if (check_stat && sd->sd_dev != (unsigned int ) st->st_dev) +if (cfg->check_stat && sd->sd_dev != (unsigned int ) st->st_dev) changed |=INODE_CHANGED ;#endif
Signature · 1 line
2.53.0.155.g9f36b15afa
PATCH 3/8 environment: move `zlib_compression_level` into repo_config_values
In reply to Olamide Caleb BelloThe zlib_compression_level configuration is currently stored in the global variable zlib_compression_level, which makes it shared across repository instances within a single process.
Store it instead in repo_config_values so the value is associated with the repository from which it was read. This preserves existing behavior while avoiding cross-repository state leakage and continues the effort to reduce reliance on global configuration state.
Update all references to use repo_config_values().
--- builtin/index-pack.c | 3 ++- diff.c | 3 ++- environment.c | 6 +++--- environment.h | 2 +- http-push.c | 3 ++- object-file.c | 3 ++- 6 files changed, 12 insertions(+), 8 deletions(-)
builtin/index-pack.c +2 -1
@@ -1416,8 +1416,9 @@ static int write_compressed(struct hashfile *f, void *in, unsigned int size)git_zstream stream;int status;unsigned char outbuf[4096 ]; +struct repo_config_values *cfg =repo_config_values (the_repository); -git_deflate_init (&stream, zlib_compression_level); +git_deflate_init (&stream, cfg->zlib_compression_level); stream.next_in = in; stream.avail_in = size;
diff.c +2 -1
@@ -3365,8 +3365,9 @@ static unsigned char *deflate_it(char *data,int bound;unsigned char *deflated;git_zstream stream; +struct repo_config_values *cfg =repo_config_values (the_repository); -git_deflate_init (&stream, zlib_compression_level); +git_deflate_init (&stream, cfg->zlib_compression_level); bound =git_deflate_bound (&stream, size); deflated =xmalloc (bound); stream.next_out = deflated;
environment.c +3 -3
@@ -52,7 +52,6 @@ char *git_commit_encoding;char *git_log_output_encoding;char *apply_default_whitespace;char *apply_default_ignorewhitespace; -int zlib_compression_level =Z_BEST_SPEED ;int pack_compression_level =Z_DEFAULT_COMPRESSION ;int fsync_object_files =-1 ;int use_fsync =-1 ;
@@ -377,7 +376,7 @@ int git_default_core_config(const char *var, const char *value, level =Z_DEFAULT_COMPRESSION ; else if (level < 0 || level > Z_BEST_COMPRESSION)die (_ ("bad zlib compression level %d" ), level); - zlib_compression_level = level; + cfg->zlib_compression_level = level; zlib_compression_seen =1 ;return 0 ; }
@@ -389,7 +388,7 @@ int git_default_core_config(const char *var, const char *value,else if (level <0 || level >Z_BEST_COMPRESSION )die (_ ("bad zlib compression level %d" ), level);if (!zlib_compression_seen) - zlib_compression_level = level; + cfg->zlib_compression_level = level;if (!pack_compression_seen) pack_compression_level = level;return 0 ;
@@ -721,4 +720,5 @@ void repo_config_values_init(struct repo_config_values *cfg) cfg->branch_track =BRANCH_TRACK_REMOTE ; cfg->trust_ctime =1 ; cfg->check_stat =1 ; + cfg->zlib_compression_level =Z_BEST_SPEED ; }
environment.h +1 -1
@@ -93,6 +93,7 @@ struct repo_config_values {int apply_sparse_checkout;int trust_ctime;int check_stat; +int zlib_compression_level;/* section "branch" config values */ enum branch_track branch_track;
@@ -170,7 +171,6 @@ extern int assume_unchanged;extern int warn_on_object_refname_ambiguity;extern char *apply_default_whitespace;extern char *apply_default_ignorewhitespace; -extern int zlib_compression_level;extern int pack_compression_level;extern unsigned long pack_size_limit_cfg;
http-push.c +2 -1
@@ -369,13 +369,14 @@ static void start_put(struct transfer_request *request)int hdrlen;ssize_t size;git_zstream stream; +struct repo_config_values *cfg =repo_config_values (the_repository); unpacked =odb_read_object (the_repository->objects, &request->obj->oid, &type, &len); hdrlen =format_object_header (hdr,sizeof (hdr), type, len);/* Set it up */ -git_deflate_init (&stream, zlib_compression_level); +git_deflate_init (&stream, cfg->zlib_compression_level); size =git_deflate_bound (&stream, len + hdrlen);strbuf_grow (&request->buffer.buf, size); request->buffer.posn =0 ;
object-file.c +2 -1
@@ -891,6 +891,7 @@ static int start_loose_object_common(struct odb_source *source,const struct git_hash_algo *algo = source->odb->repo->hash_algo;const struct git_hash_algo *compat = source->odb->repo->compat_hash_algo;int fd; +struct repo_config_values *cfg =repo_config_values (the_repository); fd =create_tmpfile (source->odb->repo, tmp_file, filename);if (fd <0 ) {
@@ -906,7 +907,7 @@ static int start_loose_object_common(struct odb_source *source, }/* Setup zlib stream for compression */ -git_deflate_init (stream, zlib_compression_level); +git_deflate_init (stream, cfg->zlib_compression_level); stream->next_out = buf; stream->avail_out = buflen; algo->init_fn (c);
Signature · 1 line
2.53.0.155.g9f36b15afa
PATCH 4/8 environment: move "pack_compression_level" into `struct repo_config_values`
In reply to Olamide Caleb BelloThe pack_compression_level configuration is currently stored in the global variable pack_compression_level, which makes it shared across repository instances within a single process.
Store it instead in repo_config_values so the value is associated with the repository from which it was read. This preserves existing behavior while avoiding cross-repository state leakage and is another step toward eliminating repository-dependent global state.
Update all references to use repo_config_values().
--- builtin/fast-import.c | 8 +++++--- builtin/pack-objects.c | 8 ++++++-- environment.c | 8 +++++--- environment.h | 2 +- object-file.c | 3 ++- 5 files changed, 19 insertions(+), 10 deletions(-)
builtin/fast-import.c +5 -3
@@ -963,6 +963,7 @@ static int store_object(unsigned long hdrlen, deltalen;struct git_hash_ctx c;git_zstream s; +struct repo_config_values *cfg =repo_config_values (the_repository); hdrlen =format_object_header ((char *)hdr,sizeof (hdr), type, dat->len);
@@ -1001,7 +1002,7 @@ static int store_object( }else delta =NULL ; -git_deflate_init (&s, pack_compression_level); +git_deflate_init (&s, cfg->pack_compression_level);if (delta) { s.next_in = delta; s.avail_in = deltalen;
@@ -1028,7 +1029,7 @@ static int store_object(if (delta) {FREE_AND_NULL (delta); -git_deflate_init (&s, pack_compression_level); +git_deflate_init (&s, cfg->pack_compression_level); s.next_in = (void *)dat->buf; s.avail_in = dat->len; s.avail_out =git_deflate_bound (&s, s.avail_in);
@@ -1111,6 +1112,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)struct git_hash_ctx c;git_zstream s;struct hashfile_checkpoint checkpoint; +struct repo_config_values *cfg =repo_config_values (the_repository);int status =Z_OK ;/* Determine if we should auto-checkpoint. */
@@ -1130,7 +1132,7 @@ static void stream_blob(uintmax_t len, struct object_id *oidout, uintmax_t mark)crc32_begin (pack_file); -git_deflate_init (&s, pack_compression_level); +git_deflate_init (&s, cfg->pack_compression_level); hdrlen =encode_in_pack_object_header (out_buf, out_sz,OBJ_BLOB , len);
builtin/pack-objects.c +6 -2
@@ -383,8 +383,9 @@ static unsigned long do_compress(void **pptr, unsigned long size)git_zstream stream;void *in, *out;unsigned long maxsize; +struct repo_config_values *cfg =repo_config_values (the_repository); -git_deflate_init (&stream, pack_compression_level); +git_deflate_init (&stream, cfg->pack_compression_level); maxsize =git_deflate_bound (&stream, size); in = *pptr;
@@ -410,8 +411,9 @@ static unsigned long write_large_blob_data(struct odb_read_stream *st, struct haunsigned char ibuf[1024 *16 ];unsigned char obuf[1024 *16 ];unsigned long olen =0 ; +struct repo_config_values *cfg =repo_config_values (the_repository); -git_deflate_init (&stream, pack_compression_level); +git_deflate_init (&stream, cfg->pack_compression_level);for (;;) {ssize_t readlen;
@@ -4868,6 +4870,8 @@ int cmd_pack_objects(int argc,struct string_list keep_pack_list =STRING_LIST_INIT_NODUP ;struct list_objects_filter_options filter_options =LIST_OBJECTS_FILTER_INIT ; +struct repo_config_values *cfg =repo_config_values (the_repository); +int pack_compression_level = cfg->pack_compression_level;struct option pack_objects_options[] = { OPT_CALLBACK_F('q', "quiet", &progress, NULL,
environment.c +5 -3
@@ -52,7 +52,6 @@ char *git_commit_encoding;char *git_log_output_encoding;char *apply_default_whitespace;char *apply_default_ignorewhitespace; -int pack_compression_level =Z_DEFAULT_COMPRESSION ;int fsync_object_files =-1 ;int use_fsync =-1 ;enum fsync_method fsync_method =FSYNC_METHOD_DEFAULT ;
@@ -390,7 +389,7 @@ int git_default_core_config(const char *var, const char *value,if (!zlib_compression_seen) cfg->zlib_compression_level = level;if (!pack_compression_seen) - pack_compression_level = level; + cfg->pack_compression_level = level;return 0 ; }
@@ -662,6 +661,8 @@ static int git_default_attr_config(const char *var, const char *value)int git_default_config (const char *var,const char *value,const struct config_context *ctx,void *cb) { +struct repo_config_values *cfg =repo_config_values (the_repository); +if (starts_with (var,"core." ))return git_default_core_config (var, value, ctx, cb);
@@ -701,7 +702,7 @@ int git_default_config(const char *var, const char *value, level =Z_DEFAULT_COMPRESSION ; else if (level < 0 || level > Z_BEST_COMPRESSION)die (_ ("bad pack compression level %d" ), level); - pack_compression_level = level; + cfg->pack_compression_level = level; pack_compression_seen =1 ;return 0 ; }
@@ -721,4 +722,5 @@ void repo_config_values_init(struct repo_config_values *cfg) cfg->trust_ctime =1 ; cfg->check_stat =1 ; cfg->zlib_compression_level =Z_BEST_SPEED ; + cfg->pack_compression_level =Z_DEFAULT_COMPRESSION ; }
environment.h +1 -1
@@ -94,6 +94,7 @@ struct repo_config_values {int trust_ctime;int check_stat;int zlib_compression_level; +int pack_compression_level;/* section "branch" config values */ enum branch_track branch_track;
@@ -171,7 +172,6 @@ extern int assume_unchanged;extern int warn_on_object_refname_ambiguity;extern char *apply_default_whitespace;extern char *apply_default_ignorewhitespace; -extern int pack_compression_level;extern unsigned long pack_size_limit_cfg;extern int precomposed_unicode;
object-file.c +2 -1
@@ -1420,8 +1420,9 @@ static int stream_blob_to_pack(struct transaction_packfile *state,int status =Z_OK ;int write_object = (flags &INDEX_WRITE_OBJECT );off_t offset =0 ; +struct repo_config_values *cfg =repo_config_values (the_repository); -git_deflate_init (&s, pack_compression_level); +git_deflate_init (&s, cfg->pack_compression_level); hdrlen =encode_in_pack_object_header (obuf,sizeof (obuf),OBJ_BLOB , size); s.next_out = obuf + hdrlen;
Signature · 1 line
2.53.0.155.g9f36b15afa
PATCH 5/8 environment: move "precomposed_unicode" into `struct repo_config_values`
In reply to Olamide Caleb BelloThe core.precomposeunicode configuration is currently stored in the global variable precomposed_unicode, which makes it shared across repository instances within a single process.
Store it instead in repo_config_values so the value is associated with the repository from which it was read. This preserves existing behavior while avoiding cross-repository state leakage and is another step toward eliminating repository-dependent global state.
Update all references to use repo_config_values().
--- compat/precompose_utf8.c | 20 +++++++++++++------- environment.c | 4 ++-- environment.h | 2 +- upload-pack.c | 3 ++- 4 files changed, 18 insertions(+), 11 deletions(-)
compat/precompose_utf8.c +13 -7
@@ -48,16 +48,18 @@ void probe_utf8_pathname_composition(void)static const char *auml_nfc ="\xc3\xa4" ;static const char *auml_nfd ="\x61\xcc\x88" ;int output_fd; -if (precomposed_unicode !=-1 ) +struct repo_config_values *cfg =repo_config_values (the_repository); + +if (cfg->precomposed_unicode !=-1 )return ;/* We found it defined in the global config, respect it */ repo_git_path_replace (the_repository, &path,"%s" , auml_nfc); output_fd =open (path.buf,O_CREAT |O_EXCL |O_RDWR ,0600 );if (output_fd >=0 ) {close (output_fd);repo_git_path_replace (the_repository, &path,"%s" , auml_nfd); - precomposed_unicode =access (path.buf,R_OK ) ?0 :1 ; + cfg->precomposed_unicode =access (path.buf,R_OK ) ?0 :1 ;repo_config_set (the_repository,"core.precomposeunicode" , - precomposed_unicode ?"true" :"false" ); + cfg->precomposed_unicode ?"true" :"false" );repo_git_path_replace (the_repository, &path,"%s" , auml_nfc);if (unlink (path.buf))die_errno (_ ("failed to unlink '%s'" ), path.buf);
@@ -69,14 +71,16 @@ const char *precompose_string_if_needed(const char *in) {size_t inlen;size_t outlen; +struct repo_config_values *cfg =repo_config_values (the_repository); +if (!in)return NULL ;if (has_non_ascii (in, (size_t )-1 , &inlen)) {iconv_t ic_prec;char *out; -if (precomposed_unicode <0 ) -repo_config_get_bool (the_repository,"core.precomposeunicode" , &precomposed_unicode); -if (precomposed_unicode !=1 ) +if (cfg->precomposed_unicode <0 ) +repo_config_get_bool (the_repository,"core.precomposeunicode" , &cfg->precomposed_unicode); +if (cfg->precomposed_unicode !=1 )return in; ic_prec =iconv_open (repo_encoding, path_encoding);if (ic_prec == (iconv_t) -1 )
@@ -130,7 +134,9 @@ PREC_DIR *precompose_utf8_opendir(const char *dirname)struct dirent_prec_psx *precompose_utf8_readdir (PREC_DIR *prec_dir) { +struct repo_config_values *cfg =repo_config_values (the_repository);struct dirent *res; + res =readdir (prec_dir->dirp);if (res) {size_t namelenz =strlen (res->d_name) +1 ;/* \0 */
@@ -149,7 +155,7 @@ struct dirent_prec_psx *precompose_utf8_readdir(PREC_DIR *prec_dir) prec_dir->dirent_nfc->d_ino = res->d_ino; prec_dir->dirent_nfc->d_type = res->d_type; -if ((precomposed_unicode ==1 ) &&has_non_ascii (res->d_name, (size_t )-1 ,NULL )) { +if ((cfg->precomposed_unicode ==1 ) &&has_non_ascii (res->d_name, (size_t )-1 ,NULL )) {if (prec_dir->ic_precompose == (iconv_t)-1 ) {die ("iconv_open(%s,%s) failed, but needed:\n" " precomposed unicode is not supported.\n"
environment.c +2 -2
@@ -72,7 +72,6 @@ enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;int grafts_keep_true_parents;int core_sparse_checkout_cone;int sparse_expect_files_outside_of_patterns; -int precomposed_unicode =-1 ;/* see probe_utf8_pathname_composition() */ unsigned long pack_size_limit_cfg;#ifndef PROTECT_HFS_DEFAULT
@@ -532,7 +531,7 @@ int git_default_core_config(const char *var, const char *value, }if (!strcmp (var,"core.precomposeunicode" )) { - precomposed_unicode =git_config_bool (var, value); + cfg->precomposed_unicode =git_config_bool (var, value);return 0 ; }
@@ -723,4 +722,5 @@ 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->precomposed_unicode =-1 ;/* see probe_utf8_pathname_composition() */ }
environment.h +1 -1
@@ -95,6 +95,7 @@ struct repo_config_values {int check_stat;int zlib_compression_level;int pack_compression_level; +int precomposed_unicode;/* section "branch" config values */ enum branch_track branch_track;
@@ -174,7 +175,6 @@ extern char *apply_default_whitespace;extern char *apply_default_ignorewhitespace;extern unsigned long pack_size_limit_cfg; -extern int precomposed_unicode;extern int protect_hfs;extern int protect_ntfs;
upload-pack.c +2 -1
@@ -1288,6 +1288,7 @@ static int upload_pack_config(const char *var, const char *value, void *cb_data) {struct upload_pack_data *data = cb_data; +struct repo_config_values *cfg =repo_config_values (the_repository);if (!strcmp ("uploadpack.allowtipsha1inwant" , var)) {if (git_config_bool (var, value))
@@ -1318,7 +1319,7 @@ static int upload_pack_config(const char *var, const char *value,if (value) data->allow_packfile_uris =1 ; }else if (!strcmp ("core.precomposeunicode" , var)) { - precomposed_unicode =git_config_bool (var, value); + cfg->precomposed_unicode =git_config_bool (var, value); }else if (!strcmp ("transfer.advertisesid" , var)) { data->advertise_sid =git_config_bool (var, value); }
Signature · 1 line
2.53.0.155.g9f36b15afa
PATCH 6/8 env: move "core_sparse_checkout_cone" into `struct repo_config_values`
In reply to Olamide Caleb BelloThe core_sparse_checkout_cone variable was previously a global integer, uninitialized by default. Storing repository-dependent configuration in globals can lead to cross-repository state leakage.
Move it into repo_config_values and initialize it to 0 by default. This ensures predictable behavior for repositories that do not set this configuration while preserving existing semantics.
Update all references to use repo_config_values().
--- builtin/mv.c | 2 +- builtin/sparse-checkout.c | 37 ++++++++++++++++++++++--------------- dir.c | 3 ++- environment.c | 4 ++-- environment.h | 2 +- sparse-index.c | 2 +- 6 files changed, 29 insertions(+), 21 deletions(-)
builtin/mv.c +1 -1
@@ -574,7 +574,7 @@ int cmd_mv(int argc,if (ignore_sparse && cfg->apply_sparse_checkout && - core_sparse_checkout_cone) { + cfg->core_sparse_checkout_cone) {/* * NEEDSWORK: we are *not* paying attention to * "out-to-out" move (<source> is out-of-cone and
builtin/sparse-checkout.c +22 -15
@@ -73,7 +73,7 @@ static int sparse_checkout_list(int argc, const char **argv, const char *prefix,memset (&pl,0 ,sizeof (pl)); - pl.use_cone_patterns = core_sparse_checkout_cone; + pl.use_cone_patterns = cfg->core_sparse_checkout_cone; sparse_filename =get_sparse_checkout_filename (); res =add_patterns_from_file_to_list (sparse_filename,"" ,0 , &pl,NULL ,0 );
@@ -334,6 +334,7 @@ static int write_patterns_and_update(struct repository *repo,FILE *fp;struct lock_file lk =LOCK_INIT ;int result; +struct repo_config_values *cfg =repo_config_values (the_repository); sparse_filename =get_sparse_checkout_filename ();
@@ -353,7 +354,7 @@ static int write_patterns_and_update(struct repository *repo,if (!fp)die_errno (_ ("unable to fdopen %s" ),get_lock_file_path (&lk)); -if (core_sparse_checkout_cone) +if (cfg->core_sparse_checkout_cone)write_cone_to_file (fp, pl);else write_patterns_to_file (fp, pl);
@@ -402,15 +403,15 @@ static enum sparse_checkout_mode update_cone_mode(int *cone_mode) {/* If not specified, use previous definition of cone mode */ if (*cone_mode ==-1 && cfg->apply_sparse_checkout) - *cone_mode = core_sparse_checkout_cone; + *cone_mode = cfg->core_sparse_checkout_cone;/* Set cone/non-cone mode appropriately */ cfg->apply_sparse_checkout =1 ;if (*cone_mode ==1 || *cone_mode ==-1 ) { - core_sparse_checkout_cone =1 ; + cfg->core_sparse_checkout_cone =1 ;return MODE_CONE_PATTERNS ; } - core_sparse_checkout_cone =0 ; + cfg->core_sparse_checkout_cone =0 ;return MODE_ALL_PATTERNS ; }
@@ -577,7 +578,9 @@ static void add_patterns_from_input(struct pattern_list *pl, FILE *file) {int i; -if (core_sparse_checkout_cone) { +struct repo_config_values *cfg =repo_config_values (the_repository); + +if (cfg->core_sparse_checkout_cone) {struct strbuf line =STRBUF_INIT ;hashmap_init (&pl->recursive_hashmap, pl_hashmap_cmp,NULL ,0 );
@@ -636,13 +639,14 @@ static void add_patterns_cone_mode(int argc, const char **argv,struct pattern_entry *pe;struct hashmap_iter iter;struct pattern_list existing; +struct repo_config_values *cfg =repo_config_values (the_repository);char *sparse_filename =get_sparse_checkout_filename ();add_patterns_from_input (pl, argc, argv, use_stdin ? stdin :NULL );memset (&existing,0 ,sizeof (existing)); - existing.use_cone_patterns = core_sparse_checkout_cone; + existing.use_cone_patterns = cfg->core_sparse_checkout_cone;if (add_patterns_from_file_to_list (sparse_filename,"" ,0 , &existing,NULL ,0 ))
@@ -690,7 +694,7 @@ static int modify_pattern_list(struct repository *repo,switch (m) {case ADD : -if (core_sparse_checkout_cone) +if (cfg->core_sparse_checkout_cone)add_patterns_cone_mode (args->nr, args->v, pl, use_stdin);else add_patterns_literal (args->nr, args->v, pl, use_stdin);
@@ -723,11 +727,12 @@ static void sanitize_paths(struct repository *repo, const char *prefix, int skip_checks) { int i; +struct repo_config_values *cfg =repo_config_values (the_repository);if (!args->nr)return ; -if (prefix && *prefix && core_sparse_checkout_cone) { +if (prefix && *prefix && cfg->core_sparse_checkout_cone) {/* * The args are not pathspecs, so unfortunately we * cannot imitate how cmd_add() uses parse_pathspec().
@@ -744,10 +749,10 @@ static void sanitize_paths(struct repository *repo,if (skip_checks)return ; -if (prefix && *prefix && !core_sparse_checkout_cone) +if (prefix && *prefix && !cfg->core_sparse_checkout_cone)die (_ ("please run from the toplevel directory in non-cone mode" )); -if (core_sparse_checkout_cone) { +if (cfg->core_sparse_checkout_cone) {for (i =0 ; i < args->nr; i++) {if (args->v[i][0 ] =='/' )die (_ ("specify directories rather than patterns (no leading slash)" ));
@@ -769,7 +774,7 @@ static void sanitize_paths(struct repository *repo,if (S_ISSPARSEDIR (ce->ce_mode))continue ; -if (core_sparse_checkout_cone) +if (cfg->core_sparse_checkout_cone)die (_ ("'%s' is not a directory; to treat it as a directory anyway, rerun with --skip-checks" ), args->v[i]);else warning (_ ("pass a leading slash before paths such as '%s' if you want a single file (see NON-CONE PROBLEMS in the git-sparse-checkout manual)." ), args->v[i]);
@@ -836,6 +841,7 @@ static struct sparse_checkout_set_opts {static int sparse_checkout_set (int argc,const char **argv,const char *prefix,struct repository *repo) { +struct repo_config_values *cfg =repo_config_values (the_repository);int default_patterns_nr =2 ;const char *default_patterns[] = {"/*" ,"!/*/" ,NULL };
@@ -873,7 +879,7 @@ static int sparse_checkout_set(int argc, const char **argv, const char *prefix,* non-cone mode, if nothing is specified, manually select just the * top-level directory (much as 'init' would do). */ -if (!core_sparse_checkout_cone && !set_opts.use_stdin && argc ==0 ) { +if (!cfg->core_sparse_checkout_cone && !set_opts.use_stdin && argc ==0 ) {for (int i =0 ; i < default_patterns_nr; i++)strvec_push (&patterns, default_patterns[i]); }else {
@@ -977,7 +983,7 @@ static int sparse_checkout_clean(int argc, const char **argv,setup_work_tree ();if (!cfg->apply_sparse_checkout)die (_ ("must be in a sparse-checkout to clean directories" )); -if (!core_sparse_checkout_cone) +if (!cfg->core_sparse_checkout_cone)die (_ ("must be in a cone-mode sparse-checkout to clean directories" )); argc = parse_options(argc, argv, prefix,
@@ -1141,6 +1147,7 @@ static int sparse_checkout_check_rules(int argc, const char **argv, const char *FILE *fp;int ret;struct pattern_list pl = {0 }; +struct repo_config_values *cfg =repo_config_values (the_repository);char *sparse_filename; check_rules_opts.cone_mode =-1 ;
@@ -1152,7 +1159,7 @@ static int sparse_checkout_check_rules(int argc, const char **argv, const char * check_rules_opts.cone_mode =1 ;update_cone_mode (&check_rules_opts.cone_mode); - pl.use_cone_patterns = core_sparse_checkout_cone; + pl.use_cone_patterns = cfg->core_sparse_checkout_cone;if (check_rules_opts.rules_file) { fp =xfopen (check_rules_opts.rules_file,"r" );add_patterns_from_input (&pl, argc, argv, fp);
dir.c +2 -1
@@ -3508,8 +3508,9 @@ int get_sparse_checkout_patterns(struct pattern_list *pl) {int res;char *sparse_filename =get_sparse_checkout_filename (); +struct repo_config_values *cfg =repo_config_values (the_repository); - pl->use_cone_patterns = core_sparse_checkout_cone; + pl->use_cone_patterns = cfg->core_sparse_checkout_cone; res =add_patterns_from_file_to_list (sparse_filename,"" ,0 , pl,NULL ,0 );free (sparse_filename);
environment.c +2 -2
@@ -70,7 +70,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; -int core_sparse_checkout_cone;int sparse_expect_files_outside_of_patterns;unsigned long pack_size_limit_cfg;
@@ -526,7 +525,7 @@ int git_default_core_config(const char *var, const char *value, }if (!strcmp (var,"core.sparsecheckoutcone" )) { - core_sparse_checkout_cone =git_config_bool (var, value); + cfg->core_sparse_checkout_cone =git_config_bool (var, value);return 0 ; }
@@ -723,4 +722,5 @@ void repo_config_values_init(struct repo_config_values *cfg) 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.h +1 -1
@@ -96,6 +96,7 @@ struct repo_config_values {int zlib_compression_level;int pack_compression_level;int precomposed_unicode; +int core_sparse_checkout_cone;/* section "branch" config values */ enum branch_track branch_track;
@@ -178,7 +179,6 @@ extern unsigned long pack_size_limit_cfg;extern int protect_hfs;extern int protect_ntfs; -extern int core_sparse_checkout_cone;extern int sparse_expect_files_outside_of_patterns;enum rebase_setup_type {
sparse-index.c +1 -1
@@ -154,7 +154,7 @@ int is_sparse_index_allowed(struct index_state *istate, int flags) {struct repo_config_values *cfg =repo_config_values (the_repository); -if (!cfg->apply_sparse_checkout || !core_sparse_checkout_cone) +if (!cfg->apply_sparse_checkout || !cfg->core_sparse_checkout_cone)return 0 ;if (!(flags &SPARSE_INDEX_MEMORY_ONLY )) {
Signature · 1 line
2.53.0.155.g9f36b15afa
PATCH 7/8 env: put "sparse_expect_files_outside_of_patterns" in `repo_config_values`
In reply to Olamide Caleb BelloThe sparse_expect_files_outside_of_patterns variable was previously a global variable, which makes it shared across repository instances within a single process.
Move it into repo_config_values, this makes the value tied to the repository from which it was read. This preserves existing behavior while avoiding cross-repository state leakage and is another step toward eliminating repository-dependent global state.
Update all references to use repo_config_values().
--- environment.c | 6 ++++-- environment.h | 5 +++-- sparse-index.c | 2 +- 3 files changed, 8 insertions(+), 5 deletions(-)
environment.c +4 -2
@@ -70,7 +70,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; -int sparse_expect_files_outside_of_patterns;unsigned long pack_size_limit_cfg;#ifndef PROTECT_HFS_DEFAULT
@@ -550,8 +549,10 @@ int git_default_core_config(const char *var, const char *value,static int git_default_sparse_config (const char *var,const char *value) { +struct repo_config_values *cfg =repo_config_values (the_repository); +if (!strcmp (var,"sparse.expectfilesoutsideofpatterns" )) { - sparse_expect_files_outside_of_patterns =git_config_bool (var, value); + cfg->sparse_expect_files_outside_of_patterns =git_config_bool (var, value);return 0 ; }
@@ -723,4 +724,5 @@ void repo_config_values_init(struct repo_config_values *cfg) cfg->pack_compression_level =Z_DEFAULT_COMPRESSION ; cfg->precomposed_unicode =-1 ;/* see probe_utf8_pathname_composition() */ cfg->core_sparse_checkout_cone =0 ; + cfg->sparse_expect_files_outside_of_patterns =0 ; }
environment.h +3 -2
@@ -98,6 +98,9 @@ struct repo_config_values {int precomposed_unicode;int core_sparse_checkout_cone; +/* section "sparse" config values */ +int sparse_expect_files_outside_of_patterns; +/* section "branch" config values */ enum branch_track branch_track; };
@@ -179,8 +182,6 @@ extern unsigned long pack_size_limit_cfg;extern int protect_hfs;extern int protect_ntfs; -extern int sparse_expect_files_outside_of_patterns; -enum rebase_setup_type {AUTOREBASE_NEVER =0 ,AUTOREBASE_LOCAL ,
sparse-index.c +1 -1
@@ -675,7 +675,7 @@ void clear_skip_worktree_from_present_files(struct index_state *istate)struct repo_config_values *cfg =repo_config_values (the_repository);if (!cfg->apply_sparse_checkout || - sparse_expect_files_outside_of_patterns) + cfg->sparse_expect_files_outside_of_patterns)return ;if (clear_skip_worktree_from_present_files_sparse (istate)) {
Signature · 1 line
2.53.0.155.g9f36b15afa
PATCH 8/8 env: move "warn_on_object_refname_ambiguity" into `repo_config_values`
In reply to Olamide Caleb BelloThe warn_on_object_refname_ambiguity variable was previously a global integer, which makes it shared across repository instances in a single process.
Move it into repo_config_values so the value is associated with the repository from which it was read. This preserves existing behavior while avoiding cross-repository state leakage and is another step toward eliminating repository-dependent global state.
Update all references to use repo_config_values().
--- builtin/cat-file.c | 7 ++++--- builtin/pack-objects.c | 7 ++++--- environment.c | 2 +- environment.h | 2 +- object-name.c | 3 ++- revision.c | 7 ++++--- submodule.c | 7 ++++--- 7 files changed, 20 insertions(+), 15 deletions(-)
builtin/cat-file.c +4 -3
@@ -897,6 +897,7 @@ static int batch_objects(struct batch_options *opt)struct strbuf input =STRBUF_INIT ;struct strbuf output =STRBUF_INIT ;struct expand_data data =EXPAND_DATA_INIT ; +struct repo_config_values *cfg =repo_config_values (the_repository);int save_warning;int retval =0 ;
@@ -969,8 +970,8 @@ static int batch_objects(struct batch_options *opt)* warn) ends up dwarfing the actual cost of the object lookups * themselves. We can work around it by just turning off the warning. */ - save_warning = warn_on_object_refname_ambiguity; - warn_on_object_refname_ambiguity =0 ; + save_warning = cfg->warn_on_object_refname_ambiguity; + cfg->warn_on_object_refname_ambiguity =0 ;if (opt->batch_mode ==BATCH_MODE_QUEUE_AND_DISPATCH ) {batch_objects_command (opt, &output, &data);
@@ -998,7 +999,7 @@ static int batch_objects(struct batch_options *opt) cleanup:strbuf_release (&input);strbuf_release (&output); - warn_on_object_refname_ambiguity = save_warning; + cfg->warn_on_object_refname_ambiguity = save_warning;return retval; }
builtin/pack-objects.c +4 -3
@@ -4653,6 +4653,7 @@ static void get_object_list(struct rev_info *revs, struct strvec *argv)struct setup_revision_opt s_r_opt = { .allow_exclude_promisor_objects =1 , }; +struct repo_config_values *cfg =repo_config_values (the_repository);char line[1000 ];int flags =0 ;int save_warning;
@@ -4663,8 +4664,8 @@ static void get_object_list(struct rev_info *revs, struct strvec *argv)/* make sure shallows are read */ is_repository_shallow (the_repository); - save_warning = warn_on_object_refname_ambiguity; - warn_on_object_refname_ambiguity =0 ; + save_warning = cfg->warn_on_object_refname_ambiguity; + cfg->warn_on_object_refname_ambiguity =0 ;while (fgets (line,sizeof (line), stdin) !=NULL ) {int len =strlen (line);
@@ -4692,7 +4693,7 @@ static void get_object_list(struct rev_info *revs, struct strvec *argv)die (_ ("bad revision '%s'" ), line); } - warn_on_object_refname_ambiguity = save_warning; + cfg->warn_on_object_refname_ambiguity = save_warning;if (use_bitmap_index && !get_object_list_from_bitmap (revs))return ;
environment.c +1 -1
@@ -47,7 +47,6 @@ int minimum_abbrev = 4, default_abbrev = -1;int ignore_case;int assume_unchanged;int is_bare_repository_cfg =-1 ;/* unspecified */ -int warn_on_object_refname_ambiguity =1 ;char *git_commit_encoding;char *git_log_output_encoding;char *apply_default_whitespace;
@@ -725,4 +724,5 @@ void repo_config_values_init(struct repo_config_values *cfg) cfg->precomposed_unicode =-1 ;/* see probe_utf8_pathname_composition() */ cfg->core_sparse_checkout_cone =0 ; cfg->sparse_expect_files_outside_of_patterns =0 ; + cfg->warn_on_object_refname_ambiguity =1 ; }
environment.h +1 -1
@@ -97,6 +97,7 @@ struct repo_config_values {int pack_compression_level;int precomposed_unicode;int core_sparse_checkout_cone; +int warn_on_object_refname_ambiguity;/* section "sparse" config values */ int sparse_expect_files_outside_of_patterns;
@@ -174,7 +175,6 @@ extern int has_symlinks;extern int minimum_abbrev, default_abbrev;extern int ignore_case;extern int assume_unchanged; -extern int warn_on_object_refname_ambiguity;extern char *apply_default_whitespace;extern char *apply_default_ignorewhitespace;extern unsigned long pack_size_limit_cfg;
object-name.c +2 -1
@@ -969,11 +969,12 @@ static int get_oid_basic(struct repository *r, const char *str, int len,int refs_found =0 ;int at, reflog_len, nth_prior =0 ;int fatal = !(flags &GET_OID_QUIETLY ); +struct repo_config_values *cfg =repo_config_values (the_repository);if (len == r->hash_algo->hexsz && !get_oid_hex (str, oid)) {if (!(flags &GET_OID_SKIP_AMBIGUITY_CHECK ) &&repo_settings_get_warn_ambiguous_refs (r) && - warn_on_object_refname_ambiguity) { + cfg->warn_on_object_refname_ambiguity) { refs_found =repo_dwim_ref (r, str, len, &tmp_oid, &real_ref,0 );if (refs_found >0 ) {warning (warn_msg, len, str);
revision.c +4 -3
@@ -2904,9 +2904,10 @@ static void read_revisions_from_stdin(struct rev_info *revs,int seen_end_of_options =0 ;int save_warning;int flags =0 ; +struct repo_config_values *cfg =repo_config_values (the_repository); - save_warning = warn_on_object_refname_ambiguity; - warn_on_object_refname_ambiguity =0 ; + save_warning = cfg->warn_on_object_refname_ambiguity; + cfg->warn_on_object_refname_ambiguity =0 ;strbuf_init (&sb,1000 );while (strbuf_getline (&sb, stdin) !=EOF ) {
@@ -2940,7 +2941,7 @@ static void read_revisions_from_stdin(struct rev_info *revs,read_pathspec_from_stdin (&sb, prune);strbuf_release (&sb); - warn_on_object_refname_ambiguity = save_warning; + cfg->warn_on_object_refname_ambiguity = save_warning; } static void NORETURN diagnose_missing_default(const char *def)
submodule.c +4 -3
@@ -898,12 +898,13 @@ static void collect_changed_submodules(struct repository *r,struct setup_revision_opt s_r_opt = { .assume_dashdash =1 , }; +struct repo_config_values *cfg =repo_config_values (the_repository); - save_warning = warn_on_object_refname_ambiguity; - warn_on_object_refname_ambiguity =0 ; + save_warning = cfg->warn_on_object_refname_ambiguity; + cfg->warn_on_object_refname_ambiguity =0 ;repo_init_revisions (r, &rev,NULL );setup_revisions_from_strvec (argv, &rev, &s_r_opt); - warn_on_object_refname_ambiguity = save_warning; + cfg->warn_on_object_refname_ambiguity = save_warning;if (prepare_revision_walk (&rev))die (_ ("revision walk setup failed" ));
Signature · 1 line
2.53.0.155.g9f36b15afa
Quoted 10 lines · Olamide Caleb Bello
Base series
===========
This series builds on top of the merged series:
[PATCH v7 0/3] Move repo-specific globals intorepo_config_valuesavailable at:
https://lore.kernel.org/git/<[email protected]>It should be applied on top of that series.
The above was interesting when the "Move repo-specific globals into repo_config_values" series wasn't merged to master. But now that it has been merged, you can remove it.
Thanks.
Quoted 11 lines · Christian Couder
Quoted 9 lines · Olamide Caleb Bello
Base series
===========
This series builds on top of the merged series:
[PATCH v7 0/3] Move repo-specific globals intorepo_config_valuesavailable at:
https://lore.kernel.org/git/<[email protected]>
Quoted 7 lines
Quoted 1 line
It should be applied on top of that series.
The above was interesting when the "Move repo-specific globals into
repo_config_values" series wasn't merged to master. But now that it has been merged, you can remove it.Thanks.
Thank you, Christian.
I will remove it in subsequent versions
Hi Olamide,
Review
Quoted 10 lines · Olamide Caleb Bello
int status = Z_OK; int write_object = (flags & INDEX_WRITE_OBJECT); off_t offset = 0;(quoted diff) +2
-1+ struct repo_config_values *cfg = repo_config_values(the_repository); - git_deflate_init(&s, pack_compression_level); + git_deflate_init(&s, cfg->pack_compression_level); hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB, size); s.next_out = obuf + hdrlen;
I didn't look closely at the other parts, but I have a small question about this section.
pack_compression_level before this patch is a global variable:
int pack_compression_level = Z_DEFAULT_COMPRESSION;
and struct option in cmd_pack_objects contains its pointer:
struct option pack_objects_options[] = {
...
OPT_INTEGER(0, "compression", &pack_compression_level, ...),
...
};The reason why functions such as do_compress, write_large_blob_data can work properly is beacuse they all read the same global variable, right?
However, in this patch,
Review
Quoted 2 lines
(quoted diff) +2
-0+ struct repo_config_values *cfg = repo_config_values(the_repository); + int pack_compression_level = cfg->pack_compression_level;
Here, a local variable with the same name was created via value assignment (I also find the naming a bit odd).
Review static unsigned long do_compress(void **pptr, unsigned long size) · line 389
Quoted 9 lines
(quoted diff) +2
-1@@ -383,8 +383,9 @@ static unsigned long do_compress(void **pptr, unsigned long size) git_zstream stream; void *in, *out; unsigned long maxsize; + struct repo_config_values *cfg = repo_config_values(the_repository); - git_deflate_init(&stream, pack_compression_level); + git_deflate_init(&stream, cfg->pack_compression_level); maxsize = git_deflate_bound(&stream, size);
But then in the do_compress() function, the variable being read is still that pointer, cfg->pack_compression_level. The expected input wasn't *written back* to this pointer, right? If I understand correctly, after parsing CLI, the output is written to the local variable rather than the cfg. And that's why the naming is a bit confusing to me.
struct option pack_objects_options[] = {
...
OPT_INTEGER(0, "compression", &cfg->pack_compression_level, ...),
...
};I think change like this is needed. Of course, you'll need to double-check it. _(:3 」∠ )_
Regards,
Yuchen
Hi Yuchen,
Thanks for taking a close look.
My intention here was mainly to avoid repeating cfg->pack_compression_level multiple times in the function, so I introduced a local pack_compression_level initialized from cfg->pack_compression_level.
But you are right to point out the interaction with the CLI option. The --compression option currently writes to the local variable via OPT_INTEGER, and the value is not propagated back to cfg->pack_compression_level.
I took a second look at it and will change the option to write directly into cfg->pack_compression_level instead in upcoming versions.
Thanks for pointing this out.
Best,
Olamide
Quoted 70 lines · Tian Yuchen
Hi Olamide,
Review
Quoted 10 lines · Olamide Caleb Bello
int status = Z_OK; int write_object = (flags & INDEX_WRITE_OBJECT); off_t offset = 0;(quoted diff) +2
-1+ struct repo_config_values *cfg = repo_config_values(the_repository); - git_deflate_init(&s, pack_compression_level); + git_deflate_init(&s, cfg->pack_compression_level); hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB, size); s.next_out = obuf + hdrlen;I didn't look closely at the other parts, but I have a small question about this section.
pack_compression_levelbefore this patch is a global variable:int pack_compression_level = Z_DEFAULT_COMPRESSION;and struct option in
cmd_pack_objectscontains its pointer:struct option pack_objects_options[] = { ... OPT_INTEGER(0, "compression", &pack_compression_level, ...), ... };The reason why functions such as
do_compress,write_large_blob_datacan work properly is beacuse they all read the same global variable, right?However, in this patch,
Review
Quoted 2 lines
(quoted diff) +2
-0+ struct repo_config_values *cfg = repo_config_values(the_repository); + int pack_compression_level = cfg->pack_compression_level;Here, a local variable with the same name was created via value assignment (I also find the naming a bit odd).
Review static unsigned long do_compress(void **pptr, unsigned long size) · line 389
Quoted 9 lines
(quoted diff) +2
-1@@ -383,8 +383,9 @@ static unsigned long do_compress(void **pptr, unsigned long size) git_zstream stream; void *in, *out; unsigned long maxsize; + struct repo_config_values *cfg = repo_config_values(the_repository); - git_deflate_init(&stream, pack_compression_level); + git_deflate_init(&stream, cfg->pack_compression_level); maxsize = git_deflate_bound(&stream, size);But then in the
do_compress()function, the variable being read is still that pointer,cfg->pack_compression_level. The expected input wasn't *written back* to this pointer, right? If I understand correctly, after parsing CLI, the output is written to the local variable rather than the cfg. And that's why the naming is a bit confusing to me.struct option pack_objects_options[] = { ... OPT_INTEGER(0, "compression", &cfg->pack_compression_level, ...), ... };I think change like this is needed. Of course, you'll need to double-check it.
_(:3 」∠ )_Regards,
Yuchen
Quoted 64 lines · Tian Yuchen
Hi Olamide,
Review
Quoted 10 lines · Olamide Caleb Bello
int status = Z_OK; int write_object = (flags & INDEX_WRITE_OBJECT); off_t offset = 0;(quoted diff) +2
-1+ struct repo_config_values *cfg = repo_config_values(the_repository); - git_deflate_init(&s, pack_compression_level); + git_deflate_init(&s, cfg->pack_compression_level); hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), OBJ_BLOB, size); s.next_out = obuf + hdrlen;I didn't look closely at the other parts, but I have a small question about this section.
pack_compression_levelbefore this patch is a global variable:int pack_compression_level = Z_DEFAULT_COMPRESSION;and struct option in
cmd_pack_objectscontains its pointer:struct option pack_objects_options[] = { ... OPT_INTEGER(0, "compression", &pack_compression_level, ...), ... };The reason why functions such as
do_compress,write_large_blob_datacan work properly is beacuse they all read the same global variable, right?However, in this patch,
Review
Quoted 2 lines
(quoted diff) +2
-0+ struct repo_config_values *cfg = repo_config_values(the_repository); + int pack_compression_level = cfg->pack_compression_level;Here, a local variable with the same name was created via value assignment (I also find the naming a bit odd).
Review static unsigned long do_compress(void **pptr, unsigned long size) · line 389
Quoted 9 lines
(quoted diff) +2
-1@@ -383,8 +383,9 @@ static unsigned long do_compress(void **pptr, unsigned long size) git_zstream stream; void *in, *out; unsigned long maxsize; + struct repo_config_values *cfg = repo_config_values(the_repository); - git_deflate_init(&stream, pack_compression_level); + git_deflate_init(&stream, cfg->pack_compression_level); maxsize = git_deflate_bound(&stream, size);But then in the
do_compress()function, the variable being read is still that pointer,cfg->pack_compression_level. The expected input wasn't *written back* to this pointer, right? If I understand correctly, after parsing CLI, the output is written to the local variable rather than the cfg. And that's why the naming is a bit confusing to me.struct option pack_objects_options[] = { ... OPT_INTEGER(0, "compression", &cfg->pack_compression_level, ...), ... };
Quoted 6 lines
I think change like this is needed. Of course, you'll need to double-check it.
_(:3 」∠ )_Regards,
Yuchen
Hi Yuchen,
Thanks for taking a close look.
My intention here was mainly to avoid repeating cfg->pack_compression_level multiple times in the function, so I introduced a local pack_compression_level initialized from cfg->pack_compression_level.
But you are right to point out the interaction with the CLI option. The --compression option currently writes to the local variable via OPT_INTEGER, and the value is not propagated back to cfg->pack_compression_level.
I took a second look at it and will change the option to write directly into cfg->pack_compression_level instead in upcoming versions.
Thanks for pointing this out.
Best regards,
Olamide Caleb Bello