Skip to content

store repo specific config values in new `struct repo_config_values`

Olamide Caleb Bello git 7 versions – In review

Get series with b4
b4 am [email protected]

Versions newest first · linked by author and title

  1. v7 Latest3 patches 7 repliesIn review
  2. v63 patches 8 repliesSuperseded
  3. v53 patches 6 repliesSuperseded
Earlier versions · 4
VersionPostedPatchesRepliesStatus
v43 patches0 repliesSuperseded
v33 patches13 repliesSuperseded
v23 patches7 repliesSuperseded
v1 RFC3 patches2 repliesSuperseded

Changes in v7 from the cover letter

==============
- Added the code to reset the initialization of the_repository in the fuzz-commit-graph test in Patch 1.
- Changed the call to rep_config_values_init() in initialze_repository() by passing it the repo config_values_private_ struct instead of the accessor function in Patch 1.
- Modified the commit message in Patch 1
- Added extra lines between variable declarations and code

Patches in v7 No applies recorded yet

#Subject and filesSizeRepliesApplied
1/3environment: stop storing `core.attributesFile` globallyattr.c environment.c environment.h +3 more+45 -70 replies
2/3environment: stop using core.sparseCheckout globallybuiltin/backfill.c builtin/clone.c builtin/grep.c +9 more+46 -243 replies
3/3environment: move "branch.autoSetupMerge" into `struct repo_config_values`branch.h builtin/branch.c builtin/checkout.c +4 more+19 -110 replies

Reviews trailers per patch

None of the 3 patches has a Reviewed-by or Acked-by.

PatchChristian CouderUsman AkinyemiJunio C Hamano
SeriesMentored-by
1/3Mentored-by Mentored-by Helped-by
2/3Mentored-by Mentored-by
3/3Mentored-by Mentored-by

Trailers given in replies are filled; those carried in the patches from earlier versions are outlined with dashes. Scroll the table sideways for all 3 reviewers.

Read the v7 thread

Cover letter [Outreachy PATCH v7 0/3] store repo specific config values in new `struct repo_config_values`

Hi Git Community,
Over the course of my ongoing internship, which focused on moving repo specific global variables in environment.h into local scope, I have attempted to move some variables into the struct repo-settings.
However there has been some design concerns as regards the use of prepare_repo_settings() with respect to when and where to call the function, and also the change in behaviours when the variable is lazily loaded as discussed in [1] and [2].

After different deliberations, Phillip Wood proposed creating a new config struct [3], adding it to the repository struct and passing the repo struct to git_default_config() to store parsed repo specific config values per repo. This ensures the current behaviours will be retained.

In version v6, the fuzz-commit-graph failed due to the test being run multiples times in the same process. This makes us get a BUG due to the repository being reinitialized in the same process. But now resetting the initialization using memset before rerunning the test, the failure is not longer experienced.

1. https://lore.kernel.org/git/[email protected]/
2. https://lore.kernel.org/git/[email protected]/
3. https://lore.kernel.org/git/[email protected]
4. https://github.com/git/git/actions/runs/21633462506/job/62352191148?pr=2166
5. https://gitlab.com/gitlab-community/gitlab-org/git/-/pipelines/2303203992

Changes in v7:
==============
- Added the code to reset the initialization of the_repository in the fuzz-commit-graph test in Patch 1.
- Changed the call to rep_config_values_init() in initialze_repository() by passing it the repo config_values_private_ struct instead of the accessor function in Patch 1.
- Modified the commit message in Patch 1
- Added extra lines between variable declarations and code

Olamide Caleb Bello (3):
environment: stop storing core.attributesFile globally
environment: stop using core.sparseCheckout globally
environment: move "branch.autoSetupMerge" into `struct
repo_config_values`

 attr.c                       |  7 ++++---
 branch.h                     |  2 --
 builtin/backfill.c           |  3 ++-
 builtin/branch.c             |  3 ++-
 builtin/checkout.c           |  3 ++-
 builtin/clone.c              |  4 +++-
 builtin/grep.c               |  2 +-
 builtin/mv.c                 |  3 ++-
 builtin/push.c               |  3 ++-
 builtin/sparse-checkout.c    | 31 ++++++++++++++++++++-----------
 builtin/submodule--helper.c  |  3 ++-
 builtin/worktree.c           |  3 ++-
 dir.c                        |  4 +++-
 environment.c                | 28 ++++++++++++++++++----------
 environment.h                | 17 +++++++++++++++--
 oss-fuzz/fuzz-commit-graph.c |  1 +
 repository.c                 | 14 ++++++++++++++
 repository.h                 |  7 +++++++
 sparse-index.c               |  7 +++++--
 unpack-trees.c               |  3 ++-
 wt-status.c                  |  4 +++-
 21 files changed, 110 insertions(+), 42 deletions(-)

