diff mbox series

repo-settings.c: simplify the setup

Message ID patch-1.1-e1d8c842c70-20210428T161817Z-avarab@gmail.com (mailing list archive)
State New
Headers show
Series repo-settings.c: simplify the setup | expand

Commit Message

Ævar Arnfjörð Bjarmason April 28, 2021, 4:26 p.m. UTC
Simplify the setup code in repo-settings.c in various ways, making the
code shorter, easier to read, and requiring fewer hacks to do the same
thing as it did before:

Since 7211b9e7534 (repo-settings: consolidate some config settings,
2019-08-13) we have memset() the whole "settings" structure to -1, and
subsequently relied on the -1 value. As it turns out most things did
not need to be initialized to -1, and e.g. UNTRACKED_CACHE_UNSET and
FETCH_NEGOTIATION_UNSET existed purely to reflect the previous
internal state of the prepare_repo_settings() function.

Much of the "are we -1, then read xyz" can simply be removed by
re-arranging what we read first. E.g. we should read
feature.experimental first, set some values, and then e.g. an explicit
index.version setting should override that. We don't need to read
index.version first, and then check when reading feature.experimental
if it's still -1.

Instead of the global ignore_untracked_cache_config variable added in
dae6c322fa1 (test-dump-untracked-cache: don't modify the untracked
cache, 2016-01-27) we can make use of the new facility to set config
via environment variables added in d8d77153eaf (config: allow
specifying config entries via envvar pairs, 2021-01-12).

It's arguably a bit hacky to use setenv() and getenv() to pass
messages between the same program, but since the test helpers are not
the main intended audience of repo-settings.c I think it's better than
hardcoding the test-only special-case in prepare_repo_settings().

In ad0fb659993 (repo-settings: parse core.untrackedCache, 2019-08-13)
the "unset" and "keep" handling for core.untrackedCache was
consolidated. But it apparently wasn't noticed that while we
understand the "keep" value, we actually don't handle it differently
than the case of any other unknown value.

So we can remove UNTRACKED_CACHE_KEEP from the codebase. It's not
handled any differently than UNTRACKED_CACHE_UNSET once we get past
the config parsing step.

The UPDATE_DEFAULT_BOOL() wrapper added in 31b1de6a09b (commit-graph:
turn on commit-graph by default, 2019-08-13) is redundant to simply
using the return value from repo_config_get_bool(), which is non-zero
if the provided key exists in the config.

This also fixes an (admittedly obscure) logic error in the previous
code where we'd conflate an explicit "-1" value in the config with our
own earlier memset() -1.

Since the two enum fields added in aaf633c2ad1 (repo-settings: create
feature.experimental setting, 2019-08-13) and
ad0fb659993 (repo-settings: parse core.untrackedCache, 2019-08-13)
don't rely on the memzero() setting them to "-1" anymore we don't have
to provide them with explicit values. Let's also explicitly use the
enum type in read-cache.c and fetch-negotiator.c for
self-documentation. Since the FETCH_NEGOTIATION_UNSET is gone we can
remove the "default" case in fetch-negotiator.c, and rely on the
compiler to complain about missing enum values instead.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

On Wed, Apr 28 2021, Ævar Arnfjörð Bjarmason wrote:

> On Tue, Apr 27 2021, Derrick Stolee wrote:
>
>> This is correct. I suppose it would be a good change to make some time.
>> Such a rename could be combined with the refactor above.
>>
>> I would recommend waiting until such a change isn't conflicting with
>> ongoing topics, such as this one.
>
> I'm not planning to work on it, but thought I'd ask/prod the original
> author if they were interested :)

Seems I'm pretty bad at sticking to my plans. Here's that refactoring,
since I mostly had this hacked-up locally anyway.

The conflict with the fsmonitor work can be resolved by adding:

	repo_config_get_bool_or(r, "core.usebuiltinfsmonitor",
				&r->settings.use_builtin_fsmonitor, 0);

To the "Boolean config or default, does not cascade (simple)" section
in my version. I.e. I assume nothing past 04/23 cares about the case
where it was set to "-1", which as noted in the commit message above
was (like many other setting variables) leaking an internal
implementation detail.

 cache.h                              |   7 --
 environment.c                        |   7 --
 fetch-negotiator.c                   |   6 +-
 read-cache.c                         |  17 ++--
 repo-settings.c                      | 119 +++++++++++++++------------
 repository.h                         |  15 ++--
 t/helper/test-dump-untracked-cache.c |   6 +-
 7 files changed, 92 insertions(+), 85 deletions(-)

