Repository navigation
Conversation
Welcome to GitGitGadgetHi @Joiejoie1, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that either:
You can CC potential reviewers by adding a footer to the PR description with the following syntax: NOTE: DO NOT copy/paste your CC list from a previous GGG PR's description, Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
b06d921 to
8013ea0
Compare
|
There is an issue in commit 8013ea0:
|
8013ea0 to
e3e36c3
Compare
|
/allow |
|
User Joiejoie1 is now allowed to use GitGitGadget. |
|
/preview |
|
Preview email sent as pull.2271.git.1792514748146.gitgitgadget@gmail.com |
|
/preview |
|
Preview email sent as pull.2271.git.1792519549513.gitgitgadget@gmail.com |
|
/preview |
|
Preview email sent as pull.2271.git.1792522164276.gitgitgadget@gmail.com |
|
/submit |
|
Submitted as pull.2271.git.1792522587065.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
"Kristoffer Haugsbakk" wrote on the Git mailing list (how to reply to this email): On Tue, Oct 20, 2026, at 20:56, Joy via GitGitGadget wrote:
> From: Joy Gundu <soniqjjj@gmail.com>
>
> [...]
>
> This change removes one piece of global configuration state from Git
> and helps make Git code easier to reuse as a library component.
It's nice to see a paragraph explaining the
overall project in plain terms. Not just
"we are getting rid of globals" or "this is
libification".
>
> Signed-off-by: Joy Gundu <soniqjjj@gmail.com>
> [...]
Sent from mobile |
|
User |
|
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Tue, Oct 20, 2026 at 06:56:27PM +0000, Joy via GitGitGadget wrote:
> From: Joy Gundu <soniqjjj@gmail.com>
>
> Store the pack.packsizelimit value in repository configuration
> rather than keeping it in a standalone global variable for the repository.
>
> Update the configuration parser and pack-writing code to read
> the value from repo_config_values rather than a global variable.
>
> This change removes one piece of global configuration state from Git
> and helps make Git code easier to reuse as a library component.
The commit message typically starts with an observation, followed by an
explanation what's bad about the observed status quo and then finally
explains how that state is improved. The order you have here does not
match that typical flow.
> diff --git a/environment.c b/environment.c
> index c83cf44839..e350d6949e 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -728,7 +727,7 @@ int git_default_config(const char *var, const char *value,
> }
>
> if (!strcmp(var, "pack.packsizelimit")) {
> - pack_size_limit_cfg = git_config_ulong(var, value, ctx->kvi);
> + cfg->pack_size_limit = git_config_ulong(var, value, ctx->kvi);
> return 0;
> }
>
> @@ -768,6 +767,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
> cfg->check_stat = 1;
> cfg->zlib_compression_level = Z_BEST_SPEED;
> cfg->pack_compression_level = Z_DEFAULT_COMPRESSION;
> + cfg->pack_size_limit = 0;
> cfg->precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
> cfg->core_sparse_checkout_cone = 0;
> cfg->warn_on_object_refname_ambiguity = 1;
Hmm, quite annoying that we need to explicitly zero out all these
fields. Maybe we should adapt `repo_config_values_init()` to first
`memset(cfg, 0, sizeof(*cfg))`? That would allow us to remove a lot of
assignments. If you want to do that, it would be great to do it as a
preparatory patch in the same series.
Other than that the diff looks exactly the same as a local patch I had
already written myself, so thanks for allowing me to worry about one
less thing :)
Patrick |
|
User |
Signed-off-by: Joy Gundu <soniqjjj@gmail.com>
e3e36c3 to
a2d8165
Compare
|
/preview |
1 similar comment
|
/preview |
|
Preview email sent as pull.2271.v2.git.1792596636.gitgitgadget@gmail.com |
|
Preview email sent as pull.2271.v2.git.1792597400.gitgitgadget@gmail.com |
The pack size limit is currently stored in a standalone global variable, even though it is repository configuration. Move the value into repo_config_values and update the configuration parser and pack-writing code to read it from the repository's configuration values. This removes one piece of global configuration state and helps make Git code easier to reuse as a library component. Signed-off-by: Joy Gundu <soniqjjj@gmail.com>
a2d8165 to
05ae50b
Compare
|
/preview |
|
Preview email sent as pull.2271.v2.git.1792598771.gitgitgadget@gmail.com |
|
Joie Joie wrote on the Git mailing list (how to reply to this email): Hi Patrick,
Thanks for the feedback! I’ll take a closer look at zero-initializing
repo_config_values and see if it makes sense to include it as a
preparatory patch.
I’ll also keep your advice on commit message structure in mind for
future patches.
Thanks again for taking the time to review my work!
Best,
Joy
On Wed, Oct 21, 2026 at 9:09 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Tue, Oct 20, 2026 at 06:56:27PM +0000, Joy via GitGitGadget wrote:
> > From: Joy Gundu <soniqjjj@gmail.com>
> >
> > Store the pack.packsizelimit value in repository configuration
> > rather than keeping it in a standalone global variable for the repository.
> >
> > Update the configuration parser and pack-writing code to read
> > the value from repo_config_values rather than a global variable.
> >
> > This change removes one piece of global configuration state from Git
> > and helps make Git code easier to reuse as a library component.
>
> The commit message typically starts with an observation, followed by an
> explanation what's bad about the observed status quo and then finally
> explains how that state is improved. The order you have here does not
> match that typical flow.
>
> > diff --git a/environment.c b/environment.c
> > index c83cf44839..e350d6949e 100644
> > --- a/environment.c
> > +++ b/environment.c
> > @@ -728,7 +727,7 @@ int git_default_config(const char *var, const char *value,
> > }
> >
> > if (!strcmp(var, "pack.packsizelimit")) {
> > - pack_size_limit_cfg = git_config_ulong(var, value, ctx->kvi);
> > + cfg->pack_size_limit = git_config_ulong(var, value, ctx->kvi);
> > return 0;
> > }
> >
> > @@ -768,6 +767,7 @@ void repo_config_values_init(struct repo_config_values *cfg)
> > cfg->check_stat = 1;
> > cfg->zlib_compression_level = Z_BEST_SPEED;
> > cfg->pack_compression_level = Z_DEFAULT_COMPRESSION;
> > + cfg->pack_size_limit = 0;
> > cfg->precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
> > cfg->core_sparse_checkout_cone = 0;
> > cfg->warn_on_object_refname_ambiguity = 1;
>
> Hmm, quite annoying that we need to explicitly zero out all these
> fields. Maybe we should adapt `repo_config_values_init()` to first
> `memset(cfg, 0, sizeof(*cfg))`? That would allow us to remove a lot of
> assignments. If you want to do that, it would be great to do it as a
> preparatory patch in the same series.
>
> Other than that the diff looks exactly the same as a local patch I had
> already written myself, so thanks for allowing me to worry about one
> less thing :)
>
> Patrick |
|
User |
|
The Windows |
|
/submit |
|
Submitted as pull.2271.v2.git.1792662432.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
Yes, sadly some of the Windows jobs in particular "loose connection" and then time out. |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -752,34 +752,24 @@ int git_default_config(const char *var, const char *value, | |||
|
|
|||
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Thu, Oct 22, 2026 at 09:47:11AM +0000, Joy Gundu via GitGitGadget wrote:
> From: Joy Gundu <soniqjjj@gmail.com>
Even though the patch speaks for itself, we typically still like to a
sentence or two in the commit message itself. E.g.:
When initializing `struct repo_config_values` we manually zero out a
lot of its members, which is tedious. Reduce the boilerplate by first
zeroing out the whole structure so that we only have to set those
members that have a non-zero default value.
> Signed-off-by: Joy Gundu <soniqjjj@gmail.com>
> ---
> environment.c | 14 ++------------
> 1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/environment.c b/environment.c
> index c83cf44839..89af09aa40 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -752,34 +752,24 @@ int git_default_config(const char *var, const char *value,
>
> void repo_config_values_init(struct repo_config_values *cfg)
> {
> + memset(cfg, 0, sizeof(*cfg));
> +
> /* section "core" config values */
> - cfg->attributes_file = NULL;
> - cfg->excludes_file = NULL;
> - cfg->editor_program = NULL;
> - cfg->pager_program = NULL;
> - cfg->askpass_program = NULL;
> - cfg->apply_default_whitespace = NULL;
> - cfg->apply_default_ignorewhitespace = NULL;
> cfg->push_default = PUSH_DEFAULT_UNSPECIFIED;
> cfg->autorebase = AUTOREBASE_NEVER;
> cfg->object_creation_mode = OBJECT_CREATION_MODE;
> - cfg->apply_sparse_checkout = 0;
> cfg->trust_ctime = 1;
> cfg->check_stat = 1;
> cfg->zlib_compression_level = Z_BEST_SPEED;
> cfg->pack_compression_level = Z_DEFAULT_COMPRESSION;
> cfg->precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
> - cfg->core_sparse_checkout_cone = 0;
> cfg->warn_on_object_refname_ambiguity = 1;
> cfg->protect_hfs = PROTECT_HFS_DEFAULT;
> cfg->protect_ntfs = PROTECT_NTFS_DEFAULT;
> - cfg->ignore_case = 0;
> cfg->trust_executable_bit = 1;
> cfg->has_symlinks = platform_has_symlinks();
> - cfg->use_nanosec = 0;
>
> /* section "sparse" config values */
> - cfg->sparse_expect_files_outside_of_patterns = 0;
We should probably drop this comment together with the code.
> /* section "branch" config values */
> cfg->branch_track = BRANCH_TRACK_REMOTE;
Other than that this looks good to me.
Patrick
|
User |
| @@ -5370,7 +5370,7 @@ int cmd_pack_objects(int argc, | |||
| if (!HAVE_THREADS && delta_search_threads != 1) | |||
There was a problem hiding this comment.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
On Thu, Oct 22, 2026 at 09:47:12AM +0000, Joy Gundu via GitGitGadget wrote:
> From: Joy Gundu <soniqjjj@gmail.com>
>
> The pack size limit is currently stored in a standalone global
> variable, even though it is repository configuration.
>
> Move the value into repo_config_values and update the configuration
> parser and pack-writing code to read it from the repository's
> configuration values.
>
> This removes one piece of global configuration state and helps
> make Git code easier to reuse as a library component.
This reads well to me, thanks.
Patrick
|
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Thu, Oct 22, 2026 at 09:47:10AM +0000, Joy via GitGitGadget wrote:
> Changes since v1
> ================
>
> Thanks to Patrick Steinhardt for reviewing the first version and suggesting
> the initialization cleanup.
>
> Following his feedback, I've:
>
> * Added a preparatory patch to simplify how repository configuration values
> are initialized.
> * Reworked the main commit message to explain the existing problem before
> describing the change.
> * Removed the need to explicitly initialize the new pack size limit field
> to zero.
I've got two more small nits on the first patch that you probably want
to address, but other than that this looks as expected to me. Thanks!
Patrick |
2 participants
This series moves
pack.packsizelimitfrom a standalone global variable intostruct repo_config_values, as part of the ongoing effort to reduce Git's reliance on global state and make its code easier to reuse as a library.The series includes two patches:
repo_config_values_init()to usememset()for zero-initialization, while keeping the defaults that require explicit values.repo_config_valuesand update the relevant code to read it from there.Changes since v1
Thanks to Patrick Steinhardt for reviewing the first version and suggesting the initialization cleanup.
Following his feedback, I've:
Testing
I built Git and ran the relevant configuration and pack-object tests:
make -j2— passed.t1300-config.sh— 518 successful tests.t5300-pack-object.sh— 63 passed, 2 skipped, 0 failed.Both test runs reported permission errors during cleanup after the tests had completed.
CC: Patrick Steinhardt <ps@pks.im>, Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com>, Christian Couder <christian.couder@gmail.com>, Usman Akinyemi <usmanakinyemi202@gmail.com>
cc: Joie Joie soniqjjj@gmail.com
cc: Patrick Steinhardt ps@pks.im