Conversation
As part of the ongoing libification effort, dynamically allocated global configuration variables are being moved into 'struct repo_config_values'. To prevent memory leaks, we need a destructor to free these heap-allocated variables when a repository instance is torn down. Introduce 'repo_config_values_clear()' in environment.c and invoke it from 'repo_clear()' in repository.c. As a starting point, update this new function to handle the cleanup of 'attributes_file'. Note: Submodules are currently not supported by repo_config_values(), which explicitly BUG()s out if 'repo != the_repository'. Since repo_clear() cleans up all repository instances, we must bypass them to prevent crashing. Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com> Mentored-by: Olamide Caleb Bello <belkid98@gmail.com> Signed-off-by: Tian Yuchen <cat@malon.dev>
The global variable 'excludes_file' is used to track the path to the global ignore file. If this variable is NULL, 'setup_standard_excludes()' in 'dir.c' forcefully evaluates and assigns the XDG default path to it. Continue the libification effort by encapsulating this lazy-loading fallback logic into a proper getter and moving the variable into 'struct repo_config_values'. Since 'excludes_file' is a dynamically allocated string, it requires proper heap memory management. It is safely freed using the newly introduced `repo_config_values_clear()` function when the repository is torn down. Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com> Mentored-by: Olamide Caleb Bello <belkid98@gmail.com> Signed-off-by: Tian Yuchen <cat@malon.dev>
The global variable 'editor_program' holds the path to the user's preferred editor. Move 'editor_program' into 'struct repo_config_values' to continue the libification effort. There have been discussions on whether external programs like editors truly need to be configured on a per-repository basis within the same process. While a single process might rarely invoke different editors, this migration is necessary for two reasons: 1. Developers frequently use different toolchains for different projects. Per-repo configuration respects this. 2. Moving this string into 'repo_config_values' eliminates mutable global state. As the codebase moves toward becoming a long-running processes managing multiple repositories concurrently must not overwrite each other's program configurations. No standalone getter function is introduced. Callers directly access the field via 'repo_config_values()'. Heap memory is safely reclaimed in 'repo_config_values_clear()'. Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com> Mentored-by: Olamide Caleb Bello <belkid98@gmail.com> Signed-off-by: Tian Yuchen <cat@malon.dev>
The 'pager_program' variable is currently defined as a file-scoped static string in pager.c. Move it into 'struct repo_config_values'. The configuration parsing logic remains strictly within pager.c to respect subsystem boundaries. The read/write operations are simply redirected to the repository-specific structure using 'repo_config_values()'. Similar to the recent editor_program migration, no standalone getter is introduced to keep the code minimal. The dynamically allocated memory is now managed by 'repo_config_values_clear()'. On top of that, fix a memory leak in pager.c while we are at it. Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com> Mentored-by: Olamide Caleb Bello <belkid98@gmail.com> Signed-off-by: Tian Yuchen <cat@malon.dev>
The global variable 'askpass_program' stores the path to the program used to prompt the user for credentials. Move it into repo_config_values to continue the libification effort. While it is uncommon for a single process to require different askpass programs for different repositories, maintaining this value as a mutable global string is a blocker for libification. Global heap-allocated strings introduce thread-safety issues in a multi-repo environment. Move 'askpass_program' into 'struct repo_config_values' to eliminate this global state. The memory is now safely managed and freed via 'repo_config_values_clear()'. Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com> Mentored-by: Olamide Caleb Bello <belkid98@gmail.com> Signed-off-by: Tian Yuchen <cat@malon.dev>
…ewhitespace The global variables 'apply_default_whitespace' and 'apply_default_ignorewhitespace' are used to store the default whitespace configuration for 'git apply'. Move these variables into 'struct repo_config_values' to continue the libification effort. Dynamically allocated strings fetched via 'repo_config_get_string()' are now tracked per-repository and safely freed in 'repo_config_values_clear()'. As part of this transition, update 'git_apply_config()' to accept a 'struct repository *' argument rather than relying on the 'the_repository' global. Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com> Mentored-by: Olamide Caleb Bello <belkid98@gmail.com> Signed-off-by: Tian Yuchen <cat@malon.dev>
The global variable 'push_default' specifies the default behavior of 'git push' when no explicit refspec is provided. Move 'push_default' into 'struct repo_config_values' to continue the libification effort. While 'enum push_default_type' ideally belongs in 'remote.h', moving it there introduces a circular dependency chain: remote.h -> hash.h -> repository.h -> environment.h. Therefore, the enum definition is kept in 'environment.h' just above 'struct repo_config_values' with a NEEDSWORK comment for future cleanup. Modify the configuration parsing in environment.c to update the per-repository structure directly, and update caller across the codebase to access the value via 'repo_config_values()'. Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com> Mentored-by: Olamide Caleb Bello <belkid98@gmail.com> Signed-off-by: Tian Yuchen <cat@malon.dev>
The global variable 'autorebase' dictates whether a newly created branch should be configured to automatically rebase by default. Move it into 'struct repo_config_values' to continue the libification effort. The 'enum rebase_setup_type' definition is moved higher up in 'environment.h' so that it is visible to the repository-specific structure. The default state AUTOREBASE_NEVER is now correctly initialized in 'repo_config_values_init()'. Configuration parsing in 'git_default_branch_config()' is updated to write directly to the repository's configuration instance. Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com> Mentored-by: Olamide Caleb Bello <belkid98@gmail.com> Signed-off-by: Tian Yuchen <cat@malon.dev>
The global variable 'object_creation_mode' controls how Git creates object files, specifically determining whether to use hardlinks or renames when moving temporary files into the object database. Move it into 'struct repo_config_values' to continue the libification effort. Move the 'enum object_creation_mode' definition higher up in 'environment.h' to ensure it is visible to the structure. Initialize the per-repository value to its default macro value OBJECT_CREATION_MODE inside 'repo_config_values_init()'. Update configuration parsing in 'git_default_core_config()' to write directly to the repository-specific configuration structure. Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Ayush Chandekar <ayu.chandekar@gmail.com> Mentored-by: Olamide Caleb Bello <belkid98@gmail.com> Signed-off-by: Tian Yuchen <cat@malon.dev>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Final version?
Subject: [PATCH v8 0/9] migrate more variables into repo_config_values
Hi everyone,
This patch series continues the ongoing libification effort by migrating
a batch of global configuration variables into struct repo_config_values.
What does this series do:
infrastructure & strings (commits 1-6):
Introduce 'repo_config_values_clear()' to manage the lifecycle
of heap-allocated configuration strings. This infrastructure is utilized
to migrate string variables, including 'excludes_file', 'apply' whitespace
configs, and external programs including 'editor', 'pager', 'askpass'.
enums (commits 7-9):
Migrate enumerations 'push_default', 'autorebase', and
'object_creation_mode'. Care was taken to make these types available
to the configuration structure without triggering circular header
dependencies.
RFC:
Commit 3~5. Is it really necessary to migrate _program variables?
https://lore.kernel.org/git/8e657184-ee0b-453a-9f2d-a98080d3582e@gmail.com/
Commit 6~9. Previous related discussions on 'git_branch_track'.
https://lore.kernel.org/git/CAD=f0L-mPX+KECUjXk-WBzEbTP7wCa8sB56GySQT0yh9mfUOWw@mail.gmail.com/
Note:
Since a new getter 'repo_excludes_file()' is introduced, as previously
promised, once it is finally merged into 'master', there will be a patch to
update and squash the comments.
Similarly, I've noticed that the classification and sorting of variables in
'repo_config_values' don't seem to be correct. There will also be a patch
to fix this, and I think it will form a commit series along with the comment
patch?
Change since v7:
Fixed a memory leak in pager.c.
Thanks!
Tian Yuchen (9):
repository: introduce repo_config_values_clear()
environment: move excludes_file into repo_config_values
environment: move editor_program into repo_config_values
environment: move pager_program into repo_config_values
environment: move askpass_program into repo_config_values
environment: migrate apply_default_whitespace and
apply_default_ignorewhitespace
environment: move push_default into repo_config_values
environment: move autorebase into repo_config_values
environment: move object_creation_mode into repo_config_values
apply.c | 20 +++++++-----
branch.c | 2 +-
builtin/push.c | 8 ++---
dir.c | 4 +--
editor.c | 4 +--
environment.c | 87 +++++++++++++++++++++++++++++++++++---------------
environment.h | 75 +++++++++++++++++++++++++++----------------
object-file.c | 2 +-
pager.c | 21 +++++++-----
prompt.c | 3 +-
remote.c | 2 +-
repository.c | 1 +
12 files changed, 148 insertions(+), 81 deletions(-)