Comments

Derrick Stolee April 28, 2021, 7:09 p.m. UTC | #1
On 4/28/2021 12:26 PM, Ævar Arnfjörð Bjarmason wrote:
> Simplify the setup code in repo-settings.c in various ways, making the
> code shorter, easier to read, and requiring fewer hacks to do the same
> thing as it did before:

This patch is interesting, and I'll review it when I have some more
time. Probably tomorrow.

But I thought that I would point out that this pattern of adding a
patch within the thread of a larger series makes it very difficult
to separate the two. I use an email client that groups messages by
thread in order to help parse meaningful discussion from the list
which otherwise looks like a fire hose of noise. Now, this patch is
linked to the FS Monitor thread and feedback to either will trigger
the thread as having unread messages.

I find it very difficult to track multiple patch series that are
being juggled in the same thread. It is mentally taxing enough that
I have avoided reviewing code presented this way to save myself the
effort of tracking which patches go with what topic in what order.

Since I've committed to reviewing the FS Monitor code, I'd prefer if
this patch (or maybe its v2, since this is here already) be sent as
a top-level message so it can be discussed independently.

Thanks,
-Stolee
Ævar Arnfjörð Bjarmason April 28, 2021, 11:01 p.m. UTC | #2
On Wed, Apr 28 2021, Derrick Stolee wrote:

> On 4/28/2021 12:26 PM, Ævar Arnfjörð Bjarmason wrote:
>> Simplify the setup code in repo-settings.c in various ways, making the
>> code shorter, easier to read, and requiring fewer hacks to do the same
>> thing as it did before:
>
> This patch is interesting, and I'll review it when I have some more
> time. Probably tomorrow.
>
> But I thought that I would point out that this pattern of adding a
> patch within the thread of a larger series makes it very difficult
> to separate the two. I use an email client that groups messages by
> thread in order to help parse meaningful discussion from the list
> which otherwise looks like a fire hose of noise. Now, this patch is
> linked to the FS Monitor thread and feedback to either will trigger
> the thread as having unread messages.
>
> I find it very difficult to track multiple patch series that are
> being juggled in the same thread. It is mentally taxing enough that
> I have avoided reviewing code presented this way to save myself the
> effort of tracking which patches go with what topic in what order.
>
> Since I've committed to reviewing the FS Monitor code, I'd prefer if
> this patch (or maybe its v2, since this is here already) be sent as
> a top-level message so it can be discussed independently.

As a practical matter I think any effort I make to accommodate your
request will be dwarfed by your own starting of a sub-thread on
E-Mail/MUA nuances :)

When [1] was brought up the other day (showing that I'm probably not the
best person to ask about on-list In-Reply-To semantics) I was surprised
to find that we don't have much (if any) explicit documentation about
In-Reply-To best practices. There's a passing mention in
Documentation/MyFirstContribution.txt, but as far as I can tell from a
cursory glance that's it.

Personally I draw the line at "this random unrelated thing occurred to
me while reading X" v.s. "this is directly in reply to X".

Reading the upthread I don't really see a good point at which to start
breaking the reply chain and not make things harder for others reading
along with clients that aren't yours (which, looking at your headers
seems to be Thunderbird 78).

I.e. the one feedback on the patch idea is your upthread "waiting until
such a change". With threading you can see the context, but without
you'd need to get it via some not-MUA side-channel (presumably
lore.kernel.org link). Sending a v2 (if any) without threading would
break the chain again.

1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2103191540330.57@tvgsbejvaqbjf.bet/
Junio C Hamano April 29, 2021, 5:12 a.m. UTC | #3
Derrick Stolee <stolee@gmail.com> writes:

