Skip to contentSkip to conversation

repo: zero-initialize configuration values

PATCH In review Joy via GitGitGadget 4 participants 6 messages Nov 6 – Nov 9

lore
Share & tools
lore

Joy via GitGitGadget

author
#0 lore raw

PATCH 1/1 repo: zero-initialize configuration values

From: Joy Gundu <[email protected]>

Use memset() to initialize repository configuration values to zero, removing the need for explicit zero and NULL assignments. Keep non-zero defaults initialized separately.

Signed-off-by Joy Gundu <[email protected]>

---
[Outreachy] repo: zero-initialize configuration values

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2298%2FJoiejoie1%2Frepo-zero-initialize-config-values-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2298/Joiejoie1/repo-zero-initialize-config-values-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2298

 environment.c | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)
environment.c +2 -14
index c83cf44839..afe35cff99 100644
@@ -752,34 +752,22 @@ 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;
 
 	/* section "branch" config values */
 	cfg->branch_track = BRANCH_TRACK_REMOTE;

base-commit: 3de8ebe6c8cfcd312cdbfa675635810140f84c8a

Signature · 1 line
gitgitgadget
#1 lore raw
Quoted 9 lines · Joy via GitGitGadget

From: Joy Gundu <[email protected]>

Use memset() to initialize repository configuration values to zero, removing the need for explicit zero and NULL assignments. Keep non-zero defaults initialized separately.

Signed-off-by Joy Gundu <[email protected]>

---
[Outreachy] repo: zero-initialize configuration values

So I guess this patch was split out because of our recent push to not make further changes to repo_config_values()? If so, it would have made sense to point that out explicitly both here, and in the original thread that you've sent.

In any case, the patch is obviously correct and is a good improvement in my opinion. So this looks good to me, thanks.

Patrick

#2 lore raw

Thanks, Patrick.

Yes, that was my reason for splitting the patch. It only changes how the existing configuration values are initialized and doesn't add a new struct member, so I felt it could be reviewed separately from the changes affected by the recent direction.

You're right, though. I should have explained that more clearly in the patch description and the original thread. I'll make sure to include that context in future submissions.

Thanks for taking the time to review it!

Quoted 21 lines · Patrick Steinhardt
Quoted 9 lines · Joy via GitGitGadget

From: Joy Gundu <[email protected]>

Use memset() to initialize repository configuration values to zero, removing the need for explicit zero and NULL assignments. Keep non-zero defaults initialized separately.

Signed-off-by Joy Gundu <[email protected]>

---
[Outreachy] repo: zero-initialize configuration values

So I guess this patch was split out because of our recent push to not make further changes to repo_config_values()? If so, it would have made sense to point that out explicitly both here, and in the original thread that you've sent.

In any case, the patch is obviously correct and is a good improvement in my opinion. So this looks good to me, thanks.

Patrick

#3 lore raw
In reply to Patrick Steinhardt
Quoted 2 lines · Patrick Steinhardt

In any case, the patch is obviously correct and is a good improvement in my opinion. So this looks good to me, thanks.

It however does not look good to me. I would prefer to see

	struct foo foo = {0};

if everything is zero initialized, or if there are some members initialized to specific values, C99 designated initializers to be used:

	struct foo foo = {
		/* Comment on this stuff */
		.object_creation_mode = OBJECT_CREATION_MODE,

                /* Comment on this other stuff */
		.branch_track = BRANCH_TRACK_REMOTE,
	};

Now this is overwriting the passed in structure members, so how we write the initialization is not the whole story.

Since we have avoided assuming the universal availability of the more recent "compound literals" syntax, which is:

	*cfg = (struct foo) {
		... an initialization with designated initializers ...
	};

we seem to (ab)use struct assignment, e.g.,

	static struct foo foo_template = {
		... an initialization with designated initializers ...
	};

	*cfg = foo_template;

in a few places as a fallback in the existing code (DI is C99, struct assignment is C89, so it has been a safer assumption than assuming compound literals).

But I do recall we made this update to our CodingGuidelines document.

. since late 2024 with v2.48.0-rc0~20, we have been using compound

     literals, e.g., (struct foo){ .member = value };

So perhaps it is time to switch this to assign a compound literal?

#4 lore raw

Thanks, Junio.

I hadn't considered using a compound literal here. I was following the existing initialization approach, but your suggestion makes sense, especially given the update to the CodingGuidelines.

I'll look into changing the initialization to use a compound literal while preserving the existing defaults and behavior.

Quoted 50 lines · Junio C Hamano
Quoted 2 lines · Patrick Steinhardt

In any case, the patch is obviously correct and is a good improvement in my opinion. So this looks good to me, thanks.

It however does not look good to me. I would prefer to see

        struct foo foo = {0};

if everything is zero initialized, or if there are some members initialized to specific values, C99 designated initializers to be used:

        struct foo foo = {
                /* Comment on this stuff */
                .object_creation_mode = OBJECT_CREATION_MODE,

                /* Comment on this other stuff */
                .branch_track = BRANCH_TRACK_REMOTE,
        };

Now this is overwriting the passed in structure members, so how we write the initialization is not the whole story.

Since we have avoided assuming the universal availability of the more recent "compound literals" syntax, which is:

        *cfg = (struct foo) {
                ... an initialization with designated initializers ...
        };

we seem to (ab)use struct assignment, e.g.,

        static struct foo foo_template = {
                ... an initialization with designated initializers ...
        };

        *cfg = foo_template;

in a few places as a fallback in the existing code (DI is C99, struct assignment is C89, so it has been a safer assumption than assuming compound literals).

But I do recall we made this update to our CodingGuidelines document.

. since late 2024 with v2.48.0-rc0~20, we have been using compound

     literals, e.g., (struct foo){ .member = value };

So perhaps it is time to switch this to assign a compound literal?

#5 lore raw
Quoted 8 lines · Joie Joie

Thanks, Junio.

I hadn't considered using a compound literal here. I was following the existing initialization approach, but your suggestion makes sense, especially given the update to the CodingGuidelines.

I'll look into changing the initialization to use a compound literal while preserving the existing defaults and behavior.

Sounds good, but do not top post ;-)

Quoted 52 lines · Junio C Hamano
Quoted 50 lines
Quoted 2 lines · Patrick Steinhardt

In any case, the patch is obviously correct and is a good improvement in my opinion. So this looks good to me, thanks.

It however does not look good to me. I would prefer to see

        struct foo foo = {0};

if everything is zero initialized, or if there are some members initialized to specific values, C99 designated initializers to be used:

        struct foo foo = {
                /* Comment on this stuff */
                .object_creation_mode = OBJECT_CREATION_MODE,

                /* Comment on this other stuff */
                .branch_track = BRANCH_TRACK_REMOTE,
        };

Now this is overwriting the passed in structure members, so how we write the initialization is not the whole story.

Since we have avoided assuming the universal availability of the more recent "compound literals" syntax, which is:

        *cfg = (struct foo) {
                ... an initialization with designated initializers ...
        };

we seem to (ab)use struct assignment, e.g.,

        static struct foo foo_template = {
                ... an initialization with designated initializers ...
        };

        *cfg = foo_template;

in a few places as a fallback in the existing code (DI is C99, struct assignment is C89, so it has been a safer assumption than assuming compound literals).

But I do recall we made this update to our CodingGuidelines document.

. since late 2024 with v2.48.0-rc0~20, we have been using compound

     literals, e.g., (struct foo){ .member = value };

So perhaps it is time to switch this to assign a compound literal?