Range diff versus v6:
=====================
1: 7e3082125d ! 1: 48821a3848 environment: stop storing core.attributesFile globally
@@ Commit message

The core.attributeFile config value is parsed in git_default_core_config(), loaded eagerly and stored in the global
- variable git_attributes_file. Storing this value in a global variable
- can lead to it being overwritten by another repository when more than one
- Git repository run in the same Git process.
+ variable git_attributes_file. Storing this value in a global
+ variable can lead to it being overwritten by another repository when
+ more than one Git repository run in the same Git process.

Create a new struct repo_config_values to hold this value and other repository dependent values parsed by git_default_config(). This will ensure the current behaviour remains the same while also enabling the libification of Git.

- An accessor function 'repo_config_values()' is created and used to access
- the new struct member of the repository struct.
- This is to ensure that we detect if the struct repository has been
- initialized and also prevent double initialization of the repository.
-
- It is important to note that git_default_config() is a wrapper to other
- git_default_*_config() functions such as git_default_core_config().
- Therefore to access and modify this global variable,
- the change has to be made git_default_core_config().
+ An accessor function 'repo_config_values()' to ensure that we do not
+ access an uninitialized repository, or an instance of a different
+ repository than the current one.

@@ environment.h: extern int assume_unchanged;
extern int pack_compression_level; extern unsigned long pack_size_limit_cfg;

+ ## oss-fuzz/fuzz-commit-graph.c ##
+@@ oss-fuzz/fuzz-commit-graph.c: int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
+ {
+ struct commit_graph *g;
+
++ memset(the_repository, 0, sizeof(*the_repository));
+ initialize_repository(the_repository);
+
+ /*
+
## repository.c ##
@@ repository.c: static void set_default_hash_algo(struct repository *repo)
repo_set_hash_algo(repo, algo);
@@ repository.c: static void set_default_hash_algo(struct repository *repo)

+struct repo_config_values *repo_config_values(struct repository *repo) +{
++ if (repo != the_repository)
++ BUG("trying to read config from wrong repository instance");
+ if(!repo->initialized)
+ BUG("config values from uninitialized repository");
+ return &repo->config_values_private_;
@@ repository.c: static void set_default_hash_algo(struct repository *repo)
ALLOC_ARRAY(repo->index, 1);
index_state_init(repo->index, repo); repo->check_deprecated_config = true;
-+ repo_config_values_init(repo_config_values(repo));
++ repo_config_values_init(&repo->config_values_private_);

/*
* When a command runs inside a repository, it learns what
2: 8645b4f595 ! 2: 7d33f1ef0f environment: stop using core.sparseCheckout globally

    @@ builtin/clone.c: static int git_sparse_checkout_init(const char *repo)
      	struct child_process cmd = CHILD_PROCESS_INIT;
      	int result = 0;
     +	struct repo_config_values *cfg = repo_config_values(the_repository);
    ++
      	strvec_pushl(&cmd.args, "-C", repo, "sparse-checkout", "set", NULL);

      	/*
    @@ dir.c: enum pattern_match_result path_matches_pattern_list(
      {
     -	if (!core_apply_sparse_checkout)
     +	struct repo_config_values *cfg = repo_config_values(the_repository);
    ++
     +	if (!cfg->apply_sparse_checkout)
      		return 1;
      	if (istate->sparse_checkout_patterns)
    @@ sparse-index.c: static int index_has_unmerged_entries(struct index_state *istate
      {
     -	if (!core_apply_sparse_checkout || !core_sparse_checkout_cone)
     +	struct repo_config_values *cfg = repo_config_values(the_repository);
    ++
     +	if (!cfg->apply_sparse_checkout || !core_sparse_checkout_cone)
      		return 0;

3: 60451b93a5 = 3: c3ecfa63b9 environment: move "branch.autoSetupMerge" into struct repo_config_values

Signature · 1 line
2.34.1