> On 4/28/2021 12:26 PM, Ævar Arnfjörð Bjarmason wrote:
>> Simplify the setup code in repo-settings.c in various ways, making the
>> code shorter, easier to read, and requiring fewer hacks to do the same
>> thing as it did before:
>
> This patch is interesting, and I'll review it when I have some more
> time. Probably tomorrow.
>
> But I thought that I would point out that this pattern of adding a
> patch within the thread of a larger series makes it very difficult
> to separate the two. I use an email client that groups messages by
> thread in order to help parse meaningful discussion from the list
> which otherwise looks like a fire hose of noise. Now, this patch is
> linked to the FS Monitor thread and feedback to either will trigger
> the thread as having unread messages.
>
> I find it very difficult to track multiple patch series that are
> being juggled in the same thread. It is mentally taxing enough that
> I have avoided reviewing code presented this way to save myself the
> effort of tracking which patches go with what topic in what order.

I do find it distracting to have a full "ah, I just thought of
something while discussing this unrelated series" patch fairly
irritating for the same reason.  It however is unavoidable human
nature that we come up with ideas while thinking about something not
necessarily related.  So it largely is a presentation issue.

I really appreciate the way some people (Peff is a stellar example,
but there are others who are as good at this) handle these tangents,
where the message sent to an existing thread is limited to only give
an outline of the idea (possibly with "something like this?" patch
for illustration) and then they quickly get out of the way of the
discussion by starting a separate thread, while back-referencing "So
here is a proper patch based on the idea I interjected in the
discussion of that other topic."  And the discussion on the tangent
will be done on its own thread.
Ævar Arnfjörð Bjarmason April 29, 2021, 12:14 p.m. UTC | #4
On Thu, Apr 29 2021, Junio C Hamano wrote:

> Derrick Stolee <stolee@gmail.com> writes:
>
>> On 4/28/2021 12:26 PM, Ævar Arnfjörð Bjarmason wrote:
>>> Simplify the setup code in repo-settings.c in various ways, making the
>>> code shorter, easier to read, and requiring fewer hacks to do the same
>>> thing as it did before:
>>
>> This patch is interesting, and I'll review it when I have some more
>> time. Probably tomorrow.
>>
>> But I thought that I would point out that this pattern of adding a
>> patch within the thread of a larger series makes it very difficult
>> to separate the two. I use an email client that groups messages by
>> thread in order to help parse meaningful discussion from the list
>> which otherwise looks like a fire hose of noise. Now, this patch is
>> linked to the FS Monitor thread and feedback to either will trigger
>> the thread as having unread messages.
>>
>> I find it very difficult to track multiple patch series that are
>> being juggled in the same thread. It is mentally taxing enough that
>> I have avoided reviewing code presented this way to save myself the
>> effort of tracking which patches go with what topic in what order.
>
> I do find it distracting to have a full "ah, I just thought of
> something while discussing this unrelated series" patch fairly
> irritating for the same reason.  It however is unavoidable human
> nature that we come up with ideas while thinking about something not
> necessarily related.  So it largely is a presentation issue.
>
> I really appreciate the way some people (Peff is a stellar example,
> but there are others who are as good at this) handle these tangents,
> where the message sent to an existing thread is limited to only give
> an outline of the idea (possibly with "something like this?" patch
> for illustration) and then they quickly get out of the way of the
> discussion by starting a separate thread, while back-referencing "So
> here is a proper patch based on the idea I interjected in the
> discussion of that other topic."  And the discussion on the tangent
> will be done on its own thread.

In RFC 822 terms. Are you talking about the In-Reply-To[1] or
References[2] headers, or both/neither?

I'm happy to go along with whatever the convention is, but as noted
think it's valuable to come to some explicit decision to document the
convention.

Threading isn't a concept that exists in E-Mail protocols per-se. Just
In-Reply-To and References. The References header can reference N
messages most would think about as a separate "thread", and "thread" is
ultimately some fuzzy MUA-specific concept on top of these (and others).

E.g. in my client right now I'm looking at just 4 messages in this
"thread", it doesn't descend down the whole In-Reply-To, others would
act differently.

Some (such as GMail) have their own ad-hoc concept of "thread" separate
from anything in RFCs (which includes some fuzzy group-by-subject). In
GMail's web UI everything as of my "upthread"
<patch-1.1-e1d8c842c70-20210428T161817Z-avarab@gmail.com> is presented
as its own thread.

The ML read as it happens, but it's also a collectively maintained
datastructure.

It seems to me to be better to veer on the side of using standard fields
for their intended purpose for archiving / future use. I.e. making "a
reference" universally machine-readable, as opposed to a lore.kernel.org
link, or a free-form "in a recent thread" blurb.

ML Archive Formats Matter[3] :)

