Remove is_bare_repository_cfg global state
cover Remove is_bare_repository_cfg global state
This patch series removes the global state introduced by the is_bare_repository_cfg variable by moving it into the repository struct. Most of the refactor is done by patch 1. Patch 2 initializes the member in places that left it unInitialized, while patch 3 adds a safety measure by BUG()ing when the variable has not been properly initialized.
John Cai (3):
git: remove is_bare_repository_cfg global variable
setup: initialize is_bare_cfg
repository: BUG when is_bare_cfg is not initialized
attr.c | 4 ++-- builtin/bisect.c | 2 +- builtin/blame.c | 2 +- builtin/check-attr.c | 2 +- builtin/clone.c | 4 ++-- builtin/gc.c | 2 +- builtin/init-db.c | 14 +++++++------- builtin/repack.c | 2 +- builtin/reset.c | 2 +- builtin/rev-parse.c | 2 +- builtin/submodule--helper.c | 2 +- config.c | 2 +- dir.c | 2 +- environment.c | 7 ------- environment.h | 3 +-- git.c | 2 +- mailmap.c | 4 ++-- refs/files-backend.c | 2 +- refs/reftable-backend.c | 2 +- repository.c | 23 +++++++++++++++++++---- repository.h | 12 +++++++++++- scalar.c | 2 +- setup.c | 19 +++++++++++++------ submodule.c | 2 +- t/helper/test-partial-clone.c | 2 +- t/helper/test-repository.c | 4 ++-- transport.c | 4 ++-- worktree.c | 4 ++-- 28 files changed, 79 insertions(+), 55 deletions(-)
base-commit: 8f8d6eee531b3fa1a8ef14f169b0cb5035f7a772
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1826%2Fjohn-cai%2Fjc%2Fremove_is_bare_global-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1826/john-cai/jc/remove_is_bare_global-v1
Pull-Request: https://github.com/git/git/pull/1826
Signature · 1 line
gitgitgadget
PATCH 1/3 git: remove is_bare_repository_cfg global variable
From: John Cai <[email protected]>
The is_bare_repository_cfg global variable is used for storing a bare repository setting, either through the config, an env var, or the commandline. This variable is global, and hence introduces global state everywhere it is used.
In order to reduce global state, add a member to the repository struct to keep track of the setting there. For now, the_repository is what's used to set the member, which still represents global state. However, there is a parallel effort to replace calls to the_repository with a repository struct that is passed into builtins, see [1]. Hence, this change will help the overall effort in reducing global state.
1. 9b1cb5070f (builtin: add a repository parameter for builtin functions, Fri Sep 13 21:16:14 2024 +0000)
--- attr.c | 4 ++-- builtin/bisect.c | 2 +- builtin/blame.c | 2 +- builtin/check-attr.c | 2 +- builtin/clone.c | 4 ++-- builtin/gc.c | 2 +- builtin/init-db.c | 14 +++++++------- builtin/repack.c | 2 +- builtin/reset.c | 2 +- builtin/rev-parse.c | 2 +- builtin/submodule--helper.c | 2 +- config.c | 2 +- dir.c | 2 +- environment.c | 7 ------- environment.h | 3 +-- git.c | 2 +- mailmap.c | 4 ++-- refs/files-backend.c | 2 +- refs/reftable-backend.c | 2 +- repository.c | 21 +++++++++++++++++---- repository.h | 12 +++++++++++- scalar.c | 2 +- setup.c | 12 ++++++------ submodule.c | 2 +- t/helper/test-partial-clone.c | 2 +- t/helper/test-repository.c | 4 ++-- transport.c | 4 ++-- worktree.c | 4 ++-- 28 files changed, 70 insertions(+), 55 deletions(-)
attr.c +2 -2
@@ -716,7 +716,7 @@ static enum git_attr_direction direction;void git_attr_set_direction (enum git_attr_direction new_direction) { -if (is_bare_repository () && new_direction !=GIT_ATTR_INDEX ) +if (repo_is_bare (the_repository) && new_direction !=GIT_ATTR_INDEX )BUG ("non-INDEX attr direction in a bare repo" );if (new_direction != direction)
@@ -883,7 +883,7 @@ static struct attr_stack *read_attr(struct index_state *istate, res =read_attr_from_index (istate, path, flags); }else if (tree_oid) { res =read_attr_from_blob (istate, tree_oid, path, flags); - }else if (!is_bare_repository ()) { + }else if (!repo_is_bare (the_repository)) {if (direction ==GIT_ATTR_CHECKOUT ) { res =read_attr_from_index (istate, path, flags);if (!res)
builtin/bisect.c +1 -1
@@ -705,7 +705,7 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,struct object_id oid;const char *head; -if (is_bare_repository ()) +if (repo_is_bare (the_repository)) no_checkout =1 ;/*
builtin/blame.c +1 -1
@@ -1092,7 +1092,7 @@ parse_done: revs.disable_stdin =1 ;setup_revisions (argc, argv, &revs,NULL ); -if (!revs.pending.nr &&is_bare_repository ()) { +if (!revs.pending.nr &&repo_is_bare (the_repository)) {struct commit *head_commit;struct object_id head_oid;
builtin/check-attr.c +1 -1
@@ -116,7 +116,7 @@ int cmd_check_attr(int argc,struct object_id initialized_oid;int cnt, i, doubledash, filei; -if (!is_bare_repository ()) +if (!repo_is_bare (the_repository))setup_work_tree ();git_config (git_default_config,NULL );
builtin/clone.c +2 -2
@@ -1415,7 +1415,7 @@ int cmd_clone(int argc,repo_clear (the_repository);/* At this point, we need the_repository to match the cloned repo. */ -if (repo_init (the_repository, git_dir, work_tree)) +if (repo_init (the_repository, git_dir, work_tree,-1 ))warning (_ ("failed to initialize the repo, skipping bundle URI" ));else if (fetch_bundle_uri (the_repository, bundle_uri, &has_heuristic)) warning(_("failed to fetch objects from bundle URI '%s'"),
@@ -1446,7 +1446,7 @@ int cmd_clone(int argc,repo_clear (the_repository);/* At this point, we need the_repository to match the cloned repo. */ -if (repo_init (the_repository, git_dir, work_tree)) +if (repo_init (the_repository, git_dir, work_tree,-1 ))warning (_ ("failed to initialize the repo, skipping bundle URI" ));else if (fetch_bundle_list (the_repository, transport->bundles))
builtin/gc.c +1 -1
@@ -712,7 +712,7 @@ struct repository *repo UNUSED)die (_ ("failed to parse gc.logExpiry value %s" ), cfg.gc_log_expire);if (cfg.pack_refs <0 ) - cfg.pack_refs = !is_bare_repository (); + cfg.pack_refs = !repo_is_bare (the_repository); argc =parse_options (argc, argv, prefix, builtin_gc_options, builtin_gc_usage,0 );
builtin/init-db.c +7 -7
@@ -89,7 +89,7 @@ int cmd_init_db(int argc,const struct option init_db_options[] = {OPT_STRING (0 ,"template" , &template_dir,N_ ("template-directory" ),N_ ("directory from which templates will be used" )), -OPT_SET_INT (0 ,"bare" , &is_bare_repository_cfg, +OPT_SET_INT (0 ,"bare" , &the_repository->is_bare_cfg,N_ ("create a bare repository" ),1 ), {OPTION_CALLBACK ,0 ,"shared" , &init_shared_repository,N_ ("permissions" ),
@@ -109,7 +109,7 @@ int cmd_init_db(int argc, argc =parse_options (argc, argv, prefix, init_db_options, init_db_usage,0 ); -if (real_git_dir && is_bare_repository_cfg ==1 ) +if (real_git_dir && the_repository->is_bare_cfg ==1 )die (_ ("options '%s' and '%s' cannot be used together" ),"--separate-git-dir" ,"--bare" );if (real_git_dir && !is_absolute_path (real_git_dir))
@@ -155,7 +155,7 @@ int cmd_init_db(int argc, }else if (0 < argc) {usage (init_db_usage[0 ]); } -if (is_bare_repository_cfg ==1 ) { +if (the_repository->is_bare_cfg ==1 ) {char *cwd =xgetcwd ();setenv (GIT_DIR_ENVIRONMENT , cwd, argc >0 );free (cwd);
@@ -182,7 +182,7 @@ int cmd_init_db(int argc,*/ git_dir =xstrdup_or_null (getenv (GIT_DIR_ENVIRONMENT )); work_tree =xstrdup_or_null (getenv (GIT_WORK_TREE_ENVIRONMENT )); -if ((!git_dir || is_bare_repository_cfg ==1 ) && work_tree) +if ((!git_dir || the_repository->is_bare_cfg ==1 ) && work_tree) die(_("%s (or --work-tree=<directory>) not allowed without " "specifying %s (or --git-dir=<directory>)"), GIT_WORK_TREE_ENVIRONMENT,
@@ -218,10 +218,10 @@ int cmd_init_db(int argc,strbuf_release (&sb); } -if (is_bare_repository_cfg <0 ) - is_bare_repository_cfg =guess_repository_type (git_dir); +if (the_repository->is_bare_cfg <0 ) + the_repository->is_bare_cfg =guess_repository_type (git_dir); -if (!is_bare_repository_cfg) { +if (!the_repository->is_bare_cfg) {const char *git_dir_parent =strrchr (git_dir,'/' );if (git_dir_parent) {char *rel =xstrndup (git_dir, git_dir_parent - git_dir);
builtin/repack.c +1 -1
@@ -1266,7 +1266,7 @@ int cmd_repack(int argc,if (write_bitmaps <0 ) {if (!write_midx && - (!(pack_everything &ALL_INTO_ONE ) || !is_bare_repository ())) + (!(pack_everything &ALL_INTO_ONE ) || !repo_is_bare (the_repository))) write_bitmaps =0 ; }if (pack_kept_objects <0 )
builtin/reset.c +1 -1
@@ -448,7 +448,7 @@ int cmd_reset(int argc,if (reset_type !=SOFT && (reset_type !=MIXED ||repo_get_work_tree (the_repository)))setup_work_tree (); -if (reset_type ==MIXED &&is_bare_repository ()) +if (reset_type ==MIXED &&repo_is_bare (the_repository))die (_ ("%s reset is not allowed in a bare repository" ),_ (reset_type_names[reset_type]));
builtin/rev-parse.c +1 -1
@@ -1063,7 +1063,7 @@ int cmd_rev_parse(int argc,continue ; }if (!strcmp (arg,"--is-bare-repository" )) { -printf ("%s\n" ,is_bare_repository () ?"true" +printf ("%s\n" ,repo_is_bare (the_repository) ?"true" :"false" );continue ; }
builtin/submodule--helper.c +1 -1
@@ -1591,7 +1591,7 @@ static int add_possible_reference_from_superproject(struct strbuf err =STRBUF_INIT ;strbuf_add (&sb, odb->path, len); -if (repo_init (&alternate, sb.buf,NULL ) <0 ) +if (repo_init (&alternate, sb.buf,NULL , the_repository->is_bare_cfg) <0 )die (_ ("could not get a repository handle for gitdir '%s'" ), sb.buf);
config.c +1 -1
@@ -1441,7 +1441,7 @@ static int git_default_core_config(const char *var, const char *value, }if (!strcmp (var,"core.bare" )) { - is_bare_repository_cfg =git_config_bool (var, value); + the_repository->is_bare_cfg =git_config_bool (var, value);return 0 ; }
dir.c +1 -1
@@ -4008,7 +4008,7 @@ static void connect_wt_gitdir_in_nested(const char *sub_worktree,const struct submodule *sub;/* If the submodule has no working tree, we can ignore it. */ -if (repo_init (&subrepo, sub_gitdir, sub_worktree)) +if (repo_init (&subrepo, sub_gitdir, sub_worktree, the_repository->is_bare_cfg))return ;if (repo_read_index (&subrepo) <0 )
environment.c +0 -7
@@ -34,7 +34,6 @@ int has_symlinks = 1;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 ;int repository_format_precious_objects;char *git_commit_encoding;
@@ -146,12 +145,6 @@ const char *getenv_safe(struct strvec *argv, const char *name)return argv->v[argv->nr -1 ]; } -int is_bare_repository (void ) -{ -/* if core.bare is not 'false', let's see if there is a work tree */ -return is_bare_repository_cfg && !repo_get_work_tree (the_repository); -} -int have_git_dir (void ) {return startup_info->have_repository
environment.h +1 -2
@@ -144,8 +144,7 @@ void set_shared_repository(int value);int get_shared_repository (void );void reset_shared_repository (void ); -extern int is_bare_repository_cfg; -int is_bare_repository (void ); +int is_bare_repository (struct repository *repo);extern char *git_work_tree_cfg;/* Environment bits from configuration mechanism */
git.c +1 -1
@@ -251,7 +251,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) *envchanged =1 ; }else if (!strcmp (cmd,"--bare" )) {char *cwd =xgetcwd (); - is_bare_repository_cfg =1 ; + the_repository->is_bare_cfg =1 ;setenv (GIT_DIR_ENVIRONMENT , cwd,0 );free (cwd);setenv (GIT_IMPLICIT_WORK_TREE_ENVIRONMENT ,"0" ,1 );
mailmap.c +2 -2
@@ -216,10 +216,10 @@ int read_mailmap(struct string_list *map) map->strdup_strings =1 ; map->cmp = namemap_cmp; -if (!git_mailmap_blob &&is_bare_repository ()) +if (!git_mailmap_blob &&repo_is_bare (the_repository)) git_mailmap_blob =xstrdup ("HEAD:.mailmap" ); -if (!startup_info->have_repository || !is_bare_repository ()) +if (!startup_info->have_repository || !repo_is_bare (the_repository)) err |=read_mailmap_file (map,".mailmap" , startup_info->have_repository ?MAILMAP_NOFOLLOW :0 );
refs/files-backend.c +1 -1
@@ -1779,7 +1779,7 @@ static int log_ref_setup(struct files_ref_store *refs,char *logfile;if (log_refs_cfg ==LOG_REFS_UNSET ) - log_refs_cfg =is_bare_repository () ?LOG_REFS_NONE :LOG_REFS_NORMAL ; + log_refs_cfg =repo_is_bare (refs->base.repo) ?LOG_REFS_NONE :LOG_REFS_NORMAL ;files_reflog_path (refs, &logfile_sb, refname); logfile =strbuf_detach (&logfile_sb,NULL );
refs/reftable-backend.c +1 -1
@@ -163,7 +163,7 @@ static int should_write_log(struct reftable_ref_store *refs, const char *refname {enum log_refs_config log_refs_cfg = refs->log_all_ref_updates;if (log_refs_cfg ==LOG_REFS_UNSET ) - log_refs_cfg =is_bare_repository () ?LOG_REFS_NONE :LOG_REFS_NORMAL ; + log_refs_cfg =repo_is_bare (refs->base.repo) ?LOG_REFS_NONE :LOG_REFS_NORMAL ;switch (log_refs_cfg) {case LOG_REFS_NONE :
repository.c +17 -4
@@ -25,7 +25,9 @@extern struct repository *the_repository;/* The main repository */ -static struct repository the_repo; +static struct repository the_repo = { + .is_bare_cfg =-1 , +};struct repository *the_repository = &the_repo;/*
@@ -263,10 +265,13 @@ static int read_and_verify_repository_format(struct repository_format *format,/* * Initialize 'repo' based on the provided 'gitdir'. * Return 0 upon success and a non-zero value upon failure. +* is_bare can be passed to indicate whether or not the repository should be +* treated as bare when repo_init() is used to initiate a secondary repository. */ int repo_init (struct repository *repo,const char *gitdir, -const char *worktree) +const char *worktree, +int is_bare) {struct repository_format format =REPOSITORY_FORMAT_INIT ;memset (repo,0 ,sizeof (*repo));
@@ -283,6 +288,8 @@ int repo_init(struct repository *repo,repo_set_compat_hash_algo (repo, format.compat_hash_algo);repo_set_ref_storage_format (repo, format.ref_storage_format); repo->repository_format_worktree_config = format.worktree_config; +if (is_bare >0 ) + repo->is_bare_cfg = is_bare;/* take ownership of format.partial_clone */ repo->repository_format_partial_clone = format.partial_clone;
@@ -314,7 +321,7 @@ int repo_submodule_init(struct repository *subrepo,strbuf_repo_worktree_path (&gitdir, superproject,"%s/.git" , path);strbuf_repo_worktree_path (&worktree, superproject,"%s" , path); -if (repo_init (subrepo, gitdir.buf, worktree.buf)) { +if (repo_init (subrepo, gitdir.buf, worktree.buf, superproject->is_bare_cfg)) {/* * If initialization fails then it may be due to the submodule * not being populated in the superproject's worktree. Instead
@@ -332,7 +339,7 @@ int repo_submodule_init(struct repository *subrepo,strbuf_reset (&gitdir);submodule_name_to_gitdir (&gitdir, superproject, sub->name); -if (repo_init (subrepo, gitdir.buf,NULL )) { +if (repo_init (subrepo, gitdir.buf,NULL , superproject->is_bare_cfg)) { ret =-1 ; goto out; }
@@ -453,3 +460,9 @@ int repo_hold_locked_index(struct repository *repo,BUG ("the repo hasn't been setup" );return hold_lock_file_for_update (lf, repo->index_file, flags); } + +int repo_is_bare (struct repository *repo) +{ +/* if core.bare is not 'false', let's see if there is a work tree */ +return repo->is_bare_cfg && !repo_get_work_tree (repo); +}
repository.h +11 -1
@@ -153,6 +153,14 @@ struct repository {/* Indicate if a repository has a different 'commondir' from 'gitdir' */ unsigned different_commondir:1; + +/* +* Indicates if the repository is set to be treated as a bare repository, +* through a command line argument, configuration, or environment +* variable. +* -1 means unspecified, 0 indicates non-bare, and 1 indicates bare. +*/ +int is_bare_cfg; };#ifdef USE_THE_REPOSITORY_VARIABLE
@@ -188,7 +196,7 @@ void repo_set_ref_storage_format(struct repository *repo, enum ref_storage_format format);void initialize_repository (struct repository *repo);RESULT_MUST_BE_USED -int repo_init(struct repository *r, const char *gitdir, const char *worktree); +int repo_init(struct repository *r, const char *gitdir, const char *worktree, int is_bare);/* * Initialize the repository 'subrepo' as the submodule at the given path. If
@@ -232,4 +240,6 @@ void repo_update_index_if_able(struct repository *, struct lock_file *);*/ int upgrade_repository_format (int target_version); +int repo_is_bare (struct repository *repo); +#endif /* REPOSITORY_H */
scalar.c +1 -1
@@ -722,7 +722,7 @@ static int cmd_reconfigure(int argc, const char **argv)git_config_clear (); -if (repo_init (&r, gitdir.buf, commondir.buf)) +if (repo_init (&r, gitdir.buf, commondir.buf, the_repository->is_bare_cfg)) goto loop_end; old_repo = the_repository;
setup.c +6 -6
@@ -766,8 +766,8 @@ static int check_repository_format_gently(const char *gitdir, struct repository_if (!has_common) {if (candidate->is_bare !=-1 ) { - is_bare_repository_cfg = candidate->is_bare; -if (is_bare_repository_cfg ==1 ) + the_repository->is_bare_cfg = candidate->is_bare; +if (the_repository->is_bare_cfg ==1 ) inside_work_tree =-1 ; }if (candidate->work_tree) {
@@ -1030,7 +1030,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,/* #3, #7, #11, #15, #19, #23, #27, #31 (see t1510) */ if (work_tree_env)set_git_work_tree (work_tree_env); -else if (is_bare_repository_cfg >0 ) { +else if (the_repository->is_bare_cfg >0 ) {if (git_work_tree_cfg) {/* #22.2, #30 */ warning ("core.bare and core.worktree do not make sense" );
@@ -1116,7 +1116,7 @@ static const char *setup_discovered_git_dir(const char *gitdir, }/* #16.2, #17.2, #20.2, #21.2, #24, #25, #28, #29 (see t1510) */ -if (is_bare_repository_cfg >0 ) { +if (the_repository->is_bare_cfg >0 ) {set_git_dir (gitdir, (offset != cwd->len));if (chdir (cwd->buf))die_errno (_ ("cannot come back to cwd" ));
@@ -2323,7 +2323,7 @@ static int create_default_files(const char *template_path,if (init_shared_repository !=-1 )set_shared_repository (init_shared_repository); - is_bare_repository_cfg = !work_tree; + the_repository->is_bare_cfg = !work_tree;/* * We would have created the above under user's umask -- under
@@ -2349,7 +2349,7 @@ static int create_default_files(const char *template_path, }git_config_set ("core.filemode" , filemode ?"true" :"false" ); -if (is_bare_repository ()) +if (repo_is_bare (the_repository))git_config_set ("core.bare" ,"true" );else {git_config_set ("core.bare" ,"false" );
submodule.c +1 -1
@@ -535,7 +535,7 @@ static struct repository *open_submodule(const char *path)struct strbuf sb =STRBUF_INIT ;struct repository *out =xmalloc (sizeof (*out)); -if (submodule_to_gitdir (&sb, path) ||repo_init (out, sb.buf,NULL )) { +if (submodule_to_gitdir (&sb, path) ||repo_init (out, sb.buf,NULL ,-1 )) {strbuf_release (&sb);free (out);return NULL ;
t/helper/test-partial-clone.c +1 -1
@@ -19,7 +19,7 @@ static void object_info(const char *gitdir, const char *oid_hex)struct object_info oi = {.sizep = &size};const char *p; -if (repo_init (&r, gitdir,NULL )) +if (repo_init (&r, gitdir,NULL ,-1 ))die ("could not init repo" );if (parse_oid_hex_algop (oid_hex, &oid, &p, r.hash_algo))die ("could not parse oid" );
t/helper/test-repository.c +2 -2
@@ -21,7 +21,7 @@ static void test_parse_commit_in_graph(const char *gitdir, const char *worktree,repo_clear (the_repository); -if (repo_init (&r, gitdir, worktree)) +if (repo_init (&r, gitdir, worktree,-1 ))die ("Couldn't init repo" );repo_set_hash_algo (the_repository,hash_algo_by_ptr (r.hash_algo));
@@ -51,7 +51,7 @@ static void test_get_commit_tree_in_graph(const char *gitdir,repo_clear (the_repository); -if (repo_init (&r, gitdir, worktree)) +if (repo_init (&r, gitdir, worktree,-1 ))die ("Couldn't init repo" );repo_set_hash_algo (the_repository,hash_algo_by_ptr (r.hash_algo));
transport.c +2 -2
@@ -1428,7 +1428,7 @@ int transport_push(struct repository *r,if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |TRANSPORT_RECURSE_SUBMODULES_ONLY )) && - !is_bare_repository ()) { + !repo_is_bare (r)) {struct ref *ref = remote_refs;struct oid_array commits =OID_ARRAY_INIT ;
@@ -1455,7 +1455,7 @@ int transport_push(struct repository *r,if (((flags &TRANSPORT_RECURSE_SUBMODULES_CHECK ) || ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |TRANSPORT_RECURSE_SUBMODULES_ONLY )) && - !pretend)) && !is_bare_repository ()) { + !pretend)) && !repo_is_bare (r)) {struct ref *ref = remote_refs;struct string_list needs_pushing =STRING_LIST_INIT_DUP ;struct oid_array commits =OID_ARRAY_INIT ;
worktree.c +2 -2
@@ -85,8 +85,8 @@ static struct worktree *get_main_worktree(int skip_reading_head)* This means that worktree->is_bare may be set to 0 even if the main * worktree is configured to be bare. */ - worktree->is_bare = (is_bare_repository_cfg ==1 ) || -is_bare_repository (); + + worktree->is_bare = the_repository->is_bare_cfg ==1 ; worktree->is_current =is_current_worktree (worktree);if (!skip_reading_head)add_head_info (worktree);
Signature · 1 line
gitgitgadget
PATCH 2/3 setup: initialize is_bare_cfg
In reply to John Cai via GitGitGadgetFrom: John Cai <[email protected]>
A subsequent commit will BUG() when the is_bare_cfg member is uninitialized. Since there are still some codepaths that initializing the is_bare_cfg variable, initialize them.
--- setup.c | 7 +++++++ 1 file changed, 7 insertions(+)
setup.c +7 -0
@@ -741,6 +741,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_if (verify_repository_format (candidate, &err) <0 ) {if (nongit_ok) { + the_repository->is_bare_cfg =1 ;warning ("%s" , err.buf);strbuf_release (&err); *nongit_ok =-1 ;
@@ -1017,6 +1018,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,if (nongit_ok) { *nongit_ok =1 ;free (gitfile); + the_repository->is_bare_cfg =0 ;return NULL ; }die (_ ("not a git repository: '%s'" ), gitdirenv);
@@ -1069,6 +1071,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,/* set_git_work_tree() must have been called by now */ worktree =repo_get_work_tree (the_repository); + the_repository->is_bare_cfg =0 ;/* both repo_get_work_tree() and cwd are already normalized */ if (!strcmp (cwd->buf, worktree)) {/* cwd == worktree */
@@ -1125,6 +1128,9 @@ static const char *setup_discovered_git_dir(const char *gitdir,/* #0, #1, #5, #8, #9, #12, #13 */ set_git_work_tree ("." ); + +if (the_repository->is_bare_cfg <0 ) + the_repository->is_bare_cfg =0 ;if (strcmp (gitdir,DEFAULT_GIT_DIR_ENVIRONMENT ))set_git_dir (gitdir,0 ); inside_git_dir =0 ;
@@ -1767,6 +1773,7 @@ const char *setup_git_directory_gently(int *nongit_ok)die (_ ("not a git repository (or any of the parent directories): %s" ),DEFAULT_GIT_DIR_ENVIRONMENT ); *nongit_ok =1 ; + the_repository->is_bare_cfg =1 ;break ;case GIT_DIR_HIT_MOUNT_POINT :if (!nongit_ok)
Signature · 1 line
gitgitgadget
PATCH 3/3 repository: BUG when is_bare_cfg is not initialized
In reply to John Cai via GitGitGadgetFrom: John Cai <[email protected]>
The is_bare_cfg member of the repository struct should be properly initiated when setting up a repository. BUG when repo_is_bare() sees that the flag has not been set.
--- repository.c | 2 ++ 1 file changed, 2 insertions(+)
repository.c +2 -0
@@ -464,5 +464,7 @@ int repo_hold_locked_index(struct repository *repo,int repo_is_bare (struct repository *repo) {/* if core.bare is not 'false', let's see if there is a work tree */ +if (repo->is_bare_cfg <0 ) +BUG ("is_bare_cfg unspecified" );return repo->is_bare_cfg && !repo_get_work_tree (repo); }
Signature · 1 line
gitgitgadget
Re: [PATCH 1/3] git: remove is_bare_repository_cfg global variable
In reply to John Cai via GitGitGadgetQuoted 3 lines · John Cai via GitGitGadget
The
is_bare_repository_cfgglobal variable is used for storing a bare repository setting, either through the config, an env var, or the commandline.
I found it curious that the above enumeration does not include the case where we go through the repository discovery process and find that we are in a bare repository. Looking at the original implementation of is_bare_repository() call, we do check if the directory structure does have a working tree when these three sources you listed above say "we are bare" or "we do not know yet".
So it would be be helpful if we made these two points
- The above enumeration is not meant to be exhausitive.
- The answer to anybody who asks "is this repository bare?" is more subtle than just reading the variable.
clear to readers.
Quoted 9 lines
This variable is global, and hence introduces global state everywhere it is used.
In order to reduce global state, add a member to the repository struct to keep track of the setting there. For now,
the_repositoryis what's used to set the member, which still represents global state. However, there is a parallel effort to replace calls tothe_repositorywith a repository struct that is passed into builtins, see [1]. Hence, this change will help the overall effort in reducing global state.
OK.
attr.c static enum git_attr_direction direction; · line 720
Quoted 11 lines
attr.c +1
-1@@ -716,7 +716,7 @@ static enum git_attr_direction direction;void git_attr_set_direction (enum git_attr_direction new_direction) { -if (is_bare_repository () && new_direction !=GIT_ATTR_INDEX ) +if (repo_is_bare (the_repository) && new_direction !=GIT_ATTR_INDEX )BUG ("non-INDEX attr direction in a bare repo" );
So everybody called is_bare_repository() which implicitly relied on the global variable now calls repo_is_bare() on the_repository, where the new member in the struct serves the purpose of the old global variable.
This replacement to repo_is_bare(the_repository) from is_bare_repository() is a recurring pattern in this patch, so I'll remove them from my quoting.
I've used coccinelle to apply this semantic patchlet
- is_bare_repository()
+ repo_is_bare(the_repository)
and then compared the result with applying this patch to see what else this patch contains, so I can comment on them.
Quoted 14 lines
builtin/clone.c +1
-1@@ -1415,7 +1415,7 @@ int cmd_clone(int argc,repo_clear (the_repository);/* At this point, we need the_repository to match the cloned repo. */ -if (repo_init (the_repository, git_dir, work_tree)) +if (repo_init (the_repository, git_dir, work_tree,-1 ))warning (_ ("failed to initialize the repo, skipping bundle URI" ));else if (fetch_bundle_uri (the_repository, bundle_uri, &has_heuristic)) warning(_("failed to fetch objects from bundle URI '%s'"),
builtin/clone.c int cmd_clone(int argc, · line 1452
Quoted 8 lines
builtin/clone.c +1
-1@@ -1446,7 +1446,7 @@ int cmd_clone(int argc,repo_clear (the_repository);/* At this point, we need the_repository to match the cloned repo. */ -if (repo_init (the_repository, git_dir, work_tree)) +if (repo_init (the_repository, git_dir, work_tree,-1 ))warning (_ ("failed to initialize the repo, skipping bundle URI" ));else if (fetch_bundle_list (the_repository, transport->bundles))
OK, so our repo_init() now takes one extra parameter. We'll see what the new parameter means when we look at the changes to repository.c.
builtin/init-db.c int cmd_init_db(int argc, · line 95
Quoted 13 lines
builtin/init-db.c +1
-1@@ -89,7 +89,7 @@ int cmd_init_db(int argc,const struct option init_db_options[] = {OPT_STRING (0 ,"template" , &template_dir,N_ ("template-directory" ),N_ ("directory from which templates will be used" )), -OPT_SET_INT (0 ,"bare" , &is_bare_repository_cfg, +OPT_SET_INT (0 ,"bare" , &the_repository->is_bare_cfg,N_ ("create a bare repository" ),1 ), {OPTION_CALLBACK ,0 ,"shared" , &init_shared_repository,N_ ("permissions" ),
As you said, this depends on the fact that the_repository is a pointer pointing at the static singleton variable the_repo at the compile time, so while it is already safe to take the address of the_repository->is_bare_cfg to prepare the array of options here, we haven't really solved the "we shouldn't be using this global variable" yet. But we can go one step at a time.
The remaining hunks in the file now all access the "global variable" via the_repository pointer, but the fact remains that the address of the thing being accessed is determined at the compile time, so it is just like accessing a global variable.
Which is naturally expected.
builtin/submodule--helper.c static int add_possible_reference_from_superproject( · line 1594
Quoted 10 lines
builtin/submodule--helper.c +1
-1@@ -1591,7 +1591,7 @@ static int add_possible_reference_from_superproject(struct strbuf err =STRBUF_INIT ;strbuf_add (&sb, odb->path, len); -if (repo_init (&alternate, sb.buf,NULL ) <0 ) +if (repo_init (&alternate, sb.buf,NULL , the_repository->is_bare_cfg) <0 )
OK. I do not recall what the original repo_init() did, but I presume that it initialized the new one depending on what the global variable said. We now propagate the setting from the superproject down to the submodule, which amounts to the same thing but probably is better as the "inheritance" is more explicitly visible here?
config.c static int git_default_core_config(const char *var, const char *value, · line 1446
Quoted 12 lines
config.c +1
-1@@ -1441,7 +1441,7 @@ static int git_default_core_config(const char *var, const char *value, }if (!strcmp (var,"core.bare" )) { - is_bare_repository_cfg =git_config_bool (var, value); + the_repository->is_bare_cfg =git_config_bool (var, value);return 0 ; }
OK. This is the same as what init-db did.
dir.c static void connect_wt_gitdir_in_nested(const char *sub_worktree, · line 4012
Quoted 11 lines
dir.c +1
-1@@ -4008,7 +4008,7 @@ static void connect_wt_gitdir_in_nested(const char *sub_worktree,const struct submodule *sub;/* If the submodule has no working tree, we can ignore it. */ -if (repo_init (&subrepo, sub_gitdir, sub_worktree)) +if (repo_init (&subrepo, sub_gitdir, sub_worktree, the_repository->is_bare_cfg))return ;
Same logic for submodule inheriting from the superproject?
environment.c int has_symlinks = 1; · line 37
Quoted 9 lines
environment.c +0
-1@@ -34,7 +34,6 @@ int has_symlinks = 1;int minimum_abbrev =4 , default_abbrev =-1 ;int ignore_case;int assume_unchanged; -int is_bare_repository_cfg =-1 ;/* unspecified */
This is now gone. We'll see a corresponding change in repository.c where the_repo instance is initialized, I presume.
environment.c const char *getenv_safe(struct strvec *argv, const char *name) · line 153
Quoted 9 lines
environment.c +0
-5@@ -146,12 +145,6 @@ const char *getenv_safe(struct strvec *argv, const char *name)return argv->v[argv->nr -1 ]; } -int is_bare_repository (void ) -{ -/* if core.bare is not 'false', let's see if there is a work tree */ -return is_bare_repository_cfg && !repo_get_work_tree (the_repository); -}
This is now gone, and we'll see a corresponding change in repository.c, I presume.
It is somewhat curious that in a repository where core.bare says true, we countermand it if we cannot figure out where its working tree is and say "core.bare is lying; we are in a bare repository".
The curiousity is not the fault of this patch, of course. The updated code in repository.c would hopefully have the same curiousity (or we'd be looking at an unintended behaviour change, if it didn't).
environment.h void set_shared_repository(int value); · line 147
Quoted 11 lines
environment.h +1
-2@@ -144,8 +144,7 @@ void set_shared_repository(int value);int get_shared_repository (void );void reset_shared_repository (void ); -extern int is_bare_repository_cfg; -int is_bare_repository (void ); +int is_bare_repository (struct repository *repo);
Curious. I somehow thought is_bare_repository() will be gone, and everybody is supposed to call repo_is_bare(the_repository), instead.
What makes a caller pick one over the other?
git.c static int handle_options(const char ***argv, int *argc, int *envchanged) · line 254
Quoted 10 lines
git.c +1
-1@@ -251,7 +251,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged) *envchanged =1 ; }else if (!strcmp (cmd,"--bare" )) {char *cwd =xgetcwd (); - is_bare_repository_cfg =1 ; + the_repository->is_bare_cfg =1 ;
OK. The same as how init-db and config now access the new member of the global singleton the_repository instead of the global variable.
repository.c line 30
Quoted 12 lines
repository.c +3
-1@@ -25,7 +25,9 @@extern struct repository *the_repository;/* The main repository */ -static struct repository the_repo; +static struct repository the_repo = { + .is_bare_cfg =-1 , +};
OK, this is just as expected by reading the patch so far.
repository.c static int read_and_verify_repository_format(struct repository_format *format, · line 269
Quoted 6 lines
repository.c +2
-0@@ -263,10 +265,13 @@ static int read_and_verify_repository_format(struct repository_format *format,/* * Initialize 'repo' based on the provided 'gitdir'. * Return 0 upon success and a non-zero value upon failure. +* is_bare can be passed to indicate whether or not the repository should be +* treated as bare when repo_init() is used to initiate a secondary repository.
"initiate" -> "initialize" perhaps?
Quoted 9 lines
repository.c +2
-1int repo_init (struct repository *repo,const char *gitdir, -const char *worktree) +const char *worktree, +int is_bare) {struct repository_format format =REPOSITORY_FORMAT_INIT ;memset (repo,0 ,sizeof (*repo));
repository.c int repo_init(struct repository *repo, · line 292
Quoted 5 lines
repository.c +2
-0@@ -283,6 +288,8 @@ int repo_init(struct repository *repo,repo_set_compat_hash_algo (repo, format.compat_hash_algo);repo_set_ref_storage_format (repo, format.ref_storage_format); repo->repository_format_worktree_config = format.worktree_config; +if (is_bare >0 ) + repo->is_bare_cfg = is_bare;
When repo_init() is called with anything other than &the_repo, who initializes repo->is_bare_cfg? If the answer is "nobody", shouldn't this function be doing something like
repo->is_bare_cfg = (0 <= is_bare) ? is_bare : -1;
which actually amounts to an unconditional
repo->is_bare_cfg = is_bare;
as is_bare can only take one of (-1, 0, 1).
Perhaps I am missing some subtleties in the original construction you wrote? You leave repo->is_bare_cfg unset when is_bare parameter explicitly says "false", which I suspect might be related to the source of confusion I am having.
repository.c int repo_hold_locked_index(struct repository *repo, · line 468
Quoted 5 lines
repository.c +5
-0+int repo_is_bare (struct repository *repo) +{ +/* if core.bare is not 'false', let's see if there is a work tree */ +return repo->is_bare_cfg && !repo_get_work_tree (repo); +}
The curiosity we saw in the original implementation of is_bare_repository() above is faithfully reproduced, which is good.
Quoted 7 lines
repository.h +0
-0@@ -153,6 +153,14 @@ struct repository {/* Indicate if a repository has a different 'commondir' from 'gitdir' */
repository.h struct repository { · line 164
Quoted 10 lines
repository.h +8
-0unsigned different_commondir:1; + +/* +* Indicates if the repository is set to be treated as a bare repository, +* through a command line argument, configuration, or environment +* variable. +* -1 means unspecified, 0 indicates non-bare, and 1 indicates bare. +*/ +int is_bare_cfg; };
I am very happy with the above phrasing. The member tells us what the repo is "set to be treated as", which implies that the code does a bit more on top of that setting.
scalar.c static int cmd_reconfigure(int argc, const char **argv) · line 726
Quoted 11 lines
scalar.c +1
-1@@ -722,7 +722,7 @@ static int cmd_reconfigure(int argc, const char **argv)git_config_clear (); -if (repo_init (&r, gitdir.buf, commondir.buf)) +if (repo_init (&r, gitdir.buf, commondir.buf, the_repository->is_bare_cfg)) goto loop_end;
Given this caller, if is_bare_cfg of the current state says "I am set to be treated as a non-bare repository" by having 0, shouldn't repo_init() copy it to the repository at r? IOW, this yells at me saying that repo_init() patch we saw earlier is somewhat buggy.
setup.c static int check_repository_format_gently(const char *gitdir, struct repository_ · line 771
Quoted 13 lines
setup.c +2
-2@@ -766,8 +766,8 @@ static int check_repository_format_gently(const char *gitdir, struct repository_if (!has_common) {if (candidate->is_bare !=-1 ) { - is_bare_repository_cfg = candidate->is_bare; -if (is_bare_repository_cfg ==1 ) + the_repository->is_bare_cfg = candidate->is_bare; +if (the_repository->is_bare_cfg ==1 ) inside_work_tree =-1 ;
OK, this is as expected.
All other hunks to this file, except for the last one, follow the same pattern as init-db and config to access the member of the singleton struct instead of global variable. And the last one is to call repo_is_bare(the_repository) instead of is_bare_repository().
Makes sense.
Quoted 14 lines
transport.c +1
-1@@ -1428,7 +1428,7 @@ int transport_push(struct repository *r,if ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |TRANSPORT_RECURSE_SUBMODULES_ONLY )) && - !is_bare_repository ()) { + !repo_is_bare (r)) {struct ref *ref = remote_refs;struct oid_array commits =OID_ARRAY_INIT ;
transport.c int transport_push(struct repository *r, · line 1461
Quoted 8 lines
transport.c +1
-1@@ -1455,7 +1455,7 @@ int transport_push(struct repository *r,if (((flags &TRANSPORT_RECURSE_SUBMODULES_CHECK ) || ((flags & (TRANSPORT_RECURSE_SUBMODULES_ON_DEMAND |TRANSPORT_RECURSE_SUBMODULES_ONLY )) && - !pretend)) && !is_bare_repository ()) { + !pretend)) && !repo_is_bare (r)) {struct ref *ref = remote_refs;struct string_list needs_pushing =STRING_LIST_INIT_DUP ;struct oid_array commits =OID_ARRAY_INIT ;
OK, these are better than the mechanical "singleton the_repository is the new home for the global". It makes it even more important for us to answer "Who initializes the repository r? Is is_bare_cfg initialized to -1 just like the_repo.is_bare_cfg is? Is repo_init() doing the right thing to update it by doing only when it is set to 1 but ignoring -1 and 0 as incoming parameter?" questions, though.
worktree.c static struct worktree *get_main_worktree(int skip_reading_head) · line 89
Quoted 12 lines
worktree.c +2
-2@@ -85,8 +85,8 @@ static struct worktree *get_main_worktree(int skip_reading_head)* This means that worktree->is_bare may be set to 0 even if the main * worktree is configured to be bare. */ - worktree->is_bare = (is_bare_repository_cfg ==1 ) || -is_bare_repository (); + + worktree->is_bare = the_repository->is_bare_cfg ==1 ;
If this changes the behaviour subtly without explaining, it needs to be justified, I suspect.
We used to pay attention to what is_bare_repository() says, which is a bit more than "set to bbe treated as" with config. We no longer do so, and also unconfigured case is always treated as non-bare.
Re: [PATCH 2/3] setup: initialize is_bare_cfg
In reply to John Cai via GitGitGadgetQuoted 10 lines · John Cai via GitGitGadget
From: John Cai <[email protected]>
A subsequent commit will
BUG()when theis_bare_cfgmember is uninitialized. Since there are still some codepaths that initializing theis_bare_cfgvariable, initialize them.Signed-off-by John Cai <[email protected]>--- setup.c | 7 +++++++ 1 file changed, 7 insertions(+)
I am not sure about the wisdom of this step (and the next one). Before this step, it used to be that the global variable (or the_repository->is_bare_cfg) can be inspected to see if there is an explicit "set to be treated as", or nobody told us if the repository ought to be bare (or not). With this and the next step, that is no longer possible, yet we still do the "core.bare says it is either true or unconfigured, so we ask repo_get_work_tree() and it returns NULL, so it is bare", which feels awfully inconsistent. Especially the change from the next patch
repository.c int repo_hold_locked_index(struct repository *repo, · line 470
Quoted 12 lines
repository.c +2
-0@@ -464,5 +464,7 @@ int repo_hold_locked_index(struct repository *repo,int repo_is_bare (struct repository *repo) {/* if core.bare is not 'false', let's see if there is a work tree */ +if (repo->is_bare_cfg <0 ) +BUG ("is_bare_cfg unspecified" );return repo->is_bare_cfg && !repo_get_work_tree (repo); }
the returned value does not make much sense anymore. One half of it used to be "if not configured, we can ask if there is worktree and the lack of one by definition means we are bare", which made perfect sense, but now what remains is "the configuration says it is, but when we ask if there is a worktree, there is, so it is not bare after all", which is somewhat dubious.
And if the goal of steps 2 & 3 is to redefine what is_bare_cfg means and make it "this is the only thing we need to check if the repository is bare" (which by itself is not a bad thing), shouldn't the checking of worktree be done where the code assigns true to the repo->is_bare_cfg, no?
setup.c static int check_repository_format_gently(const char *gitdir, struct repository_ · line 744
Quoted 9 lines
setup.c +1
-0@@ -741,6 +741,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_if (verify_repository_format (candidate, &err) <0 ) {if (nongit_ok) { + the_repository->is_bare_cfg =1 ;
It is unclear how we can be certain that we are looking at a bare repository in this case. We do not even understand the repository format, GIT_DIR we were given to decide which file called "config" may not even be a repository. We are losing a bit of information (i.e. nobody has told us if we ought to treat the repository as a bare one, or a non-bare one") by overriding the value here.
setup.c static const char *setup_explicit_git_dir(const char *gitdirenv, · line 1022
Quoted 6 lines
setup.c +1
-0@@ -1017,6 +1018,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,if (nongit_ok) { *nongit_ok =1 ;free (gitfile); + the_repository->is_bare_cfg =0 ;return NULL ;
Ditto.
setup.c static const char *setup_explicit_git_dir(const char *gitdirenv, · line 1074
Quoted 5 lines
setup.c +1
-0@@ -1069,6 +1071,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,/* set_git_work_tree() must have been called by now */ worktree =repo_get_work_tree (the_repository); + the_repository->is_bare_cfg =0 ;
What if worktree is NULL? Wouldn't it be more meaningful to say is_bare_cfg is true in such a case?
setup.c static const char *setup_discovered_git_dir(const char *gitdir, · line 1133
Quoted 7 lines
setup.c +3
-0@@ -1125,6 +1128,9 @@ static const char *setup_discovered_git_dir(const char *gitdir,/* #0, #1, #5, #8, #9, #12, #13 */ set_git_work_tree ("." ); + +if (the_repository->is_bare_cfg <0 ) + the_repository->is_bare_cfg =0 ;
OK. We did discovery, is_bare_cfg did not say true (it would have returned before we got here if is_bare_cfg were set to true). We decided to treat the current directory as the top of the working tree, so by definition, we are not treating the repository as bare.
But this makes me wonder what should happen the_repository->is_bare_cfg is already set to true. Shouldn't that be a BUG()?
setup.c const char *setup_git_directory_gently(int *nongit_ok) · line 1776
Quoted 5 lines
setup.c +1
-0@@ -1767,6 +1773,7 @@ const char *setup_git_directory_gently(int *nongit_ok)die (_ ("not a git repository (or any of the parent directories): %s" ),DEFAULT_GIT_DIR_ENVIRONMENT ); *nongit_ok =1 ; + the_repository->is_bare_cfg =1 ;
This is not bare nor non-bare---simply we did not find any usable git repository, and we lose the single bit of information "nobody told us to treat the repository as bare or non-bare".
Not that the loss of information is a huge deal. But having to make an arbitrary choice like the above (and similar ones in previous hunks where we didn't have any repository to begin with) is an indication that the entire "is_bare_cfg must mean if our repository is bare or non-bare" premise patch 3/3 wants to enforce may be misguided, I am afraid.
Quoted 3 lines
setup.c +0
-0break ;case GIT_DIR_HIT_MOUNT_POINT :if (!nongit_ok)
Re: [PATCH 1/3] git: remove is_bare_repository_cfg global variable
In reply to Junio C HamanoOn Thu, Nov 07, 2024 at 02:46:15PM +0900, Junio C Hamano wrote:
[snip]
Quoted 9 lines
Quoted 9 lines
int repo_init(struct repository *repo,
repository.c +2
-1const char *gitdir, - const char *worktree) + const char *worktree, + int is_bare) { struct repository_format format = REPOSITORY_FORMAT_INIT;memset (repo,0 ,sizeof (*repo));@@ -283,6 +288,8 @@ int repo_init(struct repository *repo,
Quoted 8 lines
repository.c int repo_init(struct repository *repo, · line 292
Quoted 5 lines
repository.c +2
-0repo_set_compat_hash_algo (repo, format.compat_hash_algo);repo_set_ref_storage_format (repo, format.ref_storage_format); repo->repository_format_worktree_config = format.worktree_config; +if (is_bare >0 ) + repo->is_bare_cfg = is_bare;When
repo_init()is called with anything other than&the_repo, who initializesrepo->is_bare_cfg?
I also want to ask this question. Actually, I feel quite strange about why we need to add a new parameter is_bare to repo_init function.
For this call:
repo_init(the_repository, git_dir, work_tree, -1);
We add a new field "is_bare_cfg" to the "struct repository". So, at now, the_repository variable should contain the information about whether the repo is bare(1), is not bare(0) or unknown(-1). However, in this call, we pass "-1" to the parameter is_bare for "repo_init" function.
When I first look at this code, I have thought that we will set "repo->is_bare_cfg = -1" to indicate that we cannot tell whether the repo is bare or not. But it just sets the "repo->is_bare_cfg = is_bare" if bare > 0. Junio has already commented on this.
This raises a question: why we need to set up is_bare_cfg in the repo_init function? I guess this is because we need to set up other "struct repository" parameter like the following:
if (repo_init(&alternate, sb.buf, NULL, the_repository->is_bare_cfg) < 0)
And I think it's better for us to use the following way.
alternate->is_bare_cfg = the_repository->is_bare_cfg; if (repo_init(&alternate, sb.buf, NULL))
And we may create a function called repo_copy_settings to set up the common setting inherited from an existing repo:
repo_copy_settings(alternate, the_repository); if (repo_init(&alternate, sb.buf, NULL))
I agree that we could put is_bare_cfg to "struct repository *". But I don't agree with the idea that we need to pass is_bare to repo_init. I think we should know whether the repo is bare or not before calling repo_init. And from my understanding, this is what we are doing now.
Also, I think we may add a enum type instead of using (-1, 0, 1). (However, this is not the main point of this patch).
Thanks,
Jialuo
Re: [PATCH 1/3] git: remove is_bare_repository_cfg global variable
Quoted 11 lines · shejialuo
I also want to ask this question. Actually, I feel quite strange about why we need to add a new parameter
is_baretorepo_initfunction.For this call:
repo_init(the_repository, git_dir, work_tree, -1);We add a new field "
is_bare_cfg" to the "struct repository". So, at now,the_repositoryvariable should contain the information about whether the repo is bare(1), is not bare(0) or unknown(-1). However, in this call, we pass "-1" to the parameteris_barefor "repo_init" function.
Isn't this merely trying to be faithful to the original to avoid unintended behaviour change? We initialize the global variable is_bare_repository_cfg to unspecified(-1) in the original, and for a rewrite to move the global to a member in the singleton instance of the_repo, it would need to be able to do the same.
And for callers of repo_init() that prepares _another_ in-core repository instance, which is different from the_repository, because the original has a process-wide singleton global variable, copying the value from the_repository->is_bare to a newly initialized one would hopefully give us the most faithful rewrite to avoid unintended behaviour change.
At least, that is how I understood why the patch does it this way. As you noticed, too, there are ...
Quoted 4 lines
When I first look at this code, I have thought that we will set "
repo->is_bare_cfg= -1" to indicate that we cannot tell whether the repo is bare or not. But it just sets the "repo->is_bare_cfg=is_bare" ifbare > 0. Junio has already commented on this.
... places in the updated code that makes it unclear what the is_bare member really means. The corresponding global variable used to be "this is what we were told by config or env or command line", but it is unclear, with conditional assignments like the above, what it means in the updated code.
Thanks.
Re: [PATCH 1/3] git: remove is_bare_repository_cfg global variable
Quoted 20 lines · Junio C Hamano
Quoted 11 lines · shejialuo
I also want to ask this question. Actually, I feel quite strange about why we need to add a new parameter
is_baretorepo_initfunction.For this call:
repo_init(the_repository, git_dir, work_tree, -1);We add a new field "
is_bare_cfg" to the "struct repository". So, at now,the_repositoryvariable should contain the information about whether the repo is bare(1), is not bare(0) or unknown(-1). However, in this call, we pass "-1" to the parameteris_barefor "repo_init" function.Isn't this merely trying to be faithful to the original to avoid unintended behaviour change? We initialize the global variable
is_bare_repository_cfgto unspecified(-1) in the original, and for a rewrite to move the global to a member in the singleton instance ofthe_repo, it would need to be able to do the same.
Quoted 7 lines
And for callers of
repo_init()that prepares _another_ in-core repository instance, which is different fromthe_repository, because the original has a process-wide singleton global variable, copying the value fromthe_repository->is_bareto a newly initialized one would hopefully give us the most faithful rewrite to avoid unintended behaviour change.
Yes, I agree that this is the most faithful way to make sure the consistency when we want to create a new repo instead of letting the caller do this itself.
So, I think what I feel strange is that we need to do this assignment. Because we make a global variable not global by incorporating this into "struct repository *", we have to maintain this state whenever we create a new "repo".
It lets me think whether we should place "is_bare_cfg" into "struct repository" in the first place. I will explain why in the later comments.
Quoted 8 lines
At least, that is how I understood why the patch does it this way. As you noticed, too, there are ...
Quoted 4 lines
When I first look at this code, I have thought that we will set "
repo->is_bare_cfg= -1" to indicate that we cannot tell whether the repo is bare or not. But it just sets the "repo->is_bare_cfg=is_bare" ifbare > 0. Junio has already commented on this.
Quoted 6 lines
... places in the updated code that makes it unclear what the
is_baremember really means. The corresponding global variable used to be "this is what we were told by config or env or command line", but it is unclear, with conditional assignments like the above, what it means in the updated code.
Yes, John has changed the corresponding code paths by setting the global variable "the_repository->is_bare_cfg". So, we will refactor this later.
In the previous days, Kousik wanted to make "builtin/mailinfo" not to reply on "the_repository". I have commented in
https://lore.kernel.org/git/Zw6SsUyZ0oA0XqMK@ArchLinux/
In this thread, I do not agree that we should not incorporate the global variables in "git_commit_encoding" and "git_log_output_encoding" in "environment.c" into "struct repository *" because we could use these two configs outside of the repo.
So, I don't think it's a good idea to put into "is_bare_cfg" into "struct repository". Put it further more, we should not put the global variables in "struct repository" structure for the following reasons:
1. These variables are used across the whole lifecycle. Not just only related to the repository. Some variables could be used outside of the repo.
2. Currently, the config functions which set up these variables don't have parameters to access the "struct repository *". Of course, we could add the parameter, but as 1 shows, some variables could be used outside of the repo. We may need many efforts for such situation.
3. We need to maintain the consistency if we create a new "struct repository", because we will make global variables not global.
So, in my perspective, we may just create a new structure called "struct env" to incorporate all these variables in "environment.c" just like what we have done for "struct repository *". But we also introduced another overhead, we may pass this structure to every function when setting up.
Quoted 1 line
Thanks.
Thanks,
Jialuo
Quoted 5 lines · John Cai via GitGitGadget
This patch series removes the global state introduced by the
is_bare_repository_cfgvariable by moving it into the repository struct. Most of the refactor is done by patch 1. Patch 2 initializes the member in places that left it unInitialized, while patch 3 adds a safety measure by BUG()ing when the variable has not been properly initialized.
I think these patches go in the right direction in general, but the topic hasn't seen much activity for a few weeks since they received review messages. Is a new revision being worked on, or is the topic being backburnered?
Thanks.
(RFH Windows breakage) Re: [PATCH 0/3] Remove is_bare_repository_cfg global state
In reply to John Cai via GitGitGadgetQuoted 10 lines · John Cai via GitGitGadget
This patch series removes the global state introduced by the
is_bare_repository_cfgvariable by moving it into the repository struct. Most of the refactor is done by patch 1. Patch 2 initializes the member in places that left it unInitialized, while patch 3 adds a safety measure by BUG()ing when the variable has not been properly initialized.John Cai (3):
git: removeis_bare_repository_cfgglobal variable
setup: initializeis_bare_cfg
repository: BUG whenis_bare_cfgis not initialized
We've been seeing a job "win test (5)" fail on 'seen' for a while, and I happened to have rebuilt 'seen' without this topic (first by accident) and the job started passing.
The topic coming from GGG, I'd assume that it byitself will pass the tests (including Windows ones), so I suspect it is some interaction with other topics in 'seen'.
As I do not have Windows environment to test and dig into any problem, often pushing 'seen' with suspect topic(s) removed is the only way for me to isolate which topic might be causing a problem, and after doing so, I'll have to leave it up to the author of the topic to dig further with help from others.
(failing) https://github.com/git/git/actions/runs/12279217687/job/34263221584
(passing) https://github.com/git/git/actions/runs/12286174648/job/34286039276
The difference between these is that the former (failing) one has this topic with three patches merged at the tip of 'seen', and the latter (passing) one is the result of tentatively dropping this topic from the CI run.
Thanks.