* [PATCH 0/6] [RFC] Lazy-loaded default Git config
@ 2023-06-02 14:33 Derrick Stolee via GitGitGadget
2023-06-02 14:33 ` [PATCH 1/6] config: create new global config helpers Derrick Stolee via GitGitGadget
` (6 more replies)
0 siblings, 7 replies; 8+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2023-06-02 14:33 UTC (permalink / raw)
To: git; +Cc: vdye, johannes.schindelin, newren, peff, gitster, Derrick Stolee
While working on the stronger protections around replace refs config [1], I
wanted to think about a way to scale that concept of ensuring that we've
read the config before accessing the global to a wider scope of config keys.
[1]
https://lore.kernel.org/git/pull.1537.v2.git.1685716157.gitgitgadget@gmail.com/
If you wish to apply these patches, then do so on top of v2 of [1].
This RFC attempt to explore this space, to get a feeling whether this is
solving a worthwhile problem and what this system should look like.
One example option is presented here, but I'm not super-confident in it and
only implemented enough translations such that I could gather enough
examples to know this would work at least a little bit.
The basic idea is to create a new 'global-config' library that has enums for
different config keys, and from each enum value we can load the config
string and the stored global value, as well as access an indicator that the
value has been loaded from config yet.
In this way, we can guard accesses to the global state through a method
which will lazy-load the config value as needed. This avoids the kind of bug
where a builtin forgets to initialize config through a git_config() call at
the right time.
One of the issues that becomes particularly important is that some of our
globals are accessed before config is available. Specifically, in patch 6,
we replace the ignore_case global with the method accessor. This is used in
fspathcmp() to toggle between strcasecmp() and strcmp(). However, this
method is called as part of the process to set up the Git directory, which
must happen before we can start loading config. The current behavior is to
always use the default of 0 at this time.
To get around this, I introduce a declare_config_available() method which is
called at the right time in git.c and test-tool.c. Before this is called, we
will silently pass the default value instead of loading from config. Without
this behavior, our test suite would fail in a BUG() statement due to the
inconsistent use of the config library.
My hope with submitting this RFC is that we could come to some conclusions
on these questions:
* Is this a worthwhile category of bug to protect against?
* Is wrapping global state in accessor methods the right way to protect
against that class of bugs?
* Are there other benefits to doing this?
* Are there reasons why this cure is worse than the disease?
There are a few things that I personally find could use improvement and I
would like to update before submitting this for full review. (I chose to not
spend extra time re-working this until there is agreement on the general
direction.) Here are the things I've identified so far:
1. The (static) global state in global-config.c is split across two arrays
and a bitmask. It would be better to have a single array of structs so
we can have the default values described right next to the config key
strings.
2. The current accessor method is limited to returning ints, and is named
as such. But many of the keys it replace are actually boolean-valued.
The current implementation tries ...get_maybe_bool() before then trying
...get_int(), which would extend values like core.filemode to allow
taking the value "3". This could be fixed by using the struct approach
and signalling that some int-typed globals only allow boolean values.
(This holds the same for int-valued things that shouldn't take "true".)
3. The method is currently scoped globally, and does not allow for
repository-scoped values. None of these globals are currently scoped to
repositories, but it would be less intrusive to do repository-scoping
later if the accessor method allowed repository scoping and then looked
at the key-specific settings as to whether we should look globally or in
a repository.
4. The replacement of a constant like 'ignore_case' with a method like
'get_int_config_global(INT_CONFIG_IGNORE_CASE)' is quite verbose and
ugly. I specifically chose this approach instead of dedicated methods
like 'replace_refs_enabled()' in [1] because I didn't think that
approach would scale. However, we could perhaps use macros as a way to
reduce the verbosity, especially when deciding to add repository-scoping
as necessary. For instance, we could define 'get_ignore_case()' to be
'get_int_config_global(the_repository, INT_CONFIG_IGNORE_CASE)' in
global-config.h to avoid spreading this verbosity across the codebase.
Alternatively, we could define these as inline methods if that is
preferred.
Thank you for your help determining the right way forward here.
Thanks, -Stolee
Derrick Stolee (6):
config: create new global config helpers
config: add trust_executable_bit to global config
config: move trust_ctime to global config
config: move quote_path_fully to global config
config: move has_symlinks to global config
config: move ignore_case to global config
Makefile | 1 +
apply.c | 6 ++-
builtin/difftool.c | 4 +-
builtin/mv.c | 2 +-
cache.h | 9 ++++-
combine-diff.c | 2 +-
config.c | 24 -----------
dir.c | 23 ++++++-----
entry.c | 3 +-
environment.c | 4 --
environment.h | 4 --
git.c | 4 ++
global-config.c | 54 +++++++++++++++++++++++++
global-config.h | 30 ++++++++++++++
merge-recursive.c | 11 ++---
name-hash.c | 6 +--
quote.c | 6 +--
quote.h | 2 -
read-cache.c | 22 +++++-----
t/helper/test-lazy-init-name-hash.c | 5 ---
t/helper/test-tool.c | 3 ++
t/perf/p0004-lazy-init-name-hash.sh | 6 +++
t/t3008-ls-files-lazy-init-name-hash.sh | 1 +
unpack-trees.c | 2 +-
24 files changed, 156 insertions(+), 78 deletions(-)
create mode 100644 global-config.c
create mode 100644 global-config.h
base-commit: 4c7dbeb8c6dd6ab4c1903196967d5e0906a293c2
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1539%2Fderrickstolee%2Fdefault-config-safety-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1539/derrickstolee/default-config-safety-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1539
--
gitgitgadget
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/6] config: create new global config helpers
2023-06-02 14:33 [PATCH 0/6] [RFC] Lazy-loaded default Git config Derrick Stolee via GitGitGadget
@ 2023-06-02 14:33 ` Derrick Stolee via GitGitGadget
2023-06-02 14:33 ` [PATCH 2/6] config: add trust_executable_bit to global config Derrick Stolee via GitGitGadget
` (5 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2023-06-02 14:33 UTC (permalink / raw)
To: git
Cc: vdye, johannes.schindelin, newren, peff, gitster, Derrick Stolee,
Derrick Stolee
From: Derrick Stolee <derrickstolee@github.com>
Git's default config is loaded by each builtin at some point during its
cmd_*() method. If this is forgotten, then Git can behave strangely
compared to user expectations.
To avoid this kind of bug, create a new system for loading common
"global" config (config not currently scoped to a repository struct).
The basic idea is that we will add config values one-by-one to the
int_config_key enum (and non-int values can follow a similar pattern
later).
To access the value, a consumer calls get_int_config_global() with the
appropriate enum value. This method will check if the config has been
loaded already (using the 64-bit-for-now global_ints_initialized
bitmask) and decide whether to use the stored value or to load a value
from config.
While this method is not currently used by any consumer, there are cases
where some of our default config settings are looked up by global
variable before Git has a chance to load config. This cannot be helped,
due to paths not being initialized at that point in time. We could
remove the dependencies on those values, but it would require changing
things in some difficult ways or lead to unnecessary duplication across
methods.
For now, create the declare_config_available() method, which is called
in cmd_main() when it is appropriate to "unlock" looking up these values
from config. Before this method is called, get_int_config_global() will
return the default value.
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
Makefile | 1 +
git.c | 4 ++++
global-config.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
global-config.h | 25 +++++++++++++++++++++++++
t/helper/test-tool.c | 3 +++
5 files changed, 77 insertions(+)
create mode 100644 global-config.c
create mode 100644 global-config.h
diff --git a/Makefile b/Makefile
index e440728c246..d088589d818 100644
--- a/Makefile
+++ b/Makefile
@@ -1033,6 +1033,7 @@ LIB_OBJS += fsmonitor-ipc.o
LIB_OBJS += fsmonitor-settings.o
LIB_OBJS += gettext.o
LIB_OBJS += git-zlib.o
+LIB_OBJS += global-config.o
LIB_OBJS += gpg-interface.o
LIB_OBJS += graph.o
LIB_OBJS += grep.o
diff --git a/git.c b/git.c
index 3252d4c7661..72632e659d0 100644
--- a/git.c
+++ b/git.c
@@ -12,6 +12,7 @@
#include "shallow.h"
#include "trace.h"
#include "trace2.h"
+#include "global-config.h"
#define RUN_SETUP (1<<0)
#define RUN_SETUP_GENTLY (1<<1)
@@ -443,6 +444,9 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
if (!help && p->option & NEED_WORK_TREE)
setup_work_tree();
+ /* At this point, we can allow loading config. */
+ declare_config_available();
+
trace_argv_printf(argv, "trace: built-in: git");
trace2_cmd_name(p->cmd);
trace2_cmd_list_config();
diff --git a/global-config.c b/global-config.c
new file mode 100644
index 00000000000..db9643afd7a
--- /dev/null
+++ b/global-config.c
@@ -0,0 +1,44 @@
+#include "git-compat-util.h"
+#include "global-config.h"
+#include "config.h"
+
+static int global_ints[] = {
+ [INT_CONFIG_NONE] = 0, /* unused*/
+};
+
+/* Bitmask for the enum. */
+static uint64_t global_ints_initialized;
+
+static const char *global_int_names[] = {
+ [INT_CONFIG_NONE] = NULL, /* unused*/
+};
+
+static int config_available;
+
+void declare_config_available(void)
+{
+ config_available = 1;
+}
+
+int get_int_config_global(enum int_config_key key)
+{
+ uint64_t key_index;
+
+ if (key < 0 || key >= sizeof(global_ints))
+ BUG("invalid int_config_key %d", key);
+
+ key_index = (uint64_t)1 << key;
+
+ /*
+ * Is it too early to load from config?
+ * Have we already loaded from config?
+ */
+ if (!config_available || (global_ints_initialized & key_index))
+ return global_ints[key];
+ global_ints_initialized |= key_index;
+
+ /* Try getting a boolean value before trying an int. */
+ if (git_config_get_maybe_bool(global_int_names[key], &global_ints[key]) < 0)
+ git_config_get_int(global_int_names[key], &global_ints[key]);
+ return global_ints[key];
+}
diff --git a/global-config.h b/global-config.h
new file mode 100644
index 00000000000..407dff19ee9
--- /dev/null
+++ b/global-config.h
@@ -0,0 +1,25 @@
+#ifndef GLOBAL_CONFIG_H
+#define GLOBAL_CONFIG_H
+
+enum int_config_key {
+ INT_CONFIG_NONE = 0,
+};
+
+/**
+ * During initial process loading, the config system is not quite available.
+ * The config global system needs an indicator that the process is ready
+ * to read config. Before this method is called, it will return the
+ * default values.
+ */
+void declare_config_available(void);
+
+/**
+ * Given a config key (by enum), return its value.
+ *
+ * If declare_config_available() has not been called, then this returns
+ * the default value. Otherwise, it guarantees that the value has been
+ * filled from config before returning the value.
+ */
+int get_int_config_global(enum int_config_key key);
+
+#endif /* GLOBAL_CONFIG_H */
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index abe8a785eb6..36340d36307 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -3,6 +3,7 @@
#include "test-tool-utils.h"
#include "trace2.h"
#include "parse-options.h"
+#include "global-config.h"
static const char * const test_tool_usage[] = {
"test-tool [-C <directory>] <command [<arguments>...]]",
@@ -127,6 +128,8 @@ int cmd_main(int argc, const char **argv)
if (working_directory && chdir(working_directory) < 0)
die("Could not cd to '%s'", working_directory);
+ declare_config_available();
+
for (i = 0; i < ARRAY_SIZE(cmds); i++) {
if (!strcmp(cmds[i].name, argv[1])) {
argv++;
--
gitgitgadget
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/6] config: add trust_executable_bit to global config
2023-06-02 14:33 [PATCH 0/6] [RFC] Lazy-loaded default Git config Derrick Stolee via GitGitGadget
2023-06-02 14:33 ` [PATCH 1/6] config: create new global config helpers Derrick Stolee via GitGitGadget
@ 2023-06-02 14:33 ` Derrick Stolee via GitGitGadget
2023-06-02 14:33 ` [PATCH 3/6] config: move trust_ctime " Derrick Stolee via GitGitGadget
` (4 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2023-06-02 14:33 UTC (permalink / raw)
To: git
Cc: vdye, johannes.schindelin, newren, peff, gitster, Derrick Stolee,
Derrick Stolee
From: Derrick Stolee <derrickstolee@github.com>
The 'trust_executable_bit' global represents the core.filemode config
value. It is loaded when git_default_config() is used to iterate over
the config keys (which includes several other similar iterators) during
the start of a Git process. The timing of this load depends on each
process, and happens even if the global is never used.
Instead, use the new global config system to load it on-demand, if
needed. Since this is the first use of the system, let's describe the
process:
1. Create an enum value representing the value. This is named based on
the fact that we expect an 'int' type.
2. Update the default to 1 to match the existing global.
3. Update the config name so we can load it from the config set.
4. Replace uses of the global with get_int_config_global().
5. Remove the global from environment.(c|h).
The "This needs a better name" comment has been around since 17712991a59
(Add ".git/config" file parser, 2005-10-10), and in that original
context it clearly mentions "core.filemode" as the bad name, not the
containing method. Now that we are removing this global assignment (and
not changing the name any time soon) we delete the comment, too.
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
cache.h | 7 ++++++-
config.c | 5 -----
environment.c | 1 -
environment.h | 1 -
global-config.c | 2 ++
global-config.h | 1 +
read-cache.c | 5 +++--
7 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/cache.h b/cache.h
index 71e2fe74c4f..3e737d12ea8 100644
--- a/cache.h
+++ b/cache.h
@@ -9,6 +9,7 @@
#include "pathspec.h"
#include "object.h"
#include "statinfo.h"
+#include "global-config.h"
#if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)
#define DTYPE(de) ((de)->d_type)
@@ -160,10 +161,14 @@ static inline unsigned create_ce_flags(unsigned stage)
static inline unsigned int ce_mode_from_stat(const struct cache_entry *ce,
unsigned int mode)
{
- extern int trust_executable_bit, has_symlinks;
+ extern int has_symlinks;
+ int trust_executable_bit;
+
if (!has_symlinks && S_ISREG(mode) &&
ce && S_ISLNK(ce->ce_mode))
return ce->ce_mode;
+
+ trust_executable_bit = get_int_config_global(INT_CONFIG_TRUST_EXECUTABLE_BIT);
if (!trust_executable_bit && S_ISREG(mode)) {
if (ce && S_ISREG(ce->ce_mode))
return ce->ce_mode;
diff --git a/config.c b/config.c
index d0ce902af39..ffe6bea80a6 100644
--- a/config.c
+++ b/config.c
@@ -1561,11 +1561,6 @@ int git_config_color(char *dest, const char *var, const char *value)
static int git_default_core_config(const char *var, const char *value, void *cb)
{
- /* This needs a better name */
- if (!strcmp(var, "core.filemode")) {
- trust_executable_bit = git_config_bool(var, value);
- return 0;
- }
if (!strcmp(var, "core.trustctime")) {
trust_ctime = git_config_bool(var, value);
return 0;
diff --git a/environment.c b/environment.c
index e198b48081a..890705f0f69 100644
--- a/environment.c
+++ b/environment.c
@@ -30,7 +30,6 @@
#include "wrapper.h"
#include "write-or-die.h"
-int trust_executable_bit = 1;
int trust_ctime = 1;
int check_stat = 1;
int has_symlinks = 1;
diff --git a/environment.h b/environment.h
index a63f0c6a24f..66d88612085 100644
--- a/environment.h
+++ b/environment.h
@@ -111,7 +111,6 @@ void set_git_work_tree(const char *tree);
#define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
/* Environment bits from configuration mechanism */
-extern int trust_executable_bit;
extern int trust_ctime;
extern int check_stat;
extern int has_symlinks;
diff --git a/global-config.c b/global-config.c
index db9643afd7a..b6fd16b14f6 100644
--- a/global-config.c
+++ b/global-config.c
@@ -4,6 +4,7 @@
static int global_ints[] = {
[INT_CONFIG_NONE] = 0, /* unused*/
+ [INT_CONFIG_TRUST_EXECUTABLE_BIT] = 1,
};
/* Bitmask for the enum. */
@@ -11,6 +12,7 @@ static uint64_t global_ints_initialized;
static const char *global_int_names[] = {
[INT_CONFIG_NONE] = NULL, /* unused*/
+ [INT_CONFIG_TRUST_EXECUTABLE_BIT] = "core.filemode",
};
static int config_available;
diff --git a/global-config.h b/global-config.h
index 407dff19ee9..e66c3f4ac77 100644
--- a/global-config.h
+++ b/global-config.h
@@ -3,6 +3,7 @@
enum int_config_key {
INT_CONFIG_NONE = 0,
+ INT_CONFIG_TRUST_EXECUTABLE_BIT,
};
/**
diff --git a/read-cache.c b/read-cache.c
index e919af3c77a..8d14a029ce4 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -336,7 +336,7 @@ static int ce_match_stat_basic(const struct cache_entry *ce, struct stat *st)
/* We consider only the owner x bit to be relevant for
* "mode changes"
*/
- if (trust_executable_bit &&
+ if (get_int_config_global(INT_CONFIG_TRUST_EXECUTABLE_BIT) &&
(0100 & (ce->ce_mode ^ st->st_mode)))
changed |= MODE_CHANGED;
break;
@@ -806,7 +806,8 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
ce->ce_flags |= CE_INTENT_TO_ADD;
- if (trust_executable_bit && has_symlinks) {
+ if (get_int_config_global(INT_CONFIG_TRUST_EXECUTABLE_BIT) &&
+ has_symlinks) {
ce->ce_mode = create_ce_mode(st_mode);
} else {
/* If there is an existing entry, pick the mode bits and type
--
gitgitgadget
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/6] config: move trust_ctime to global config
2023-06-02 14:33 [PATCH 0/6] [RFC] Lazy-loaded default Git config Derrick Stolee via GitGitGadget
2023-06-02 14:33 ` [PATCH 1/6] config: create new global config helpers Derrick Stolee via GitGitGadget
2023-06-02 14:33 ` [PATCH 2/6] config: add trust_executable_bit to global config Derrick Stolee via GitGitGadget
@ 2023-06-02 14:33 ` Derrick Stolee via GitGitGadget
2023-06-02 14:33 ` [PATCH 4/6] config: move quote_path_fully " Derrick Stolee via GitGitGadget
` (3 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2023-06-02 14:33 UTC (permalink / raw)
To: git
Cc: vdye, johannes.schindelin, newren, peff, gitster, Derrick Stolee,
Derrick Stolee
From: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
config.c | 4 ----
environment.c | 1 -
environment.h | 1 -
global-config.c | 2 ++
global-config.h | 1 +
read-cache.c | 6 ++++--
6 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/config.c b/config.c
index ffe6bea80a6..d955a16b372 100644
--- a/config.c
+++ b/config.c
@@ -1561,10 +1561,6 @@ int git_config_color(char *dest, const char *var, const char *value)
static int git_default_core_config(const char *var, const char *value, void *cb)
{
- if (!strcmp(var, "core.trustctime")) {
- trust_ctime = git_config_bool(var, value);
- return 0;
- }
if (!strcmp(var, "core.checkstat")) {
if (!strcasecmp(value, "default"))
check_stat = 1;
diff --git a/environment.c b/environment.c
index 890705f0f69..f42f79f7f1b 100644
--- a/environment.c
+++ b/environment.c
@@ -30,7 +30,6 @@
#include "wrapper.h"
#include "write-or-die.h"
-int trust_ctime = 1;
int check_stat = 1;
int has_symlinks = 1;
int minimum_abbrev = 4, default_abbrev = -1;
diff --git a/environment.h b/environment.h
index 66d88612085..362681f63e7 100644
--- a/environment.h
+++ b/environment.h
@@ -111,7 +111,6 @@ void set_git_work_tree(const char *tree);
#define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
/* Environment bits from configuration mechanism */
-extern int trust_ctime;
extern int check_stat;
extern int has_symlinks;
extern int minimum_abbrev, default_abbrev;
diff --git a/global-config.c b/global-config.c
index b6fd16b14f6..5bd6bc45418 100644
--- a/global-config.c
+++ b/global-config.c
@@ -5,6 +5,7 @@
static int global_ints[] = {
[INT_CONFIG_NONE] = 0, /* unused*/
[INT_CONFIG_TRUST_EXECUTABLE_BIT] = 1,
+ [INT_CONFIG_TRUST_CTIME] = 1,
};
/* Bitmask for the enum. */
@@ -13,6 +14,7 @@ static uint64_t global_ints_initialized;
static const char *global_int_names[] = {
[INT_CONFIG_NONE] = NULL, /* unused*/
[INT_CONFIG_TRUST_EXECUTABLE_BIT] = "core.filemode",
+ [INT_CONFIG_TRUST_CTIME] = "core.trustctime",
};
static int config_available;
diff --git a/global-config.h b/global-config.h
index e66c3f4ac77..a6d8e5adc4e 100644
--- a/global-config.h
+++ b/global-config.h
@@ -4,6 +4,7 @@
enum int_config_key {
INT_CONFIG_NONE = 0,
INT_CONFIG_TRUST_EXECUTABLE_BIT,
+ INT_CONFIG_TRUST_CTIME,
};
/**
diff --git a/read-cache.c b/read-cache.c
index 8d14a029ce4..9e5a6ba2c76 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -192,14 +192,16 @@ int match_stat_data(const struct stat_data *sd, struct stat *st)
if (sd->sd_mtime.sec != (unsigned int)st->st_mtime)
changed |= MTIME_CHANGED;
- if (trust_ctime && check_stat &&
+ if (get_int_config_global(INT_CONFIG_TRUST_CTIME) &&
+ check_stat &&
sd->sd_ctime.sec != (unsigned int)st->st_ctime)
changed |= CTIME_CHANGED;
#ifdef USE_NSEC
if (check_stat && sd->sd_mtime.nsec != ST_MTIME_NSEC(*st))
changed |= MTIME_CHANGED;
- if (trust_ctime && check_stat &&
+ if (get_int_config_global(INT_CONFIG_TRUST_CTIME) &&
+ check_stat &&
sd->sd_ctime.nsec != ST_CTIME_NSEC(*st))
changed |= CTIME_CHANGED;
#endif
--
gitgitgadget
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 4/6] config: move quote_path_fully to global config
2023-06-02 14:33 [PATCH 0/6] [RFC] Lazy-loaded default Git config Derrick Stolee via GitGitGadget
` (2 preceding siblings ...)
2023-06-02 14:33 ` [PATCH 3/6] config: move trust_ctime " Derrick Stolee via GitGitGadget
@ 2023-06-02 14:33 ` Derrick Stolee via GitGitGadget
2023-06-02 14:33 ` [PATCH 5/6] config: move has_symlinks " Derrick Stolee via GitGitGadget
` (2 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2023-06-02 14:33 UTC (permalink / raw)
To: git
Cc: vdye, johannes.schindelin, newren, peff, gitster, Derrick Stolee,
Derrick Stolee
From: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
config.c | 5 -----
global-config.c | 2 ++
global-config.h | 1 +
quote.c | 6 +++---
quote.h | 2 --
5 files changed, 6 insertions(+), 10 deletions(-)
diff --git a/config.c b/config.c
index d955a16b372..6b4051a4eae 100644
--- a/config.c
+++ b/config.c
@@ -1568,11 +1568,6 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
check_stat = 0;
}
- if (!strcmp(var, "core.quotepath")) {
- quote_path_fully = git_config_bool(var, value);
- return 0;
- }
-
if (!strcmp(var, "core.symlinks")) {
has_symlinks = git_config_bool(var, value);
return 0;
diff --git a/global-config.c b/global-config.c
index 5bd6bc45418..395d21e0381 100644
--- a/global-config.c
+++ b/global-config.c
@@ -6,6 +6,7 @@ static int global_ints[] = {
[INT_CONFIG_NONE] = 0, /* unused*/
[INT_CONFIG_TRUST_EXECUTABLE_BIT] = 1,
[INT_CONFIG_TRUST_CTIME] = 1,
+ [INT_CONFIG_QUOTE_PATH_FULLY] = 1,
};
/* Bitmask for the enum. */
@@ -15,6 +16,7 @@ static const char *global_int_names[] = {
[INT_CONFIG_NONE] = NULL, /* unused*/
[INT_CONFIG_TRUST_EXECUTABLE_BIT] = "core.filemode",
[INT_CONFIG_TRUST_CTIME] = "core.trustctime",
+ [INT_CONFIG_QUOTE_PATH_FULLY] = "core.quotepath",
};
static int config_available;
diff --git a/global-config.h b/global-config.h
index a6d8e5adc4e..fbe5fccb1a1 100644
--- a/global-config.h
+++ b/global-config.h
@@ -5,6 +5,7 @@ enum int_config_key {
INT_CONFIG_NONE = 0,
INT_CONFIG_TRUST_EXECUTABLE_BIT,
INT_CONFIG_TRUST_CTIME,
+ INT_CONFIG_QUOTE_PATH_FULLY,
};
/**
diff --git a/quote.c b/quote.c
index 43c739671ed..25ca0473881 100644
--- a/quote.c
+++ b/quote.c
@@ -4,8 +4,7 @@
#include "quote.h"
#include "strbuf.h"
#include "strvec.h"
-
-int quote_path_fully = 1;
+#include "global-config.h"
static inline int need_bs_quote(char c)
{
@@ -212,7 +211,7 @@ int sq_dequote_to_strvec(char *arg, struct strvec *array)
}
/* 1 means: quote as octal
- * 0 means: quote as octal if (quote_path_fully)
+ * 0 means: quote as octal if core.quotePath is true
* -1 means: never quote
* c: quote as "\\c"
*/
@@ -233,6 +232,7 @@ static signed char const cq_lookup[256] = {
static inline int cq_must_quote(char c)
{
+ int quote_path_fully = get_int_config_global(INT_CONFIG_QUOTE_PATH_FULLY);
return cq_lookup[(unsigned char)c] + quote_path_fully > 0;
}
diff --git a/quote.h b/quote.h
index 0300c291041..87ff458b06d 100644
--- a/quote.h
+++ b/quote.h
@@ -3,8 +3,6 @@
struct strbuf;
-extern int quote_path_fully;
-
/* Help to copy the thing properly quoted for the shell safety.
* any single quote is replaced with '\'', any exclamation point
* is replaced with '\!', and the whole thing is enclosed in a
--
gitgitgadget
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 5/6] config: move has_symlinks to global config
2023-06-02 14:33 [PATCH 0/6] [RFC] Lazy-loaded default Git config Derrick Stolee via GitGitGadget
` (3 preceding siblings ...)
2023-06-02 14:33 ` [PATCH 4/6] config: move quote_path_fully " Derrick Stolee via GitGitGadget
@ 2023-06-02 14:33 ` Derrick Stolee via GitGitGadget
2023-06-02 14:33 ` [PATCH 6/6] config: move ignore_case " Derrick Stolee via GitGitGadget
2023-06-08 18:19 ` [PATCH 0/6] [RFC] Lazy-loaded default Git config Glen Choo
6 siblings, 0 replies; 8+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2023-06-02 14:33 UTC (permalink / raw)
To: git
Cc: vdye, johannes.schindelin, newren, peff, gitster, Derrick Stolee,
Derrick Stolee
From: Derrick Stolee <derrickstolee@github.com>
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
apply.c | 3 ++-
builtin/difftool.c | 4 ++--
cache.h | 4 ++--
combine-diff.c | 2 +-
config.c | 5 -----
entry.c | 3 ++-
environment.c | 1 -
environment.h | 1 -
global-config.c | 2 ++
global-config.h | 1 +
merge-recursive.c | 3 ++-
read-cache.c | 5 +++--
12 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/apply.c b/apply.c
index 3636bc14c2d..17092d2bb12 100644
--- a/apply.c
+++ b/apply.c
@@ -4384,7 +4384,8 @@ static int try_create_file(struct apply_state *state, const char *path,
return !!mkdir(path, 0777);
}
- if (has_symlinks && S_ISLNK(mode))
+ if (get_int_config_global(INT_CONFIG_HAS_SYMLINKS) &&
+ S_ISLNK(mode))
/* Although buf:size is counted string, it also is NUL
* terminated.
*/
diff --git a/builtin/difftool.c b/builtin/difftool.c
index f09d24d37f9..f51c9f71d5d 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -292,7 +292,7 @@ static char *get_symlink(const struct object_id *oid, const char *path)
if (is_null_oid(oid)) {
/* The symlink is unknown to Git so read from the filesystem */
struct strbuf link = STRBUF_INIT;
- if (has_symlinks) {
+ if (get_int_config_global(INT_CONFIG_HAS_SYMLINKS)) {
if (strbuf_readlink(&link, path, strlen(path)))
die(_("could not read symlink %s"), path);
} else if (strbuf_read_file(&link, path, 128))
@@ -723,7 +723,7 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
struct child_process child = CHILD_PROCESS_INIT;
git_config(difftool_config, NULL);
- symlinks = has_symlinks;
+ symlinks = get_int_config_global(INT_CONFIG_HAS_SYMLINKS);
argc = parse_options(argc, argv, prefix, builtin_difftool_options,
builtin_difftool_usage, PARSE_OPT_KEEP_UNKNOWN_OPT |
diff --git a/cache.h b/cache.h
index 3e737d12ea8..4d270e114a2 100644
--- a/cache.h
+++ b/cache.h
@@ -161,10 +161,10 @@ static inline unsigned create_ce_flags(unsigned stage)
static inline unsigned int ce_mode_from_stat(const struct cache_entry *ce,
unsigned int mode)
{
- extern int has_symlinks;
int trust_executable_bit;
- if (!has_symlinks && S_ISREG(mode) &&
+ if (!get_int_config_global(INT_CONFIG_HAS_SYMLINKS) &&
+ S_ISREG(mode) &&
ce && S_ISLNK(ce->ce_mode))
return ce->ce_mode;
diff --git a/combine-diff.c b/combine-diff.c
index f7e9fb57473..1de0a340b24 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1086,7 +1086,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
/* if symlinks don't work, assume symlink if all parents
* are symlinks
*/
- is_file = has_symlinks;
+ is_file = get_int_config_global(INT_CONFIG_HAS_SYMLINKS);
for (i = 0; !is_file && i < num_parent; i++)
is_file = !S_ISLNK(elem->parent[i].mode);
if (!is_file)
diff --git a/config.c b/config.c
index 6b4051a4eae..e104bc704ae 100644
--- a/config.c
+++ b/config.c
@@ -1568,11 +1568,6 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
check_stat = 0;
}
- if (!strcmp(var, "core.symlinks")) {
- has_symlinks = git_config_bool(var, value);
- return 0;
- }
-
if (!strcmp(var, "core.ignorecase")) {
ignore_case = git_config_bool(var, value);
return 0;
diff --git a/entry.c b/entry.c
index d89e61fa641..0c33aeb8aa9 100644
--- a/entry.c
+++ b/entry.c
@@ -308,7 +308,8 @@ static int write_entry(struct cache_entry *ce, char *path, struct conv_attrs *ca
* We can't make a real symlink; write out a regular file entry
* with the symlink destination as its contents.
*/
- if (!has_symlinks || to_tempfile)
+ if (!get_int_config_global(INT_CONFIG_HAS_SYMLINKS) ||
+ to_tempfile)
goto write_file_entry;
ret = symlink(new_blob, path);
diff --git a/environment.c b/environment.c
index f42f79f7f1b..312e006feb0 100644
--- a/environment.c
+++ b/environment.c
@@ -31,7 +31,6 @@
#include "write-or-die.h"
int check_stat = 1;
-int has_symlinks = 1;
int minimum_abbrev = 4, default_abbrev = -1;
int ignore_case;
int assume_unchanged;
diff --git a/environment.h b/environment.h
index 362681f63e7..fb2cfa9c1aa 100644
--- a/environment.h
+++ b/environment.h
@@ -112,7 +112,6 @@ void set_git_work_tree(const char *tree);
/* Environment bits from configuration mechanism */
extern int check_stat;
-extern int has_symlinks;
extern int minimum_abbrev, default_abbrev;
extern int ignore_case;
extern int assume_unchanged;
diff --git a/global-config.c b/global-config.c
index 395d21e0381..526ced5b24c 100644
--- a/global-config.c
+++ b/global-config.c
@@ -7,6 +7,7 @@ static int global_ints[] = {
[INT_CONFIG_TRUST_EXECUTABLE_BIT] = 1,
[INT_CONFIG_TRUST_CTIME] = 1,
[INT_CONFIG_QUOTE_PATH_FULLY] = 1,
+ [INT_CONFIG_HAS_SYMLINKS] = 1,
};
/* Bitmask for the enum. */
@@ -17,6 +18,7 @@ static const char *global_int_names[] = {
[INT_CONFIG_TRUST_EXECUTABLE_BIT] = "core.filemode",
[INT_CONFIG_TRUST_CTIME] = "core.trustctime",
[INT_CONFIG_QUOTE_PATH_FULLY] = "core.quotepath",
+ [INT_CONFIG_HAS_SYMLINKS] = "core.symlinks",
};
static int config_available;
diff --git a/global-config.h b/global-config.h
index fbe5fccb1a1..2532f426e2b 100644
--- a/global-config.h
+++ b/global-config.h
@@ -6,6 +6,7 @@ enum int_config_key {
INT_CONFIG_TRUST_EXECUTABLE_BIT,
INT_CONFIG_TRUST_CTIME,
INT_CONFIG_QUOTE_PATH_FULLY,
+ INT_CONFIG_HAS_SYMLINKS,
};
/**
diff --git a/merge-recursive.c b/merge-recursive.c
index 9875bdb11cb..10fdd14a642 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -983,7 +983,8 @@ static int update_file_flags(struct merge_options *opt,
goto free_buf;
}
if (S_ISREG(contents->mode) ||
- (!has_symlinks && S_ISLNK(contents->mode))) {
+ (!get_int_config_global(INT_CONFIG_HAS_SYMLINKS) &&
+ S_ISLNK(contents->mode))) {
int fd;
int mode = (contents->mode & 0100 ? 0777 : 0666);
diff --git a/read-cache.c b/read-cache.c
index 9e5a6ba2c76..b80a54133f9 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -344,7 +344,8 @@ static int ce_match_stat_basic(const struct cache_entry *ce, struct stat *st)
break;
case S_IFLNK:
if (!S_ISLNK(st->st_mode) &&
- (has_symlinks || !S_ISREG(st->st_mode)))
+ (get_int_config_global(INT_CONFIG_HAS_SYMLINKS) ||
+ !S_ISREG(st->st_mode)))
changed |= TYPE_CHANGED;
break;
case S_IFGITLINK:
@@ -809,7 +810,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
if (get_int_config_global(INT_CONFIG_TRUST_EXECUTABLE_BIT) &&
- has_symlinks) {
+ get_int_config_global(INT_CONFIG_HAS_SYMLINKS)) {
ce->ce_mode = create_ce_mode(st_mode);
} else {
/* If there is an existing entry, pick the mode bits and type
--
gitgitgadget
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 6/6] config: move ignore_case to global config
2023-06-02 14:33 [PATCH 0/6] [RFC] Lazy-loaded default Git config Derrick Stolee via GitGitGadget
` (4 preceding siblings ...)
2023-06-02 14:33 ` [PATCH 5/6] config: move has_symlinks " Derrick Stolee via GitGitGadget
@ 2023-06-02 14:33 ` Derrick Stolee via GitGitGadget
2023-06-08 18:19 ` [PATCH 0/6] [RFC] Lazy-loaded default Git config Glen Choo
6 siblings, 0 replies; 8+ messages in thread
From: Derrick Stolee via GitGitGadget @ 2023-06-02 14:33 UTC (permalink / raw)
To: git
Cc: vdye, johannes.schindelin, newren, peff, gitster, Derrick Stolee,
Derrick Stolee
From: Derrick Stolee <derrickstolee@github.com>
The core.ignoreCase config setting is currently stored in the
ignore_case global. Add it to the list of config globals as
INT_CONFIG_IGNORE_CASE instead as a precaution. This allows us to load
the config only when needed.
There is a subtle use of force-enabling this global in 'test-tool
init-name-hash', since it is trying to test a feature that is only on
when core.ignoreCase is true. Instead of forcing it on in-memory, adjust
the test scripts to set the config.
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
apply.c | 3 ++-
builtin/mv.c | 2 +-
config.c | 5 -----
dir.c | 23 ++++++++++++++---------
environment.c | 1 -
environment.h | 1 -
global-config.c | 2 ++
global-config.h | 1 +
merge-recursive.c | 8 ++++----
name-hash.c | 6 +++---
read-cache.c | 8 ++++----
t/helper/test-lazy-init-name-hash.c | 5 -----
t/perf/p0004-lazy-init-name-hash.sh | 6 ++++++
t/t3008-ls-files-lazy-init-name-hash.sh | 1 +
unpack-trees.c | 2 +-
15 files changed, 39 insertions(+), 35 deletions(-)
diff --git a/apply.c b/apply.c
index 17092d2bb12..4292d595062 100644
--- a/apply.c
+++ b/apply.c
@@ -3882,7 +3882,8 @@ static int path_is_beyond_symlink_1(struct apply_state *state, struct strbuf *na
struct cache_entry *ce;
ce = index_file_exists(state->repo->index, name->buf,
- name->len, ignore_case);
+ name->len,
+ get_int_config_global(INT_CONFIG_IGNORE_CASE));
if (ce && S_ISLNK(ce->ce_mode))
return 1;
} else {
diff --git a/builtin/mv.c b/builtin/mv.c
index 32935af48e6..e7a0280ad34 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -359,7 +359,7 @@ dir_check:
goto act_on_entry;
}
if (lstat(dst, &st) == 0 &&
- (!ignore_case || strcasecmp(src, dst))) {
+ (!get_int_config_global(INT_CONFIG_IGNORE_CASE) || strcasecmp(src, dst))) {
bad = _("destination exists");
if (force) {
/*
diff --git a/config.c b/config.c
index e104bc704ae..4292a3be416 100644
--- a/config.c
+++ b/config.c
@@ -1568,11 +1568,6 @@ static int git_default_core_config(const char *var, const char *value, void *cb)
check_stat = 0;
}
- if (!strcmp(var, "core.ignorecase")) {
- ignore_case = git_config_bool(var, value);
- return 0;
- }
-
if (!strcmp(var, "core.attributesfile"))
return git_config_pathname(&git_attributes_file, var, value);
diff --git a/dir.c b/dir.c
index aa840995c40..45f09bcbe4d 100644
--- a/dir.c
+++ b/dir.c
@@ -84,7 +84,8 @@ int count_slashes(const char *s)
int fspathcmp(const char *a, const char *b)
{
- return ignore_case ? strcasecmp(a, b) : strcmp(a, b);
+ return get_int_config_global(INT_CONFIG_IGNORE_CASE) ?
+ strcasecmp(a, b) : strcmp(a, b);
}
int fspatheq(const char *a, const char *b)
@@ -94,12 +95,14 @@ int fspatheq(const char *a, const char *b)
int fspathncmp(const char *a, const char *b, size_t count)
{
- return ignore_case ? strncasecmp(a, b, count) : strncmp(a, b, count);
+ return get_int_config_global(INT_CONFIG_IGNORE_CASE) ?
+ strncasecmp(a, b, count) : strncmp(a, b, count);
}
unsigned int fspathhash(const char *str)
{
- return ignore_case ? strihash(str) : strhash(str);
+ return get_int_config_global(INT_CONFIG_IGNORE_CASE) ?
+ strihash(str) : strhash(str);
}
int git_fnmatch(const struct pathspec_item *item,
@@ -148,7 +151,7 @@ static int fnmatch_icase_mem(const char *pattern, int patternlen,
use_str = str_buf.buf;
}
- if (ignore_case)
+ if (get_int_config_global(INT_CONFIG_IGNORE_CASE))
flags |= WM_CASEFOLD;
match_status = wildmatch(use_pat, use_str, flags);
@@ -1749,7 +1752,8 @@ static struct dir_entry *dir_add_name(struct dir_struct *dir,
struct index_state *istate,
const char *pathname, int len)
{
- if (index_file_exists(istate, pathname, len, ignore_case))
+ if (index_file_exists(istate, pathname, len,
+ get_int_config_global(INT_CONFIG_IGNORE_CASE)))
return NULL;
ALLOC_GROW(dir->entries, dir->nr+1, dir->internal.alloc);
@@ -1786,7 +1790,8 @@ static enum exist_status directory_exists_in_index_icase(struct index_state *ist
if (index_dir_exists(istate, dirname, len))
return index_directory;
- ce = index_file_exists(istate, dirname, len, ignore_case);
+ ce = index_file_exists(istate, dirname, len,
+ get_int_config_global(INT_CONFIG_IGNORE_CASE));
if (ce && S_ISGITLINK(ce->ce_mode))
return index_gitdir;
@@ -1805,7 +1810,7 @@ static enum exist_status directory_exists_in_index(struct index_state *istate,
{
int pos;
- if (ignore_case)
+ if (get_int_config_global(INT_CONFIG_IGNORE_CASE))
return directory_exists_in_index_icase(istate, dirname, len);
pos = index_name_pos(istate, dirname, len);
@@ -2313,7 +2318,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,
/* Always exclude indexed files */
has_path_in_index = !!index_file_exists(istate, path->buf, path->len,
- ignore_case);
+ get_int_config_global(INT_CONFIG_IGNORE_CASE));
if (dtype != DT_DIR && has_path_in_index)
return path_none;
@@ -3067,7 +3072,7 @@ static int cmp_icase(char a, char b)
{
if (a == b)
return 0;
- if (ignore_case)
+ if (get_int_config_global(INT_CONFIG_IGNORE_CASE))
return toupper(a) - toupper(b);
return a - b;
}
diff --git a/environment.c b/environment.c
index 312e006feb0..44cbaa3e0f9 100644
--- a/environment.c
+++ b/environment.c
@@ -32,7 +32,6 @@
int check_stat = 1;
int minimum_abbrev = 4, default_abbrev = -1;
-int ignore_case;
int assume_unchanged;
int prefer_symlink_refs;
int is_bare_repository_cfg = -1; /* unspecified */
diff --git a/environment.h b/environment.h
index fb2cfa9c1aa..d77f4b233f8 100644
--- a/environment.h
+++ b/environment.h
@@ -113,7 +113,6 @@ void set_git_work_tree(const char *tree);
/* Environment bits from configuration mechanism */
extern int check_stat;
extern int minimum_abbrev, default_abbrev;
-extern int ignore_case;
extern int assume_unchanged;
extern int prefer_symlink_refs;
extern int warn_ambiguous_refs;
diff --git a/global-config.c b/global-config.c
index 526ced5b24c..779b894cf07 100644
--- a/global-config.c
+++ b/global-config.c
@@ -8,6 +8,7 @@ static int global_ints[] = {
[INT_CONFIG_TRUST_CTIME] = 1,
[INT_CONFIG_QUOTE_PATH_FULLY] = 1,
[INT_CONFIG_HAS_SYMLINKS] = 1,
+ [INT_CONFIG_IGNORE_CASE] = 0,
};
/* Bitmask for the enum. */
@@ -19,6 +20,7 @@ static const char *global_int_names[] = {
[INT_CONFIG_TRUST_CTIME] = "core.trustctime",
[INT_CONFIG_QUOTE_PATH_FULLY] = "core.quotepath",
[INT_CONFIG_HAS_SYMLINKS] = "core.symlinks",
+ [INT_CONFIG_IGNORE_CASE] = "core.ignorecase",
};
static int config_available;
diff --git a/global-config.h b/global-config.h
index 2532f426e2b..cb32503fbd9 100644
--- a/global-config.h
+++ b/global-config.h
@@ -7,6 +7,7 @@ enum int_config_key {
INT_CONFIG_TRUST_CTIME,
INT_CONFIG_QUOTE_PATH_FULLY,
INT_CONFIG_HAS_SYMLINKS,
+ INT_CONFIG_IGNORE_CASE,
};
/**
diff --git a/merge-recursive.c b/merge-recursive.c
index 10fdd14a642..ec806be4753 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -709,10 +709,9 @@ static int remove_file(struct merge_options *opt, int clean,
return -1;
}
if (update_working_directory) {
- if (ignore_case) {
+ if (get_int_config_global(INT_CONFIG_IGNORE_CASE)) {
struct cache_entry *ce;
- ce = index_file_exists(opt->repo->index, path, strlen(path),
- ignore_case);
+ ce = index_file_exists(opt->repo->index, path, strlen(path), 1);
if (ce && ce_stage(ce) == 0 && strcmp(path, ce->name))
return 0;
}
@@ -875,7 +874,8 @@ static int was_dirty(struct merge_options *opt, const char *path)
return !dirty;
ce = index_file_exists(opt->priv->unpack_opts.src_index,
- path, strlen(path), ignore_case);
+ path, strlen(path),
+ get_int_config_global(INT_CONFIG_IGNORE_CASE));
dirty = verify_uptodate(ce, &opt->priv->unpack_opts) != 0;
return dirty;
}
diff --git a/name-hash.c b/name-hash.c
index fb13716e430..7c32e2be2e9 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -119,7 +119,7 @@ static void hash_index_entry(struct index_state *istate, struct cache_entry *ce)
hashmap_add(&istate->name_hash, &ce->ent);
}
- if (ignore_case)
+ if (get_int_config_global(INT_CONFIG_IGNORE_CASE))
add_dir_entry(istate, ce);
}
@@ -200,7 +200,7 @@ static int lookup_lazy_params(struct index_state *istate)
* code to build the "istate->name_hash". We don't
* need the complexity here.
*/
- if (!ignore_case)
+ if (!get_int_config_global(INT_CONFIG_IGNORE_CASE))
return 0;
nr_cpus = online_cpus();
@@ -642,7 +642,7 @@ void remove_name_hash(struct index_state *istate, struct cache_entry *ce)
ce->ce_flags &= ~CE_HASHED;
hashmap_remove(&istate->name_hash, &ce->ent, ce);
- if (ignore_case)
+ if (get_int_config_global(INT_CONFIG_IGNORE_CASE))
remove_dir_entry(istate, ce);
}
diff --git a/read-cache.c b/read-cache.c
index b80a54133f9..444b37aff63 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -828,12 +828,12 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
* case of the file being added to the repository matches (is folded into) the existing
* entry's directory case.
*/
- if (ignore_case) {
+ if (get_int_config_global(INT_CONFIG_IGNORE_CASE)) {
adjust_dirname_case(istate, ce->name);
}
if (!(flags & ADD_CACHE_RENORMALIZE)) {
alias = index_file_exists(istate, ce->name,
- ce_namelen(ce), ignore_case);
+ ce_namelen(ce), get_int_config_global(INT_CONFIG_IGNORE_CASE));
if (alias &&
!ce_stage(alias) &&
!ie_match_stat(istate, alias, st, ce_option)) {
@@ -854,7 +854,7 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
} else
set_object_name_for_intent_to_add_entry(ce);
- if (ignore_case && alias && different_name(ce, alias))
+ if (get_int_config_global(INT_CONFIG_IGNORE_CASE) && alias && different_name(ce, alias))
ce = create_alias_ce(istate, ce, alias);
ce->ce_flags |= CE_ADDED;
@@ -1024,7 +1024,7 @@ static int verify_dotfile(const char *rest, unsigned mode)
switch (*rest) {
/*
* ".git" followed by NUL or slash is bad. Note that we match
- * case-insensitively here, even if ignore_case is not set.
+ * case-insensitively here, regardless of core.ignoreCase.
* This outlaws ".GIT" everywhere out of an abundance of caution,
* since there's really no good reason to allow it.
*
diff --git a/t/helper/test-lazy-init-name-hash.c b/t/helper/test-lazy-init-name-hash.c
index f23d983c118..5b69f90d888 100644
--- a/t/helper/test-lazy-init-name-hash.c
+++ b/t/helper/test-lazy-init-name-hash.c
@@ -212,11 +212,6 @@ int cmd__lazy_init_name_hash(int argc, const char **argv)
argc = parse_options(argc, argv, prefix, options, usage, 0);
- /*
- * istate->dir_hash is only created when ignore_case is set.
- */
- ignore_case = 1;
-
if (dump) {
if (perf || analyze > 0)
die("cannot combine dump, perf, or analyze");
diff --git a/t/perf/p0004-lazy-init-name-hash.sh b/t/perf/p0004-lazy-init-name-hash.sh
index 85be14e4ddb..90dad259161 100755
--- a/t/perf/p0004-lazy-init-name-hash.sh
+++ b/t/perf/p0004-lazy-init-name-hash.sh
@@ -6,6 +6,12 @@ test_description='Tests multi-threaded lazy_init_name_hash'
test_perf_large_repo
test_checkout_worktree
+test_expect_success 'initialize core.ignorecase' '
+ # This is needed since the name-hash structure is ignored when
+ # core.ignorecase is false.
+ git config core.ignorecase true
+'
+
test_expect_success 'verify both methods build the same hashmaps' '
test-tool lazy-init-name-hash --dump --single >out.single &&
if test-tool lazy-init-name-hash --dump --multi >out.multi
diff --git a/t/t3008-ls-files-lazy-init-name-hash.sh b/t/t3008-ls-files-lazy-init-name-hash.sh
index 51d3dffaa66..54bde980769 100755
--- a/t/t3008-ls-files-lazy-init-name-hash.sh
+++ b/t/t3008-ls-files-lazy-init-name-hash.sh
@@ -22,6 +22,7 @@ test_expect_success 'no buffer overflow in lazy_init_name_hash' '
) |
sed "s/^/100644 $EMPTY_BLOB /" |
git update-index --index-info &&
+ git config core.ignoreCase true &&
test-tool lazy-init-name-hash -m
'
diff --git a/unpack-trees.c b/unpack-trees.c
index c0732aa0c2d..b678745e699 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2405,7 +2405,7 @@ static int check_ok_to_remove(const char *name, int len, int dtype,
*
* Ignore that lstat() if it matches.
*/
- if (ignore_case && icase_exists(o, name, len, st))
+ if (get_int_config_global(INT_CONFIG_IGNORE_CASE) && icase_exists(o, name, len, st))
return 0;
if (o->internal.dir &&
--
gitgitgadget
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 0/6] [RFC] Lazy-loaded default Git config
2023-06-02 14:33 [PATCH 0/6] [RFC] Lazy-loaded default Git config Derrick Stolee via GitGitGadget
` (5 preceding siblings ...)
2023-06-02 14:33 ` [PATCH 6/6] config: move ignore_case " Derrick Stolee via GitGitGadget
@ 2023-06-08 18:19 ` Glen Choo
6 siblings, 0 replies; 8+ messages in thread
From: Glen Choo @ 2023-06-08 18:19 UTC (permalink / raw)
To: Derrick Stolee via GitGitGadget, git
Cc: vdye, johannes.schindelin, newren, peff, gitster, Derrick Stolee
Hi Stolee!
Thanks for coming to the Libification discussion today, it was nice to
be able to discuss this idea with a bigger group.
As is custom, I'll repost my own thoughts here. If folks are interested
in a summary of the whole discussion, you can find the notes at
https://docs.google.com/document/d/1mw_qPPgfQUv1gfKMwmvZjpYaOOzxcNH2N8bRbvBWfIQ/edit#heading=h.fcm7uhwlk55z
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> * Is this a worthwhile category of bug to protect against?
If we look past the concrete bugs and consider the general problem of
process-wide state being hard to use correctly, I think this is
definitely worth solving.
And in case anyone doubts the the current interface is hard to use
correctly: the state lives in poorly scoped global variables that need
to be manually initialized by calling git_config() at the right time
with git_default_config. It's also hard to remember to do this for your
builtin because some builtins call git_default_config in _their own_
outer config_fn_t, and others call git_config(git_default_config).
> * Is wrapping global state in accessor methods the right way to protect
> against that class of bugs?
Lazy-loading via accessor methods seem slightly excessive. The crux of
the problem is that we haven't initialized the values before they are
read, so I think we can get away with plain fields in a struct as long
as they are always initialized (e.g. the builtin author doesn't need to
remember to do this). We could initialize all of the fields during the
setup process, where you placed declare_config_available(), at which
point config should be available.
If we use config hash lookups to intialize the values we want,
pre-initializing like this shouldn't be noticeably more wasteful
compared to lazy-loading, since in either case, we are already parsing
all of the config into the in-memory config sets and looking them up
with hash lookups. Pre-initializing does a small bit more work upfront
by assigning to the fields, but it means that accessing the settings
later can be faster since we can avoid the "getter method" calls.
> * Are there other benefits to doing this?
Yes! I'm generally excited about encapsulating loose global variables
into an appropriate abstraction, e.g. it was something I was thinking
about this when I was working on protected config (which is loose global
state, but also sort of a collection of repo-wide settings). It'll be
extremely relevant for libification, since this paves the way to
eventually encapsulate the process-wide state in a way that makes it
separable from the library code.
> * Are there reasons why this cure is worse than the disease?
This seems quite similar to what we're doing with repo-settings.c (which
also has its own problems with initialization), and I'd like to avoid
having two similar-looking-but-different systems in the long run. For
this to go forward, I'd like to see either an explicit goal to deprecate
repo-settings.c (e.g. this new system handles repository settings in
addition to process-wide settings), or a reasoned separation between
them.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-06-08 18:19 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-02 14:33 [PATCH 0/6] [RFC] Lazy-loaded default Git config Derrick Stolee via GitGitGadget
2023-06-02 14:33 ` [PATCH 1/6] config: create new global config helpers Derrick Stolee via GitGitGadget
2023-06-02 14:33 ` [PATCH 2/6] config: add trust_executable_bit to global config Derrick Stolee via GitGitGadget
2023-06-02 14:33 ` [PATCH 3/6] config: move trust_ctime " Derrick Stolee via GitGitGadget
2023-06-02 14:33 ` [PATCH 4/6] config: move quote_path_fully " Derrick Stolee via GitGitGadget
2023-06-02 14:33 ` [PATCH 5/6] config: move has_symlinks " Derrick Stolee via GitGitGadget
2023-06-02 14:33 ` [PATCH 6/6] config: move ignore_case " Derrick Stolee via GitGitGadget
2023-06-08 18:19 ` [PATCH 0/6] [RFC] Lazy-loaded default Git config Glen Choo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).