But yes, maybe MUAs in the wild these days mostly render things one way
or another, so catering to them would be a good trade-off. I'm writing
this from within an Emacs MUA, so I don't have much of a feel for common
MUA conventions these days.

I'm prodding to see if we can define the problem exactly, because
e.g. maybe "References: <break@threading.hack> [actual <references>]" is
something that would achieve both aims, i.e. make the references
machine-readable, but break up threading in common in-the-wild
clients. We could then patch format-patch etc. to support such
"detached" threading.

1. https://tools.ietf.org/html/rfc822#section-4.6.2
2. https://tools.ietf.org/html/rfc822#section-4.6.3
3. https://keithp.com/blogs/Repository_Formats_Matter/
Jeff King April 29, 2021, 8:14 p.m. UTC | #5
On Thu, Apr 29, 2021 at 02:14:52PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > I really appreciate the way some people (Peff is a stellar example,
> > but there are others who are as good at this) handle these tangents,
> > where the message sent to an existing thread is limited to only give
> > an outline of the idea (possibly with "something like this?" patch
> > for illustration) and then they quickly get out of the way of the
> > discussion by starting a separate thread, while back-referencing "So
> > here is a proper patch based on the idea I interjected in the
> > discussion of that other topic."  And the discussion on the tangent
> > will be done on its own thread.
> 
> In RFC 822 terms. Are you talking about the In-Reply-To[1] or
> References[2] headers, or both/neither?

Since I got listed as an example, I can tell you what I do: I start a
totally new thread with no in-reply-to or references to the old thread.
And the subject is new (usually "[PATCH 0/N] foo..."), so no clever
group-by-subject heuristics will link them.

It's usually a good idea to reference the message-id/lore link in at
least one direction, though (usually I'd do it in the new thread, saying
"this is a followup to ...", but you could also follow-up in the
original to say "I've spun this off into its own series here...").

Which is really _sort of_ like putting it into "References", except that
it's not machine readable. Which is a good thing, because it's a weaker
form and doesn't tell mail clients to group it all into one thread.

> Threading isn't a concept that exists in E-Mail protocols per-se. Just
> In-Reply-To and References. The References header can reference N
> messages most would think about as a separate "thread", and "thread" is
> ultimately some fuzzy MUA-specific concept on top of these (and others).
> 
> E.g. in my client right now I'm looking at just 4 messages in this
> "thread", it doesn't descend down the whole In-Reply-To, others would
> act differently.

Interesting. Mutt (and notmuch, and public-inbox) definitely view these
as part of a larger thread.  It looks like you're using mu4e; I'm
surprised it doesn't, too (of course some clients will give a partial
view of a thread if you've already marked the older messages as read and
moved them into an archival folder).

> It seems to me to be better to veer on the side of using standard fields
> for their intended purpose for archiving / future use. I.e. making "a
> reference" universally machine-readable, as opposed to a lore.kernel.org
> link, or a free-form "in a recent thread" blurb.

I'd disagree here. There's a long history of intentionally breaking the
thread in mailing lists and newsgroups exactly because the topic is
sufficiently different that you want to make it easy for people to treat
it as a separate unit. I admit there's a bit of an art form to deciding
when that is appropriate and when not.

-Peff
Junio C Hamano April 30, 2021, 12:07 a.m. UTC | #6
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> In RFC 822 terms. Are you talking about the In-Reply-To[1] or
> References[2] headers, or both/neither?

Neither (I think Peff explained why it is a good idea to defer to
verbal communication not to confuse tools better than I could).
Johannes Schindelin May 5, 2021, 4:12 p.m. UTC | #7
Hi Ævar,

On Thu, 29 Apr 2021, Ævar Arnfjörð Bjarmason wrote:

> On Wed, Apr 28 2021, Derrick Stolee wrote:
>
> > On 4/28/2021 12:26 PM, Ævar Arnfjörð Bjarmason wrote:
> >> Simplify the setup code in repo-settings.c in various ways, making the
> >> code shorter, easier to read, and requiring fewer hacks to do the same
> >> thing as it did before:
> >
> > [...]
> > Since I've committed to reviewing the FS Monitor code, I'd prefer if
> > this patch (or maybe its v2, since this is here already) be sent as
> > a top-level message so it can be discussed independently.
>
> As a practical matter I think any effort I make to accommodate your
> request will be dwarfed by your own starting of a sub-thread on
> E-Mail/MUA nuances :)
>
> When [1] was brought up the other day (showing that I'm probably not the
> best person to ask about on-list In-Reply-To semantics) I was surprised
> to find that we don't have much (if any) explicit documentation about
> In-Reply-To best practices. [...]
>
> 1. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2103191540330.57@tvgsbejvaqbjf.bet/

I find it a bit disingenous to reference my complaint about your
disconnected cover letter (which _definitely_ belongs with the patches for
which it covers) with the practice of hiding patches or patch
series deep in a thread discussion an (lengthy!) patch series,
_especially_ if it threatens to totally conflict with that patch series
and thereby disrupt the flow.

Couldn't you hold off with your patch for a while, instead help FSMonitor
get over the finish line, and _then_ submit that simplification of
repo-settings? That would be constructive, from my perspective.

Ciao,
Dscho
diff mbox series

Patch

diff --git a/cache.h b/cache.h
index 148d9ab5f18..7ea0feb3462 100644
--- a/cache.h
+++ b/cache.h
@@ -1684,13 +1684,6 @@  int update_server_info(int);
 const char *get_log_output_encoding(void);
 const char *get_commit_output_encoding(void);
 
-/*
- * This is a hack for test programs like test-dump-untracked-cache to
- * ensure that they do not modify the untracked cache when reading it.
- * Do not use it otherwise!
- */
-extern int ignore_untracked_cache_config;
-
 int committer_ident_sufficiently_given(void);
 int author_ident_sufficiently_given(void);
 
diff --git a/environment.c b/environment.c
index 2f27008424a..bc825cc7e05 100644
--- a/environment.c
+++ b/environment.c
@@ -96,13 +96,6 @@  int auto_comment_line_char;
 /* Parallel index stat data preload? */
 int core_preload_index = 1;
 
-/*
- * This is a hack for test programs like test-dump-untracked-cache to
- * ensure that they do not modify the untracked cache when reading it.
- * Do not use it otherwise!
- */
-int ignore_untracked_cache_config;
-
 /* This is set by setup_git_dir_gently() and/or git_default_config() */
 char *git_work_tree_cfg;
 
diff --git a/fetch-negotiator.c b/fetch-negotiator.c
index 57ed5784e14..c7c0eda7e21 100644
--- a/fetch-negotiator.c
+++ b/fetch-negotiator.c
@@ -8,8 +8,11 @@ 
 void fetch_negotiator_init(struct repository *r,
 			   struct fetch_negotiator *negotiator)
 {
+	enum fetch_negotiation_setting setting;
 	prepare_repo_settings(r);
-	switch(r->settings.fetch_negotiation_algorithm) {
+	setting = r->settings.fetch_negotiation_algorithm;
+
+	switch (setting) {
 	case FETCH_NEGOTIATION_SKIPPING:
 		skipping_negotiator_init(negotiator);
 		return;
@@ -19,7 +22,6 @@  void fetch_negotiator_init(struct repository *r,
 		return;
 
 	case FETCH_NEGOTIATION_DEFAULT:
-	default:
 		default_negotiator_init(negotiator);
 		return;
 	}
diff --git a/read-cache.c b/read-cache.c
index 5a907af2fb5..1aefe4a5c23 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1889,16 +1889,23 @@  static void check_ce_order(struct index_state *istate)
 static void tweak_untracked_cache(struct index_state *istate)
 {
 	struct repository *r = the_repository;
+	enum untracked_cache_setting setting;
 
 	prepare_repo_settings(r);
+	setting = r->settings.core_untracked_cache;
 
-	if (r->settings.core_untracked_cache  == UNTRACKED_CACHE_REMOVE) {
+	switch (setting) {
+	case UNTRACKED_CACHE_REMOVE:
 		remove_untracked_cache(istate);
-		return;
-	}
-
-	if (r->settings.core_untracked_cache == UNTRACKED_CACHE_WRITE)
+		break;
+	case UNTRACKED_CACHE_WRITE:
 		add_untracked_cache(istate);
+		break;
+	case UNTRACKED_CACHE_UNSET:
+		/* This includes core.untrackedCache=keep */
+		break;
+	}
+	return;
 }
 
 static void tweak_split_index(struct index_state *istate)
diff --git a/repo-settings.c b/repo-settings.c
index f7fff0f5ab8..2be242fde1d 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -3,40 +3,84 @@ 
 #include "repository.h"
 #include "midx.h"
 
-#define UPDATE_DEFAULT_BOOL(s,v) do { if (s == -1) { s = v; } } while(0)
+static void repo_config_get_bool_or(struct repository *r, const char *key,
+				    int *dest, int def)
+{
+	if (repo_config_get_bool(r, key, dest))
+		*dest = def;
+}
 
 void prepare_repo_settings(struct repository *r)
 {
-	int value;
+	int experimental;
+	int intval;
 	char *strval;
+	int manyfiles;
 
 	if (r->settings.initialized)
 		return;
 
 	/* Defaults */
-	memset(&r->settings, -1, sizeof(r->settings));
+	r->settings.index_version = -1;
+	r->settings.core_untracked_cache = UNTRACKED_CACHE_UNSET;
+	r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;
+
+	/* Booleans config or default, cascades to other settings */
+	repo_config_get_bool_or(r, "feature.manyfiles", &manyfiles, 0);
+	repo_config_get_bool_or(r, "feature.experimental", &experimental, 0);
+
+	/* Defaults modified by feature.* */
+	if (experimental) {
+		r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;
+	}
+	if (manyfiles) {
+		r->settings.index_version = 4;
+		r->settings.core_untracked_cache = UNTRACKED_CACHE_WRITE;
+	}
 
-	if (!repo_config_get_bool(r, "core.commitgraph", &value))
-		r->settings.core_commit_graph = value;
-	if (!repo_config_get_bool(r, "commitgraph.readchangedpaths", &value))
-		r->settings.commit_graph_read_changed_paths = value;
-	if (!repo_config_get_bool(r, "gc.writecommitgraph", &value))
-		r->settings.gc_write_commit_graph = value;
-	UPDATE_DEFAULT_BOOL(r->settings.core_commit_graph, 1);
-	UPDATE_DEFAULT_BOOL(r->settings.commit_graph_read_changed_paths, 1);
-	UPDATE_DEFAULT_BOOL(r->settings.gc_write_commit_graph, 1);
+	/* Boolean config or default, does not cascade (simple)  */
+	repo_config_get_bool_or(r, "core.commitgraph",
+				&r->settings.core_commit_graph, 1);
+	repo_config_get_bool_or(r, "commitgraph.readchangedpaths",
+				&r->settings.commit_graph_read_changed_paths, 1);
+	repo_config_get_bool_or(r, "gc.writecommitgraph",
+				&r->settings.gc_write_commit_graph, 1);
+	repo_config_get_bool_or(r, "fetch.writecommitgraph",
+				&r->settings.fetch_write_commit_graph, 0);
+	repo_config_get_bool_or(r, "pack.usesparse",
+				&r->settings.pack_use_sparse, 1);
+	repo_config_get_bool_or(r, "core.multipackindex",
+				&r->settings.core_multi_pack_index, 1);
 
-	if (!repo_config_get_int(r, "index.version", &value))
-		r->settings.index_version = value;
-	if (!repo_config_get_maybe_bool(r, "core.untrackedcache", &value)) {
-		if (value == 0)
-			r->settings.core_untracked_cache = UNTRACKED_CACHE_REMOVE;
-		else
-			r->settings.core_untracked_cache = UNTRACKED_CACHE_WRITE;
-	} else if (!repo_config_get_string(r, "core.untrackedcache", &strval)) {
-		if (!strcasecmp(strval, "keep"))
-			r->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;
+	/*
+	 * The GIT_TEST_MULTI_PACK_INDEX variable is special in that
+	 * either it *or* the config sets
+	 * r->settings.core_multi_pack_index if true. We don't take
+	 * the environment variable if it exists (even if false) over
+	 * any config, as in other cases.
+	 */
+	if (git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0))
+		r->settings.core_multi_pack_index = 1;
 
+	/*
+	 * Non-boolean config
+	 */
+	if (!repo_config_get_int(r, "index.version", &intval))
+		r->settings.index_version = intval;
+
+	if (!repo_config_get_string(r, "core.untrackedcache", &strval)) {
+		int maybe_bool = git_parse_maybe_bool(strval);
+		if (maybe_bool == -1) {
+			/*
+			 * Set to "keep", or some other non-boolean
+			 * value. In either case we do nothing but
+			 * keep UNTRACKED_CACHE_UNSET.
+			 */
+		} else {
+			r->settings.core_untracked_cache = maybe_bool
+				? UNTRACKED_CACHE_WRITE
+				: UNTRACKED_CACHE_REMOVE;
+		}
 		free(strval);
 	}
 
@@ -45,36 +89,5 @@  void prepare_repo_settings(struct repository *r)
 			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_SKIPPING;
 		else if (!strcasecmp(strval, "noop"))
 			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_NOOP;
-		else
-			r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;
 	}
-
-	if (!repo_config_get_bool(r, "pack.usesparse", &value))
-		r->settings.pack_use_sparse = value;
-	UPDATE_DEFAULT_BOOL(r->settings.pack_use_sparse, 1);
-
-	value = git_env_bool(GIT_TEST_MULTI_PACK_INDEX, 0);
-	if (value || !repo_config_get_bool(r, "core.multipackindex", &value))
-		r->settings.core_multi_pack_index = value;
-	UPDATE_DEFAULT_BOOL(r->settings.core_multi_pack_index, 1);
-
-	if (!repo_config_get_bool(r, "feature.manyfiles", &value) && value) {
-		UPDATE_DEFAULT_BOOL(r->settings.index_version, 4);
-		UPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_WRITE);
-	}
-
-	if (!repo_config_get_bool(r, "fetch.writecommitgraph", &value))
-		r->settings.fetch_write_commit_graph = value;
-	UPDATE_DEFAULT_BOOL(r->settings.fetch_write_commit_graph, 0);
-
-	if (!repo_config_get_bool(r, "feature.experimental", &value) && value)
-		UPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_SKIPPING);
-
-	/* Hack for test programs like test-dump-untracked-cache */
-	if (ignore_untracked_cache_config)
-		r->settings.core_untracked_cache = UNTRACKED_CACHE_KEEP;
-	else
-		UPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_KEEP);
-
-	UPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_DEFAULT);
 }
diff --git a/repository.h b/repository.h
index b385ca3c94b..9345423c5ba 100644
--- a/repository.h
+++ b/repository.h
@@ -12,18 +12,15 @@  struct raw_object_store;
 struct submodule_cache;
 
 enum untracked_cache_setting {
-	UNTRACKED_CACHE_UNSET = -1,
-	UNTRACKED_CACHE_REMOVE = 0,
-	UNTRACKED_CACHE_KEEP = 1,
-	UNTRACKED_CACHE_WRITE = 2
+	UNTRACKED_CACHE_UNSET,
+	UNTRACKED_CACHE_REMOVE,
+	UNTRACKED_CACHE_WRITE,
 };
 
 enum fetch_negotiation_setting {
-	FETCH_NEGOTIATION_UNSET = -1,
-	FETCH_NEGOTIATION_NONE = 0,
-	FETCH_NEGOTIATION_DEFAULT = 1,
-	FETCH_NEGOTIATION_SKIPPING = 2,
-	FETCH_NEGOTIATION_NOOP = 3,
+	FETCH_NEGOTIATION_DEFAULT,
+	FETCH_NEGOTIATION_SKIPPING,
+	FETCH_NEGOTIATION_NOOP,
 };
 
 struct repo_settings {
diff --git a/t/helper/test-dump-untracked-cache.c b/t/helper/test-dump-untracked-cache.c
index cf0f2c7228e..8b73a2f8bc3 100644
--- a/t/helper/test-dump-untracked-cache.c
+++ b/t/helper/test-dump-untracked-cache.c
@@ -45,8 +45,10 @@  int cmd__dump_untracked_cache(int ac, const char **av)
 	struct untracked_cache *uc;
 	struct strbuf base = STRBUF_INIT;
 
-	/* Hack to avoid modifying the untracked cache when we read it */
-	ignore_untracked_cache_config = 1;
+	/* Set core.untrackedCache=keep before setup_git_directory() */
+	setenv("GIT_CONFIG_COUNT", "1", 1);
+	setenv("GIT_CONFIG_KEY_0", "core.untrackedCache", 1);
+	setenv("GIT_CONFIG_VALUE_0", "keep", 1);
 
 	setup_git_directory();
 	if (read_cache() < 0)