* [PATCH v3 1/5] wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c
2021-09-19 8:47 ` [PATCH v3 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc Ævar Arnfjörð Bjarmason
@ 2021-09-19 8:47 ` Ævar Arnfjörð Bjarmason
2021-09-20 21:53 ` Taylor Blau
2021-09-19 8:47 ` [PATCH v3 2/5] environment.c: remove test-specific "ignore_untracked..." variable Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
5 siblings, 1 reply; 34+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-19 8:47 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Jeff Hostetler,
Patrick Steinhardt, Jeff King,
Ævar Arnfjörð Bjarmason
Add fatal wrappers for setenv() and unsetenv(). In d7ac12b25d3 (Add
set_git_dir() function, 2007-08-01) we started checking its return
value, and since 48988c4d0c3 (set_git_dir: die when setenv() fails,
2018-03-30) we've had set_git_dir_1() die if we couldn't set it.
Let's provide a wrapper for both, this will be useful in many other
places, a subsequent patch will make another use of xsetenv().
The checking of the return value here is over-eager according to
setenv(3) and POSIX. It's documented as returning just -1 or 0, so
perhaps we should be checking -1 explicitly.
Let's just instead die on any non-zero, if our C library is so broken
as to return something else than -1 on error (and perhaps not set
errno?) the worst we'll do is die with a nonsensical errno value, but
we'll want to die in either case.
We could make these return "void" (as far as I can tell there's no
other x*() wrappers that needed to make that decision before),
i.e. our "return 0" is only to indicate that we didn't error, which we
would have died on. Let's return "int" instead to be consistent with
the C library function signatures, including for any future code that
expects a pointer to a setenv()-like function.
I think it would be OK skip the NULL check of the "name" here for the
calls to die_errno(). Almost all of our setenv() callers are taking a
constant string hardcoded in the source as the first argument, and for
the rest we can probably assume they've done the NULL check
themselves. Even if they didn't, modern C libraries are forgiving
about it (e.g. glibc formatting it as "(null)"), on those that aren't,
well, we were about to die anyway. But let's include the check anyway
for good measure.
1. https://pubs.opengroup.org/onlinepubs/009604499/functions/setenv.html
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
environment.c | 3 +--
git-compat-util.h | 2 ++
wrapper.c | 15 +++++++++++++++
3 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/environment.c b/environment.c
index d6b22ede7ea..7d8a949285c 100644
--- a/environment.c
+++ b/environment.c
@@ -330,8 +330,7 @@ char *get_graft_file(struct repository *r)
static void set_git_dir_1(const char *path)
{
- if (setenv(GIT_DIR_ENVIRONMENT, path, 1))
- die(_("could not set GIT_DIR to '%s'"), path);
+ xsetenv(GIT_DIR_ENVIRONMENT, path, 1);
setup_git_env(path);
}
diff --git a/git-compat-util.h b/git-compat-util.h
index b46605300ab..0b0c0305165 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -875,6 +875,8 @@ void *xmemdupz(const void *data, size_t len);
char *xstrndup(const char *str, size_t len);
void *xrealloc(void *ptr, size_t size);
void *xcalloc(size_t nmemb, size_t size);
+int xsetenv(const char *name, const char *value, int overwrite);
+int xunsetenv(const char *name);
void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);
const char *mmap_os_err(void);
void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_t offset);
diff --git a/wrapper.c b/wrapper.c
index 7c6586af321..95f989260cd 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -145,6 +145,21 @@ void *xcalloc(size_t nmemb, size_t size)
return ret;
}
+int xsetenv(const char *name, const char *value, int overwrite)
+{
+ if (setenv(name, value, overwrite))
+ die_errno("setenv(%s, '%s', %d) failed", name ? name : "(null)",
+ value, overwrite);
+ return 0;
+}
+
+int xunsetenv(const char *name)
+{
+ if (!unsetenv(name))
+ die_errno("unsetenv(%s) failed", name ? name : "(null)");
+ return 0;
+}
+
/*
* Limit size of IO chunks, because huge chunks only cause pain. OS X
* 64-bit is buggy, returning EINVAL if len >= INT_MAX; and even in
--
2.33.0.1092.g44c994ea1be
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v3 1/5] wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c
2021-09-19 8:47 ` [PATCH v3 1/5] wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c Ævar Arnfjörð Bjarmason
@ 2021-09-20 21:53 ` Taylor Blau
2021-09-20 23:17 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 34+ messages in thread
From: Taylor Blau @ 2021-09-20 21:53 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Derrick Stolee, Jeff Hostetler,
Patrick Steinhardt, Jeff King
On Sun, Sep 19, 2021 at 10:47:15AM +0200, Ævar Arnfjörð Bjarmason wrote:
> Add fatal wrappers for setenv() and unsetenv(). In d7ac12b25d3 (Add
> set_git_dir() function, 2007-08-01) we started checking its return
> value, and since 48988c4d0c3 (set_git_dir: die when setenv() fails,
> 2018-03-30) we've had set_git_dir_1() die if we couldn't set it.
>
> Let's provide a wrapper for both, this will be useful in many other
> places, a subsequent patch will make another use of xsetenv().
Makes sense.
> We could make these return "void" (as far as I can tell there's no
> other x*() wrappers that needed to make that decision before),
> i.e. our "return 0" is only to indicate that we didn't error, which we
> would have died on. Let's return "int" instead to be consistent with
> the C library function signatures, including for any future code that
> expects a pointer to a setenv()-like function.
This may be a little over-clever ;). It is cute, but returning an int
makes xsetenv a drop-in replacement for setenv. Which is nice, but it
makes it all too-easy to take code like:
if (setenv(...) < 0)
die(_("..."));
and replace it with
if (xsetenv(...) < 0)
which makes the whole conditional redundant, since the wrappers are
guaranteed not to return an error.
In other words, I like the idea that s/setenv/x&/ causes a compile-time
error, and returning an int from these wrappers prevents that from
happening.
This may be a little too-theoretical, and you're certainly free to
disagree, just my $0.02.
> I think it would be OK skip the NULL check of the "name" here for the
> calls to die_errno(). Almost all of our setenv() callers are taking a
> constant string hardcoded in the source as the first argument, and for
> the rest we can probably assume they've done the NULL check
> themselves. Even if they didn't, modern C libraries are forgiving
> about it (e.g. glibc formatting it as "(null)"), on those that aren't,
> well, we were about to die anyway. But let's include the check anyway
> for good measure.
This I think is a good call. I agree in practice that most times we'd be
just fine to pass null to printf() (as we have seen from 88617d11f9
(multi-pack-index: fix potential segfault without sub-command,
2021-07-19) ;)). But there's no reason to rely on risky assumptions when
it's easy to avoid doing so.
> diff --git a/wrapper.c b/wrapper.c
> index 7c6586af321..95f989260cd 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -145,6 +145,21 @@ void *xcalloc(size_t nmemb, size_t size)
> return ret;
> }
>
> +int xsetenv(const char *name, const char *value, int overwrite)
> +{
> + if (setenv(name, value, overwrite))
> + die_errno("setenv(%s, '%s', %d) failed", name ? name : "(null)",
> + value, overwrite);
> + return 0;
> +}
> +
> +int xunsetenv(const char *name)
> +{
> + if (!unsetenv(name))
> + die_errno("unsetenv(%s) failed", name ? name : "(null)");
> + return 0;
> +}
> +
For what it's worth, I find these new messages a little wordy. Maybe
we should just sticky "could not (un)set %s"?
Thanks,
Taylor
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 1/5] wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c
2021-09-20 21:53 ` Taylor Blau
@ 2021-09-20 23:17 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 34+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-20 23:17 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Junio C Hamano, Derrick Stolee, Jeff Hostetler,
Patrick Steinhardt, Jeff King
On Mon, Sep 20 2021, Taylor Blau wrote:
> On Sun, Sep 19, 2021 at 10:47:15AM +0200, Ævar Arnfjörð Bjarmason wrote:
>> Add fatal wrappers for setenv() and unsetenv(). In d7ac12b25d3 (Add
>> set_git_dir() function, 2007-08-01) we started checking its return
>> value, and since 48988c4d0c3 (set_git_dir: die when setenv() fails,
>> 2018-03-30) we've had set_git_dir_1() die if we couldn't set it.
>>
>> Let's provide a wrapper for both, this will be useful in many other
>> places, a subsequent patch will make another use of xsetenv().
>
> Makes sense.
>
>> We could make these return "void" (as far as I can tell there's no
>> other x*() wrappers that needed to make that decision before),
>> i.e. our "return 0" is only to indicate that we didn't error, which we
>> would have died on. Let's return "int" instead to be consistent with
>> the C library function signatures, including for any future code that
>> expects a pointer to a setenv()-like function.
>
> This may be a little over-clever ;). It is cute, but returning an int
> makes xsetenv a drop-in replacement for setenv. Which is nice, but it
> makes it all too-easy to take code like:
>
> if (setenv(...) < 0)
> die(_("..."));
>
> and replace it with
>
> if (xsetenv(...) < 0)
>
> which makes the whole conditional redundant, since the wrappers are
> guaranteed not to return an error.
>
> In other words, I like the idea that s/setenv/x&/ causes a compile-time
> error, and returning an int from these wrappers prevents that from
> happening.
>
> This may be a little too-theoretical, and you're certainly free to
> disagree, just my $0.02.
I'm fine with doing that in principle, I couldn't find any "x*()"
wrappers with different signatures. Yes, having the compiler complain
because you used a "void" return value would be nice.
I did have some vague notion that this might interact badly with
something in compat, i.e. if setenv() or unsetenv() was a fallback, but
I can't think now of why that wouldn't work...
>> I think it would be OK skip the NULL check of the "name" here for the
>> calls to die_errno(). Almost all of our setenv() callers are taking a
>> constant string hardcoded in the source as the first argument, and for
>> the rest we can probably assume they've done the NULL check
>> themselves. Even if they didn't, modern C libraries are forgiving
>> about it (e.g. glibc formatting it as "(null)"), on those that aren't,
>> well, we were about to die anyway. But let's include the check anyway
>> for good measure.
>
> This I think is a good call. I agree in practice that most times we'd be
> just fine to pass null to printf() (as we have seen from 88617d11f9
> (multi-pack-index: fix potential segfault without sub-command,
> 2021-07-19) ;)). But there's no reason to rely on risky assumptions when
> it's easy to avoid doing so.
*nod*
>> diff --git a/wrapper.c b/wrapper.c
>> index 7c6586af321..95f989260cd 100644
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -145,6 +145,21 @@ void *xcalloc(size_t nmemb, size_t size)
>> return ret;
>> }
>>
>> +int xsetenv(const char *name, const char *value, int overwrite)
>> +{
>> + if (setenv(name, value, overwrite))
>> + die_errno("setenv(%s, '%s', %d) failed", name ? name : "(null)",
>> + value, overwrite);
>> + return 0;
>> +}
>> +
>> +int xunsetenv(const char *name)
>> +{
>> + if (!unsetenv(name))
>> + die_errno("unsetenv(%s) failed", name ? name : "(null)");
>> + return 0;
>> +}
>> +
>
> For what it's worth, I find these new messages a little wordy. Maybe
> we should just sticky "could not (un)set %s"?
Sure, will change it.
I think I may have wanted to include the "overwrite" in the setenv() in
some way though, so you'd know if it failed because the libc call
"really" failed, v.s. we just had an existing value in the environment
and didn't set "overwrite".
But reading the docs again that won't work, since it'll succeed if it
can't write that key, i.e. the caller with overwrite=0 needs to
follow-up with a getenv() to see if their value or someone else's was
set (or more probably, they don't care, which is why they used
overwrite=0).
So it doesn't matter either way for xsetenv(), will make it less chatty.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v3 2/5] environment.c: remove test-specific "ignore_untracked..." variable
2021-09-19 8:47 ` [PATCH v3 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc Ævar Arnfjörð Bjarmason
2021-09-19 8:47 ` [PATCH v3 1/5] wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c Ævar Arnfjörð Bjarmason
@ 2021-09-19 8:47 ` Ævar Arnfjörð Bjarmason
2021-09-20 22:10 ` Taylor Blau
2021-09-19 8:47 ` [PATCH v3 3/5] read-cache & fetch-negotiator: check "enum" values in switch() Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
5 siblings, 1 reply; 34+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-19 8:47 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Jeff Hostetler,
Patrick Steinhardt, Jeff King,
Ævar Arnfjörð Bjarmason
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().
This uses the xsetenv() wrapper added in the preceding commit, if we
don't set these in the environment we'll fail in
t7063-status-untracked-cache.sh, but let's fail earlier anyway if that
were to happen.
This breaks any parent process that's potentially using the
GIT_CONFIG_* and GIT_CONFIG_PARAMETERS mechanism to pass one-shot
config setting down to a git subprocess, but in this case we don't
care about the general case of such potential parents. This process
neither spawns other "git" processes, nor is it interested in other
configuration. We might want to pick up other test modes here, but
those will be passed via GIT_TEST_* environment variables.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
cache.h | 7 -------
environment.c | 7 -------
repo-settings.c | 7 +------
t/helper/test-dump-untracked-cache.c | 6 ++++--
4 files changed, 5 insertions(+), 22 deletions(-)
diff --git a/cache.h b/cache.h
index d23de693680..8e60fdd2a12 100644
--- a/cache.h
+++ b/cache.h
@@ -1719,13 +1719,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 7d8a949285c..d73dd0c42f7 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/repo-settings.c b/repo-settings.c
index 0cfe8b787db..b0df8b93b86 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -70,12 +70,7 @@ void prepare_repo_settings(struct repository *r)
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.core_untracked_cache, UNTRACKED_CACHE_KEEP);
UPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_DEFAULT);
/*
diff --git a/t/helper/test-dump-untracked-cache.c b/t/helper/test-dump-untracked-cache.c
index cf0f2c7228e..99010614f6d 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() */
+ xsetenv("GIT_CONFIG_COUNT", "1", 1);
+ xsetenv("GIT_CONFIG_KEY_0", "core.untrackedCache", 1);
+ xsetenv("GIT_CONFIG_VALUE_0", "keep", 1);
setup_git_directory();
if (read_cache() < 0)
--
2.33.0.1092.g44c994ea1be
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v3 2/5] environment.c: remove test-specific "ignore_untracked..." variable
2021-09-19 8:47 ` [PATCH v3 2/5] environment.c: remove test-specific "ignore_untracked..." variable Ævar Arnfjörð Bjarmason
@ 2021-09-20 22:10 ` Taylor Blau
2021-09-20 23:27 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 34+ messages in thread
From: Taylor Blau @ 2021-09-20 22:10 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Derrick Stolee, Jeff Hostetler,
Patrick Steinhardt, Jeff King
On Sun, Sep 19, 2021 at 10:47:16AM +0200, Ævar Arnfjörð Bjarmason wrote:
> 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().
Hmm. I tend to agree that using (a wrapper over) setenv() to pass
messages between the test helper and the rest of Git is a little bit of
a hack.
Everything you wrote should work based on my understanding of the
config-over-environment-variable stuff added recently. But I wish that
it didn't involve losing some grep-ability between the test-helper and
library code.
So I wouldn't be sad to see this patch get dropped, and I also wouldn't
be overly sad to see it get picked up, either. But I don't think it's a
necessary step, and we may be slightly better without it.
Thanks,
Taylor
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 2/5] environment.c: remove test-specific "ignore_untracked..." variable
2021-09-20 22:10 ` Taylor Blau
@ 2021-09-20 23:27 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 34+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-20 23:27 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Junio C Hamano, Derrick Stolee, Jeff Hostetler,
Patrick Steinhardt, Jeff King
On Mon, Sep 20 2021, Taylor Blau wrote:
> On Sun, Sep 19, 2021 at 10:47:16AM +0200, Ævar Arnfjörð Bjarmason wrote:
>> 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().
>
> Hmm. I tend to agree that using (a wrapper over) setenv() to pass
> messages between the test helper and the rest of Git is a little bit of
> a hack.
>
> Everything you wrote should work based on my understanding of the
> config-over-environment-variable stuff added recently. But I wish that
> it didn't involve losing some grep-ability between the test-helper and
> library code.
Does that grep-ability between the two have any reason to exist? The
only reason we need this special-case in the test helper is because it's
not setting up "normal" config.
It could also be made to do so, that's a bigger behavior change than
this narrow change, but likewise we'd just end up with a "git config
core.untrackedCache keep" in some test *.sh somewhere, and no
grep-ability between the test helper and library code.
But now that we have GIT_CONFIG_COUNT etc. using the environment has
become a perfectly fine way to pass this data along, we could also do
that in the *.sh setup, but this was easier, and also easier to
guarantee correctness with the new x*() wrapper.
IOW just because it's called t/helper/test-dump-untracked-cache.c it
really doesn't have any business reaching into the guts of
repo-settings.c to tweak how we set up core.untrackedCache. The only
reason it did was because the code pre-dated the
GIT_CONFIG_{COUNT,KEY,VALUE} implementation.
> So I wouldn't be sad to see this patch get dropped, and I also wouldn't
> be overly sad to see it get picked up, either. But I don't think it's a
> necessary step, and we may be slightly better without it.
>
> Thanks,
> Taylor
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v3 3/5] read-cache & fetch-negotiator: check "enum" values in switch()
2021-09-19 8:47 ` [PATCH v3 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc Ævar Arnfjörð Bjarmason
2021-09-19 8:47 ` [PATCH v3 1/5] wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c Ævar Arnfjörð Bjarmason
2021-09-19 8:47 ` [PATCH v3 2/5] environment.c: remove test-specific "ignore_untracked..." variable Ævar Arnfjörð Bjarmason
@ 2021-09-19 8:47 ` Ævar Arnfjörð Bjarmason
2021-09-20 22:14 ` Taylor Blau
2021-09-19 8:47 ` [PATCH v3 4/5] repo-settings.c: simplify the setup Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
5 siblings, 1 reply; 34+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-19 8:47 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Jeff Hostetler,
Patrick Steinhardt, Jeff King,
Ævar Arnfjörð Bjarmason
Change tweak_untracked_cache() in "read-cache.c" to use a switch() to
have the compiler assert that we checked all possible values in the
"enum untracked_cache_setting" type, and likewise remove the "default"
case in fetch_negotiator_init() in favor of checking for
"FETCH_NEGOTIATION_UNSET" and "FETCH_NEGOTIATION_NONE".
As will be discussed in a subsequent we'll only ever have either of
these set to FETCH_NEGOTIATION_NONE, FETCH_NEGOTIATION_UNSET and
UNTRACKED_CACHE_UNSET within the prepare_repo_settings() function
itself. In preparation for fixing that code let's add a BUG() here to
mark this as unreachable code.
See ad0fb659993 (repo-settings: parse core.untrackedCache, 2019-08-13)
for when the "unset" and "keep" handling for core.untrackedCache was
consolidated, and aaf633c2ad1 (repo-settings: create
feature.experimental setting, 2019-08-13) for the addition of the
"default" pattern in "fetch-negotiator.c".
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
fetch-negotiator.c | 4 +++-
read-cache.c | 15 ++++++++++-----
2 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/fetch-negotiator.c b/fetch-negotiator.c
index 57ed5784e14..237f92b8696 100644
--- a/fetch-negotiator.c
+++ b/fetch-negotiator.c
@@ -19,8 +19,10 @@ void fetch_negotiator_init(struct repository *r,
return;
case FETCH_NEGOTIATION_DEFAULT:
- default:
default_negotiator_init(negotiator);
return;
+ case FETCH_NEGOTIATION_NONE:
+ case FETCH_NEGOTIATION_UNSET:
+ BUG("FETCH_NEGOTIATION_UNSET only in prepare_repo_settings()");
}
}
diff --git a/read-cache.c b/read-cache.c
index 9048ef9e905..6918dc3d8b2 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1944,13 +1944,18 @@ static void tweak_untracked_cache(struct index_state *istate)
prepare_repo_settings(r);
- if (r->settings.core_untracked_cache == UNTRACKED_CACHE_REMOVE) {
+ switch (r->settings.core_untracked_cache) {
+ 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_KEEP:
+ break;
+ case UNTRACKED_CACHE_UNSET:
+ BUG("UNTRACKED_CACHE_UNSET only in prepare_repo_settings()");
+ }
}
static void tweak_split_index(struct index_state *istate)
--
2.33.0.1092.g44c994ea1be
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v3 3/5] read-cache & fetch-negotiator: check "enum" values in switch()
2021-09-19 8:47 ` [PATCH v3 3/5] read-cache & fetch-negotiator: check "enum" values in switch() Ævar Arnfjörð Bjarmason
@ 2021-09-20 22:14 ` Taylor Blau
2021-09-20 23:33 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 34+ messages in thread
From: Taylor Blau @ 2021-09-20 22:14 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Derrick Stolee, Jeff Hostetler,
Patrick Steinhardt, Jeff King
On Sun, Sep 19, 2021 at 10:47:17AM +0200, Ævar Arnfjörð Bjarmason wrote:
> Change tweak_untracked_cache() in "read-cache.c" to use a switch() to
> have the compiler assert that we checked all possible values in the
> "enum untracked_cache_setting" type, and likewise remove the "default"
> case in fetch_negotiator_init() in favor of checking for
> "FETCH_NEGOTIATION_UNSET" and "FETCH_NEGOTIATION_NONE".
>
> As will be discussed in a subsequent we'll only ever have either of
s/subsequent/& patch/ ?
> these set to FETCH_NEGOTIATION_NONE, FETCH_NEGOTIATION_UNSET and
> UNTRACKED_CACHE_UNSET within the prepare_repo_settings() function
> itself. In preparation for fixing that code let's add a BUG() here to
> mark this as unreachable code.
>
> See ad0fb659993 (repo-settings: parse core.untrackedCache, 2019-08-13)
> for when the "unset" and "keep" handling for core.untrackedCache was
> consolidated, and aaf633c2ad1 (repo-settings: create
> feature.experimental setting, 2019-08-13) for the addition of the
> "default" pattern in "fetch-negotiator.c".
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> fetch-negotiator.c | 4 +++-
> read-cache.c | 15 ++++++++++-----
> 2 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/fetch-negotiator.c b/fetch-negotiator.c
> index 57ed5784e14..237f92b8696 100644
> --- a/fetch-negotiator.c
> +++ b/fetch-negotiator.c
> @@ -19,8 +19,10 @@ void fetch_negotiator_init(struct repository *r,
> return;
>
> case FETCH_NEGOTIATION_DEFAULT:
> - default:
> default_negotiator_init(negotiator);
> return;
> + case FETCH_NEGOTIATION_NONE:
> + case FETCH_NEGOTIATION_UNSET:
> + BUG("FETCH_NEGOTIATION_UNSET only in prepare_repo_settings()");
I was briefly confused why this BUG message mentioned
FETCH_NEGOTIATION_UNSET, since we only support FETCH_NEGOTIATION_DEFAULT
here.
But then I realized that it said "only in prepare_repo_settings()", and
we're in fetch_negotiator_init(). So this makes sense to me.
Other than the small typo in the patch message, this looks good to me.
Thanks,
Taylor
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 3/5] read-cache & fetch-negotiator: check "enum" values in switch()
2021-09-20 22:14 ` Taylor Blau
@ 2021-09-20 23:33 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 34+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-20 23:33 UTC (permalink / raw)
To: Taylor Blau
Cc: git, Junio C Hamano, Derrick Stolee, Jeff Hostetler,
Patrick Steinhardt, Jeff King
On Mon, Sep 20 2021, Taylor Blau wrote:
> On Sun, Sep 19, 2021 at 10:47:17AM +0200, Ævar Arnfjörð Bjarmason wrote:
>> Change tweak_untracked_cache() in "read-cache.c" to use a switch() to
>> have the compiler assert that we checked all possible values in the
>> "enum untracked_cache_setting" type, and likewise remove the "default"
>> case in fetch_negotiator_init() in favor of checking for
>> "FETCH_NEGOTIATION_UNSET" and "FETCH_NEGOTIATION_NONE".
>>
>> As will be discussed in a subsequent we'll only ever have either of
>
> s/subsequent/& patch/ ?
Thanks.
>> these set to FETCH_NEGOTIATION_NONE, FETCH_NEGOTIATION_UNSET and
>> UNTRACKED_CACHE_UNSET within the prepare_repo_settings() function
>> itself. In preparation for fixing that code let's add a BUG() here to
>> mark this as unreachable code.
>>
>> See ad0fb659993 (repo-settings: parse core.untrackedCache, 2019-08-13)
>> for when the "unset" and "keep" handling for core.untrackedCache was
>> consolidated, and aaf633c2ad1 (repo-settings: create
>> feature.experimental setting, 2019-08-13) for the addition of the
>> "default" pattern in "fetch-negotiator.c".
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>> fetch-negotiator.c | 4 +++-
>> read-cache.c | 15 ++++++++++-----
>> 2 files changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/fetch-negotiator.c b/fetch-negotiator.c
>> index 57ed5784e14..237f92b8696 100644
>> --- a/fetch-negotiator.c
>> +++ b/fetch-negotiator.c
>> @@ -19,8 +19,10 @@ void fetch_negotiator_init(struct repository *r,
>> return;
>>
>> case FETCH_NEGOTIATION_DEFAULT:
>> - default:
>> default_negotiator_init(negotiator);
>> return;
>> + case FETCH_NEGOTIATION_NONE:
>> + case FETCH_NEGOTIATION_UNSET:
>> + BUG("FETCH_NEGOTIATION_UNSET only in prepare_repo_settings()");
>
> I was briefly confused why this BUG message mentioned
> FETCH_NEGOTIATION_UNSET, since we only support FETCH_NEGOTIATION_DEFAULT
> here.
>
> But then I realized that it said "only in prepare_repo_settings()", and
> we're in fetch_negotiator_init(). So this makes sense to me.
>
> Other than the small typo in the patch message, this looks good to me.
I guess I'll also mention NONE here, so:
BUG("FETCH_NEGOTIATION_{NONE,UNSET} only in prepare_repo_settings()");
Or elaborate a bit:
BUG("FETCH_NEGOTIATION_{NONE,UNSET} used outside of prepare_repo_settings()!");
In any case this lives for just one commit, and is just here to
demonstrate the transition we're in.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v3 4/5] repo-settings.c: simplify the setup
2021-09-19 8:47 ` [PATCH v3 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-09-19 8:47 ` [PATCH v3 3/5] read-cache & fetch-negotiator: check "enum" values in switch() Ævar Arnfjörð Bjarmason
@ 2021-09-19 8:47 ` Ævar Arnfjörð Bjarmason
2021-09-20 12:42 ` Derrick Stolee
2021-09-19 8:47 ` [PATCH v3 5/5] repository.h: don't use a mix of int and bitfields Ævar Arnfjörð Bjarmason
2021-09-21 13:12 ` [PATCH v4 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc Ævar Arnfjörð Bjarmason
5 siblings, 1 reply; 34+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-19 8:47 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Jeff Hostetler,
Patrick Steinhardt, Jeff King,
Ævar Arnfjörð Bjarmason
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 in
prepare_repo_settings(), and subsequently relied on the -1 value.
Most of the fields did not need to be initialized to -1, and because
we were doing that we had the enum labels "UNTRACKED_CACHE_UNSET" and
"FETCH_NEGOTIATION_UNSET" purely to reflect the resulting state
created this memset() in prepare_repo_settings(). No other code used
or relied on them, more on that below.
For the rest most of the subsequent "are we -1, then read xyz" can
simply be removed by re-arranging what we read first. E.g. when
setting the "index.version" setting we should have first read
"feature.experimental", so that it (and "feature.manyfiles") can
provide a default for our "index.version".
Instead the code setting it, added when "feature.manyFiles"[1] was
created, was using the UPDATE_DEFAULT_BOOL() macro added in an earlier
commit[2]. That macro is now gone, since it was only needed for this
pattern of reading things in the wrong order.
This also fixes an (admittedly obscure) logic error where we'd
conflate an explicit "-1" value in the config with our own earlier
memset() -1.
We can also remove the UPDATE_DEFAULT_BOOL() wrapper added in
[3]. Using it 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.
Details on edge cases relating to the memset() to -1, continued from
"more on that below" above:
* UNTRACKED_CACHE_KEEP:
In [4] the "unset" and "keep" handling for core.untrackedCache was
consolidated. But it while we understand the "keep" value, we don't
handle it differently than the case of any other unknown value.
So let's retain UNTRACKED_CACHE_KEEP and remove the
UNTRACKED_CACHE_UNSET setting (which was always implicitly
UNTRACKED_CACHE_KEEP before). We don't need to inform any code
after prepare_repo_settings() that the setting was "unset", as far
as anyone else is concerned it's core.untrackedCache=keep. if
"core.untrackedcache" isn't present in the config.
* FETCH_NEGOTIATION_UNSET & FETCH_NEGOTIATION_NONE:
Since these two two enum fields added in [5] don't rely on the
memzero() setting them to "-1" anymore we don't have to provide
them with explicit values.
1. c6cc4c5afd2 (repo-settings: create feature.manyFiles setting,
2019-08-13)
2. 31b1de6a09b (commit-graph: turn on commit-graph by default,
2019-08-13)
3. 31b1de6a09b (commit-graph: turn on commit-graph by default,
2019-08-13)
4. ad0fb659993 (repo-settings: parse core.untrackedCache,
2019-08-13)
5. aaf633c2ad1 (repo-settings: create feature.experimental setting,
2019-08-13)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
fetch-negotiator.c | 3 --
read-cache.c | 8 +++-
repo-settings.c | 102 +++++++++++++++++++++++++--------------------
repository.h | 15 +++----
4 files changed, 68 insertions(+), 60 deletions(-)
diff --git a/fetch-negotiator.c b/fetch-negotiator.c
index 237f92b8696..273390229fe 100644
--- a/fetch-negotiator.c
+++ b/fetch-negotiator.c
@@ -21,8 +21,5 @@ void fetch_negotiator_init(struct repository *r,
case FETCH_NEGOTIATION_DEFAULT:
default_negotiator_init(negotiator);
return;
- case FETCH_NEGOTIATION_NONE:
- case FETCH_NEGOTIATION_UNSET:
- BUG("FETCH_NEGOTIATION_UNSET only in prepare_repo_settings()");
}
}
diff --git a/read-cache.c b/read-cache.c
index 6918dc3d8b2..147660c3090 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1952,9 +1952,13 @@ static void tweak_untracked_cache(struct index_state *istate)
add_untracked_cache(istate);
break;
case UNTRACKED_CACHE_KEEP:
+ /*
+ * Either an explicit "core.untrackedCache=keep", the
+ * default if "core.untrackedCache" isn't configured,
+ * or a fallback on an unknown "core.untrackedCache"
+ * value.
+ */
break;
- case UNTRACKED_CACHE_UNSET:
- BUG("UNTRACKED_CACHE_UNSET only in prepare_repo_settings()");
}
}
diff --git a/repo-settings.c b/repo-settings.c
index b0df8b93b86..46b9d56aeac 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -3,40 +3,76 @@
#include "repository.h"
#include "midx.h"
-#define UPDATE_DEFAULT_BOOL(s,v) do { if (s == -1) { s = v; } } while(0)
+static void repo_cfg_bool(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 experimental;
int value;
char *strval;
+ int manyfiles;
- if (r->settings.initialized)
+ 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_KEEP;
+ r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;
+
+ /* Booleans config or default, cascades to other settings */
+ repo_cfg_bool(r, "feature.manyfiles", &manyfiles, 0);
+ repo_cfg_bool(r, "feature.experimental", &experimental, 0);
- 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);
+ /* 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;
+ }
+
+ /* Boolean config or default, does not cascade (simple) */
+ repo_cfg_bool(r, "core.commitgraph", &r->settings.core_commit_graph, 1);
+ repo_cfg_bool(r, "commitgraph.readchangedpaths", &r->settings.commit_graph_read_changed_paths, 1);
+ repo_cfg_bool(r, "gc.writecommitgraph", &r->settings.gc_write_commit_graph, 1);
+ repo_cfg_bool(r, "fetch.writecommitgraph", &r->settings.fetch_write_commit_graph, 0);
+ repo_cfg_bool(r, "pack.usesparse", &r->settings.pack_use_sparse, 1);
+ repo_cfg_bool(r, "core.multipackindex", &r->settings.core_multi_pack_index, 1);
+
+ /*
+ * 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 most 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", &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;
+ if (!repo_config_get_string(r, "core.untrackedcache", &strval)) {
+ int v = git_parse_maybe_bool(strval);
+
+ /*
+ * If it's set to "keep", or some other non-boolean
+ * value then "v < 0". Then we do nothing and keep it
+ * at the default of UNTRACKED_CACHE_KEEP.
+ */
+ if (v >= 0)
+ r->settings.core_untracked_cache = v ?
+ UNTRACKED_CACHE_WRITE : UNTRACKED_CACHE_REMOVE;
free(strval);
}
@@ -45,34 +81,8 @@ 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);
-
- UPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_KEEP);
- UPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_DEFAULT);
-
/*
* This setting guards all index reads to require a full index
* over a sparse index. After suitable guards are placed in the
diff --git a/repository.h b/repository.h
index 3740c93bc0f..bf36744e0a7 100644
--- a/repository.h
+++ b/repository.h
@@ -13,18 +13,15 @@ struct submodule_cache;
struct promisor_remote_config;
enum untracked_cache_setting {
- UNTRACKED_CACHE_UNSET = -1,
- UNTRACKED_CACHE_REMOVE = 0,
- UNTRACKED_CACHE_KEEP = 1,
- UNTRACKED_CACHE_WRITE = 2
+ UNTRACKED_CACHE_KEEP,
+ 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 {
--
2.33.0.1092.g44c994ea1be
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v3 4/5] repo-settings.c: simplify the setup
2021-09-19 8:47 ` [PATCH v3 4/5] repo-settings.c: simplify the setup Ævar Arnfjörð Bjarmason
@ 2021-09-20 12:42 ` Derrick Stolee
2021-09-20 22:18 ` Taylor Blau
0 siblings, 1 reply; 34+ messages in thread
From: Derrick Stolee @ 2021-09-20 12:42 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Junio C Hamano, Derrick Stolee, Jeff Hostetler,
Patrick Steinhardt, Jeff King
On 9/19/2021 4:47 AM, Æ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:
Popping in here to say that I agree this new arrangement is cleaner.
The previous patterns were confusing, and probably led to at least one bug,
including one in the builtin FS Monitor shipped with git-for-windows/git [1]
where enabling features.experimental overrides core.useBuiltinFSMonitor=false.
The newer organization should make it harder for mistakes like that to happen.
Thanks,
-Stolee
[1] https://github.com/git-for-windows/git/discussions/3251#discussioncomment-1296896
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 4/5] repo-settings.c: simplify the setup
2021-09-20 12:42 ` Derrick Stolee
@ 2021-09-20 22:18 ` Taylor Blau
0 siblings, 0 replies; 34+ messages in thread
From: Taylor Blau @ 2021-09-20 22:18 UTC (permalink / raw)
To: Derrick Stolee
Cc: Ævar Arnfjörð Bjarmason, git, Junio C Hamano,
Derrick Stolee, Jeff Hostetler, Patrick Steinhardt, Jeff King
On Mon, Sep 20, 2021 at 08:42:37AM -0400, Derrick Stolee wrote:
> On 9/19/2021 4:47 AM, Æ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:
>
> Popping in here to say that I agree this new arrangement is cleaner.
Agreed; I couldn't spot anything in the post-image that wasn't an
improvement in one way or another.
Thanks,
Taylor
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v3 5/5] repository.h: don't use a mix of int and bitfields
2021-09-19 8:47 ` [PATCH v3 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2021-09-19 8:47 ` [PATCH v3 4/5] repo-settings.c: simplify the setup Ævar Arnfjörð Bjarmason
@ 2021-09-19 8:47 ` Ævar Arnfjörð Bjarmason
2021-09-20 22:25 ` Taylor Blau
2021-09-21 13:12 ` [PATCH v4 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc Ævar Arnfjörð Bjarmason
5 siblings, 1 reply; 34+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-19 8:47 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Jeff Hostetler,
Patrick Steinhardt, Jeff King,
Ævar Arnfjörð Bjarmason
Change the bitfield added in 58300f47432 (sparse-index: add
index.sparse config option, 2021-03-30) and 3964fc2aae7 (sparse-index:
add guard to ensure full index, 2021-03-30) to just use an "int"
boolean instead.
It might be smart to optimize the space here in the future, but by
consistently using an "int" we can take its address and pass it to
repo_cfg_bool(), and therefore don't need to handle "sparse_index" as
a special-case when reading the "index.sparse" setting.
There's no corresponding config for "command_requires_full_index", but
let's change it too for consistency and to prevent future bugs
creeping in due to one of these being "unsigned".
Using "int" consistently also prevents subtle bugs or undesired
control flow creeping in here. Before the preceding commit the
initialization of "command_requires_full_index" in
prepare_repo_settings() did nothing, i.e. this:
r->settings.command_requires_full_index = 1
Was redundant to the earlier memset() to -1. Likewise for
"sparse_index" added in 58300f47432 (sparse-index: add index.sparse
config option, 2021-03-30) the code and comment added there was
misleading, we weren't initializing it to off, but re-initializing it
from "1" to "0", and then finally checking the config, and perhaps
setting it to "1" again. I.e. we could have applied this patch before
the preceding commit:
+ assert(r->settings.command_requires_full_index == 1);
r->settings.command_requires_full_index = 1;
/*
* Initialize this as off.
*/
+ assert(r->settings.sparse_index == 1);
r->settings.sparse_index = 0;
if (!repo_config_get_bool(r, "index.sparse", &value) && value)
r->settings.sparse_index = 1;
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
repo-settings.c | 8 +-------
repository.h | 5 ++---
2 files changed, 3 insertions(+), 10 deletions(-)
diff --git a/repo-settings.c b/repo-settings.c
index 46b9d56aeac..b93e91a212e 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -45,6 +45,7 @@ void prepare_repo_settings(struct repository *r)
repo_cfg_bool(r, "fetch.writecommitgraph", &r->settings.fetch_write_commit_graph, 0);
repo_cfg_bool(r, "pack.usesparse", &r->settings.pack_use_sparse, 1);
repo_cfg_bool(r, "core.multipackindex", &r->settings.core_multi_pack_index, 1);
+ repo_cfg_bool(r, "index.sparse", &r->settings.sparse_index, 0);
/*
* The GIT_TEST_MULTI_PACK_INDEX variable is special in that
@@ -90,11 +91,4 @@ void prepare_repo_settings(struct repository *r)
* removed.
*/
r->settings.command_requires_full_index = 1;
-
- /*
- * Initialize this as off.
- */
- r->settings.sparse_index = 0;
- if (!repo_config_get_bool(r, "index.sparse", &value) && value)
- r->settings.sparse_index = 1;
}
diff --git a/repository.h b/repository.h
index bf36744e0a7..02599ae2c98 100644
--- a/repository.h
+++ b/repository.h
@@ -31,6 +31,8 @@ struct repo_settings {
int commit_graph_read_changed_paths;
int gc_write_commit_graph;
int fetch_write_commit_graph;
+ int command_requires_full_index;
+ int sparse_index;
int index_version;
enum untracked_cache_setting core_untracked_cache;
@@ -39,9 +41,6 @@ struct repo_settings {
enum fetch_negotiation_setting fetch_negotiation_algorithm;
int core_multi_pack_index;
-
- unsigned command_requires_full_index:1,
- sparse_index:1;
};
struct repository {
--
2.33.0.1092.g44c994ea1be
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v3 5/5] repository.h: don't use a mix of int and bitfields
2021-09-19 8:47 ` [PATCH v3 5/5] repository.h: don't use a mix of int and bitfields Ævar Arnfjörð Bjarmason
@ 2021-09-20 22:25 ` Taylor Blau
0 siblings, 0 replies; 34+ messages in thread
From: Taylor Blau @ 2021-09-20 22:25 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Derrick Stolee, Jeff Hostetler,
Patrick Steinhardt, Jeff King
On Sun, Sep 19, 2021 at 10:47:19AM +0200, Ævar Arnfjörð Bjarmason wrote:
> There's no corresponding config for "command_requires_full_index", but
> let's change it too for consistency and to prevent future bugs
> creeping in due to one of these being "unsigned".
This was the only thing that I thought of while reading through the
diff, so I appreciate that you explicitly called out why
command_requires_full_index was changed in this patch.
This looks good to me.
Thanks,
Taylor
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v4 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc.
2021-09-19 8:47 ` [PATCH v3 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2021-09-19 8:47 ` [PATCH v3 5/5] repository.h: don't use a mix of int and bitfields Ævar Arnfjörð Bjarmason
@ 2021-09-21 13:12 ` Ævar Arnfjörð Bjarmason
2021-09-21 13:12 ` [PATCH v4 1/5] wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c Ævar Arnfjörð Bjarmason
` (5 more replies)
5 siblings, 6 replies; 34+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-21 13:12 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Jeff Hostetler,
Patrick Steinhardt, Jeff King, Taylor Blau,
Ævar Arnfjörð Bjarmason
A hopefully final re-roll addressing Taylor's v3 review, except for
the suggestion (that I read as) perhaps retaining the test-only code,
which I've decided not to do per
http://lore.kernel.org/git/87tuieakms.fsf@evledraar.gmail.com
The x(un)setenv() now returns void, and the error messages are less
chatty, I also improved a BUG() message in 4/5 that we end up deleting
in 5/5 anyway, so it doesn't matter for the end-state, just for
understanding the patches.
Ævar Arnfjörð Bjarmason (5):
wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c
environment.c: remove test-specific "ignore_untracked..." variable
read-cache & fetch-negotiator: check "enum" values in switch()
repo-settings.c: simplify the setup
repository.h: don't use a mix of int and bitfields
cache.h | 7 --
environment.c | 10 +--
fetch-negotiator.c | 1 -
git-compat-util.h | 2 +
read-cache.c | 19 +++--
repo-settings.c | 115 +++++++++++++--------------
repository.h | 20 ++---
t/helper/test-dump-untracked-cache.c | 6 +-
wrapper.c | 12 +++
9 files changed, 98 insertions(+), 94 deletions(-)
Range-diff against v3:
1: 4b320edc933 ! 1: 4dd317ab65e wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c
@@ Commit message
errno?) the worst we'll do is die with a nonsensical errno value, but
we'll want to die in either case.
- We could make these return "void" (as far as I can tell there's no
- other x*() wrappers that needed to make that decision before),
- i.e. our "return 0" is only to indicate that we didn't error, which we
- would have died on. Let's return "int" instead to be consistent with
- the C library function signatures, including for any future code that
- expects a pointer to a setenv()-like function.
+ Let's make these return "void" instead of "int". As far as I can tell
+ there's no other x*() wrappers that needed to make the decision of
+ deviating from the signature in the C library, but since their return
+ value is only used to indicate errors (so we'd die here), we can catch
+ unreachable code such as
+
+ if (xsetenv(...) < 0)
+ [...];
I think it would be OK skip the NULL check of the "name" here for the
calls to die_errno(). Almost all of our setenv() callers are taking a
@@ git-compat-util.h: void *xmemdupz(const void *data, size_t len);
char *xstrndup(const char *str, size_t len);
void *xrealloc(void *ptr, size_t size);
void *xcalloc(size_t nmemb, size_t size);
-+int xsetenv(const char *name, const char *value, int overwrite);
-+int xunsetenv(const char *name);
++void xsetenv(const char *name, const char *value, int overwrite);
++void xunsetenv(const char *name);
void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);
const char *mmap_os_err(void);
void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_t offset);
@@ wrapper.c: void *xcalloc(size_t nmemb, size_t size)
return ret;
}
-+int xsetenv(const char *name, const char *value, int overwrite)
++void xsetenv(const char *name, const char *value, int overwrite)
+{
+ if (setenv(name, value, overwrite))
-+ die_errno("setenv(%s, '%s', %d) failed", name ? name : "(null)",
-+ value, overwrite);
-+ return 0;
++ die_errno(_("could not setenv '%s'"), name ? name : "(null)");
+}
+
-+int xunsetenv(const char *name)
++void xunsetenv(const char *name)
+{
+ if (!unsetenv(name))
-+ die_errno("unsetenv(%s) failed", name ? name : "(null)");
-+ return 0;
++ die_errno(_("could not unsetenv '%s'"), name ? name : "(null)");
+}
+
/*
2: ece340af764 = 2: 3dc37521184 environment.c: remove test-specific "ignore_untracked..." variable
3: d837d905825 ! 3: b36b23ee173 read-cache & fetch-negotiator: check "enum" values in switch()
@@ fetch-negotiator.c: void fetch_negotiator_init(struct repository *r,
return;
+ case FETCH_NEGOTIATION_NONE:
+ case FETCH_NEGOTIATION_UNSET:
-+ BUG("FETCH_NEGOTIATION_UNSET only in prepare_repo_settings()");
++ BUG("FETCH_NEGOTIATION_{NONE,UNSET} used outside of prepare_repo_settings()!");
}
}
@@ read-cache.c: static void tweak_untracked_cache(struct index_state *istate)
+ case UNTRACKED_CACHE_KEEP:
+ break;
+ case UNTRACKED_CACHE_UNSET:
-+ BUG("UNTRACKED_CACHE_UNSET only in prepare_repo_settings()");
++ BUG("UNTRACKED_CACHE_UNSET used outside of prepare_repo_settings()!");
+ }
}
4: 28286a61162 ! 4: c9f143b26f1 repo-settings.c: simplify the setup
@@ fetch-negotiator.c: void fetch_negotiator_init(struct repository *r,
return;
- case FETCH_NEGOTIATION_NONE:
- case FETCH_NEGOTIATION_UNSET:
-- BUG("FETCH_NEGOTIATION_UNSET only in prepare_repo_settings()");
+- BUG("FETCH_NEGOTIATION_{NONE,UNSET} used outside of prepare_repo_settings()!");
}
}
@@ read-cache.c: static void tweak_untracked_cache(struct index_state *istate)
+ */
break;
- case UNTRACKED_CACHE_UNSET:
-- BUG("UNTRACKED_CACHE_UNSET only in prepare_repo_settings()");
+- BUG("UNTRACKED_CACHE_UNSET used outside of prepare_repo_settings()!");
}
}
5: 3cc033b8864 = 5: aadd4c42923 repository.h: don't use a mix of int and bitfields
--
2.33.0.1098.gf02a64c1a2d
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v4 1/5] wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c
2021-09-21 13:12 ` [PATCH v4 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc Ævar Arnfjörð Bjarmason
@ 2021-09-21 13:12 ` Ævar Arnfjörð Bjarmason
2021-09-21 13:13 ` [PATCH v4 2/5] environment.c: remove test-specific "ignore_untracked..." variable Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
5 siblings, 0 replies; 34+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-21 13:12 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Jeff Hostetler,
Patrick Steinhardt, Jeff King, Taylor Blau,
Ævar Arnfjörð Bjarmason
Add fatal wrappers for setenv() and unsetenv(). In d7ac12b25d3 (Add
set_git_dir() function, 2007-08-01) we started checking its return
value, and since 48988c4d0c3 (set_git_dir: die when setenv() fails,
2018-03-30) we've had set_git_dir_1() die if we couldn't set it.
Let's provide a wrapper for both, this will be useful in many other
places, a subsequent patch will make another use of xsetenv().
The checking of the return value here is over-eager according to
setenv(3) and POSIX. It's documented as returning just -1 or 0, so
perhaps we should be checking -1 explicitly.
Let's just instead die on any non-zero, if our C library is so broken
as to return something else than -1 on error (and perhaps not set
errno?) the worst we'll do is die with a nonsensical errno value, but
we'll want to die in either case.
Let's make these return "void" instead of "int". As far as I can tell
there's no other x*() wrappers that needed to make the decision of
deviating from the signature in the C library, but since their return
value is only used to indicate errors (so we'd die here), we can catch
unreachable code such as
if (xsetenv(...) < 0)
[...];
I think it would be OK skip the NULL check of the "name" here for the
calls to die_errno(). Almost all of our setenv() callers are taking a
constant string hardcoded in the source as the first argument, and for
the rest we can probably assume they've done the NULL check
themselves. Even if they didn't, modern C libraries are forgiving
about it (e.g. glibc formatting it as "(null)"), on those that aren't,
well, we were about to die anyway. But let's include the check anyway
for good measure.
1. https://pubs.opengroup.org/onlinepubs/009604499/functions/setenv.html
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
environment.c | 3 +--
git-compat-util.h | 2 ++
wrapper.c | 12 ++++++++++++
3 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/environment.c b/environment.c
index d6b22ede7ea..7d8a949285c 100644
--- a/environment.c
+++ b/environment.c
@@ -330,8 +330,7 @@ char *get_graft_file(struct repository *r)
static void set_git_dir_1(const char *path)
{
- if (setenv(GIT_DIR_ENVIRONMENT, path, 1))
- die(_("could not set GIT_DIR to '%s'"), path);
+ xsetenv(GIT_DIR_ENVIRONMENT, path, 1);
setup_git_env(path);
}
diff --git a/git-compat-util.h b/git-compat-util.h
index ddc65ff61d9..94d8250832d 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -875,6 +875,8 @@ void *xmemdupz(const void *data, size_t len);
char *xstrndup(const char *str, size_t len);
void *xrealloc(void *ptr, size_t size);
void *xcalloc(size_t nmemb, size_t size);
+void xsetenv(const char *name, const char *value, int overwrite);
+void xunsetenv(const char *name);
void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);
const char *mmap_os_err(void);
void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_t offset);
diff --git a/wrapper.c b/wrapper.c
index 7c6586af321..1460d4e27b0 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -145,6 +145,18 @@ void *xcalloc(size_t nmemb, size_t size)
return ret;
}
+void xsetenv(const char *name, const char *value, int overwrite)
+{
+ if (setenv(name, value, overwrite))
+ die_errno(_("could not setenv '%s'"), name ? name : "(null)");
+}
+
+void xunsetenv(const char *name)
+{
+ if (!unsetenv(name))
+ die_errno(_("could not unsetenv '%s'"), name ? name : "(null)");
+}
+
/*
* Limit size of IO chunks, because huge chunks only cause pain. OS X
* 64-bit is buggy, returning EINVAL if len >= INT_MAX; and even in
--
2.33.0.1098.gf02a64c1a2d
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v4 2/5] environment.c: remove test-specific "ignore_untracked..." variable
2021-09-21 13:12 ` [PATCH v4 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc Ævar Arnfjörð Bjarmason
2021-09-21 13:12 ` [PATCH v4 1/5] wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c Ævar Arnfjörð Bjarmason
@ 2021-09-21 13:13 ` Ævar Arnfjörð Bjarmason
2021-09-21 13:13 ` [PATCH v4 3/5] read-cache & fetch-negotiator: check "enum" values in switch() Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
5 siblings, 0 replies; 34+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-21 13:13 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Jeff Hostetler,
Patrick Steinhardt, Jeff King, Taylor Blau,
Ævar Arnfjörð Bjarmason
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().
This uses the xsetenv() wrapper added in the preceding commit, if we
don't set these in the environment we'll fail in
t7063-status-untracked-cache.sh, but let's fail earlier anyway if that
were to happen.
This breaks any parent process that's potentially using the
GIT_CONFIG_* and GIT_CONFIG_PARAMETERS mechanism to pass one-shot
config setting down to a git subprocess, but in this case we don't
care about the general case of such potential parents. This process
neither spawns other "git" processes, nor is it interested in other
configuration. We might want to pick up other test modes here, but
those will be passed via GIT_TEST_* environment variables.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
cache.h | 7 -------
environment.c | 7 -------
repo-settings.c | 7 +------
t/helper/test-dump-untracked-cache.c | 6 ++++--
4 files changed, 5 insertions(+), 22 deletions(-)
diff --git a/cache.h b/cache.h
index 0c245d4f105..8941cc62c59 100644
--- a/cache.h
+++ b/cache.h
@@ -1726,13 +1726,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 7d8a949285c..d73dd0c42f7 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/repo-settings.c b/repo-settings.c
index 0cfe8b787db..b0df8b93b86 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -70,12 +70,7 @@ void prepare_repo_settings(struct repository *r)
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.core_untracked_cache, UNTRACKED_CACHE_KEEP);
UPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_DEFAULT);
/*
diff --git a/t/helper/test-dump-untracked-cache.c b/t/helper/test-dump-untracked-cache.c
index cf0f2c7228e..99010614f6d 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() */
+ xsetenv("GIT_CONFIG_COUNT", "1", 1);
+ xsetenv("GIT_CONFIG_KEY_0", "core.untrackedCache", 1);
+ xsetenv("GIT_CONFIG_VALUE_0", "keep", 1);
setup_git_directory();
if (read_cache() < 0)
--
2.33.0.1098.gf02a64c1a2d
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v4 3/5] read-cache & fetch-negotiator: check "enum" values in switch()
2021-09-21 13:12 ` [PATCH v4 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc Ævar Arnfjörð Bjarmason
2021-09-21 13:12 ` [PATCH v4 1/5] wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c Ævar Arnfjörð Bjarmason
2021-09-21 13:13 ` [PATCH v4 2/5] environment.c: remove test-specific "ignore_untracked..." variable Ævar Arnfjörð Bjarmason
@ 2021-09-21 13:13 ` Ævar Arnfjörð Bjarmason
2021-09-21 13:13 ` [PATCH v4 4/5] repo-settings.c: simplify the setup Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
5 siblings, 0 replies; 34+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-21 13:13 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Jeff Hostetler,
Patrick Steinhardt, Jeff King, Taylor Blau,
Ævar Arnfjörð Bjarmason
Change tweak_untracked_cache() in "read-cache.c" to use a switch() to
have the compiler assert that we checked all possible values in the
"enum untracked_cache_setting" type, and likewise remove the "default"
case in fetch_negotiator_init() in favor of checking for
"FETCH_NEGOTIATION_UNSET" and "FETCH_NEGOTIATION_NONE".
As will be discussed in a subsequent we'll only ever have either of
these set to FETCH_NEGOTIATION_NONE, FETCH_NEGOTIATION_UNSET and
UNTRACKED_CACHE_UNSET within the prepare_repo_settings() function
itself. In preparation for fixing that code let's add a BUG() here to
mark this as unreachable code.
See ad0fb659993 (repo-settings: parse core.untrackedCache, 2019-08-13)
for when the "unset" and "keep" handling for core.untrackedCache was
consolidated, and aaf633c2ad1 (repo-settings: create
feature.experimental setting, 2019-08-13) for the addition of the
"default" pattern in "fetch-negotiator.c".
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
fetch-negotiator.c | 4 +++-
read-cache.c | 15 ++++++++++-----
2 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/fetch-negotiator.c b/fetch-negotiator.c
index 57ed5784e14..e61e40cb299 100644
--- a/fetch-negotiator.c
+++ b/fetch-negotiator.c
@@ -19,8 +19,10 @@ void fetch_negotiator_init(struct repository *r,
return;
case FETCH_NEGOTIATION_DEFAULT:
- default:
default_negotiator_init(negotiator);
return;
+ case FETCH_NEGOTIATION_NONE:
+ case FETCH_NEGOTIATION_UNSET:
+ BUG("FETCH_NEGOTIATION_{NONE,UNSET} used outside of prepare_repo_settings()!");
}
}
diff --git a/read-cache.c b/read-cache.c
index f5d4385c408..e4ae35ccdb2 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1944,13 +1944,18 @@ static void tweak_untracked_cache(struct index_state *istate)
prepare_repo_settings(r);
- if (r->settings.core_untracked_cache == UNTRACKED_CACHE_REMOVE) {
+ switch (r->settings.core_untracked_cache) {
+ 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_KEEP:
+ break;
+ case UNTRACKED_CACHE_UNSET:
+ BUG("UNTRACKED_CACHE_UNSET used outside of prepare_repo_settings()!");
+ }
}
static void tweak_split_index(struct index_state *istate)
--
2.33.0.1098.gf02a64c1a2d
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v4 4/5] repo-settings.c: simplify the setup
2021-09-21 13:12 ` [PATCH v4 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-09-21 13:13 ` [PATCH v4 3/5] read-cache & fetch-negotiator: check "enum" values in switch() Ævar Arnfjörð Bjarmason
@ 2021-09-21 13:13 ` Ævar Arnfjörð Bjarmason
2021-09-21 13:13 ` [PATCH v4 5/5] repository.h: don't use a mix of int and bitfields Ævar Arnfjörð Bjarmason
2021-09-21 15:58 ` [PATCH v4 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc Derrick Stolee
5 siblings, 0 replies; 34+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-21 13:13 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Jeff Hostetler,
Patrick Steinhardt, Jeff King, Taylor Blau,
Ævar Arnfjörð Bjarmason
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 in
prepare_repo_settings(), and subsequently relied on the -1 value.
Most of the fields did not need to be initialized to -1, and because
we were doing that we had the enum labels "UNTRACKED_CACHE_UNSET" and
"FETCH_NEGOTIATION_UNSET" purely to reflect the resulting state
created this memset() in prepare_repo_settings(). No other code used
or relied on them, more on that below.
For the rest most of the subsequent "are we -1, then read xyz" can
simply be removed by re-arranging what we read first. E.g. when
setting the "index.version" setting we should have first read
"feature.experimental", so that it (and "feature.manyfiles") can
provide a default for our "index.version".
Instead the code setting it, added when "feature.manyFiles"[1] was
created, was using the UPDATE_DEFAULT_BOOL() macro added in an earlier
commit[2]. That macro is now gone, since it was only needed for this
pattern of reading things in the wrong order.
This also fixes an (admittedly obscure) logic error where we'd
conflate an explicit "-1" value in the config with our own earlier
memset() -1.
We can also remove the UPDATE_DEFAULT_BOOL() wrapper added in
[3]. Using it 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.
Details on edge cases relating to the memset() to -1, continued from
"more on that below" above:
* UNTRACKED_CACHE_KEEP:
In [4] the "unset" and "keep" handling for core.untrackedCache was
consolidated. But it while we understand the "keep" value, we don't
handle it differently than the case of any other unknown value.
So let's retain UNTRACKED_CACHE_KEEP and remove the
UNTRACKED_CACHE_UNSET setting (which was always implicitly
UNTRACKED_CACHE_KEEP before). We don't need to inform any code
after prepare_repo_settings() that the setting was "unset", as far
as anyone else is concerned it's core.untrackedCache=keep. if
"core.untrackedcache" isn't present in the config.
* FETCH_NEGOTIATION_UNSET & FETCH_NEGOTIATION_NONE:
Since these two two enum fields added in [5] don't rely on the
memzero() setting them to "-1" anymore we don't have to provide
them with explicit values.
1. c6cc4c5afd2 (repo-settings: create feature.manyFiles setting,
2019-08-13)
2. 31b1de6a09b (commit-graph: turn on commit-graph by default,
2019-08-13)
3. 31b1de6a09b (commit-graph: turn on commit-graph by default,
2019-08-13)
4. ad0fb659993 (repo-settings: parse core.untrackedCache,
2019-08-13)
5. aaf633c2ad1 (repo-settings: create feature.experimental setting,
2019-08-13)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
fetch-negotiator.c | 3 --
read-cache.c | 8 +++-
repo-settings.c | 102 +++++++++++++++++++++++++--------------------
repository.h | 15 +++----
4 files changed, 68 insertions(+), 60 deletions(-)
diff --git a/fetch-negotiator.c b/fetch-negotiator.c
index e61e40cb299..273390229fe 100644
--- a/fetch-negotiator.c
+++ b/fetch-negotiator.c
@@ -21,8 +21,5 @@ void fetch_negotiator_init(struct repository *r,
case FETCH_NEGOTIATION_DEFAULT:
default_negotiator_init(negotiator);
return;
- case FETCH_NEGOTIATION_NONE:
- case FETCH_NEGOTIATION_UNSET:
- BUG("FETCH_NEGOTIATION_{NONE,UNSET} used outside of prepare_repo_settings()!");
}
}
diff --git a/read-cache.c b/read-cache.c
index e4ae35ccdb2..b0a06db5c55 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1952,9 +1952,13 @@ static void tweak_untracked_cache(struct index_state *istate)
add_untracked_cache(istate);
break;
case UNTRACKED_CACHE_KEEP:
+ /*
+ * Either an explicit "core.untrackedCache=keep", the
+ * default if "core.untrackedCache" isn't configured,
+ * or a fallback on an unknown "core.untrackedCache"
+ * value.
+ */
break;
- case UNTRACKED_CACHE_UNSET:
- BUG("UNTRACKED_CACHE_UNSET used outside of prepare_repo_settings()!");
}
}
diff --git a/repo-settings.c b/repo-settings.c
index b0df8b93b86..46b9d56aeac 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -3,40 +3,76 @@
#include "repository.h"
#include "midx.h"
-#define UPDATE_DEFAULT_BOOL(s,v) do { if (s == -1) { s = v; } } while(0)
+static void repo_cfg_bool(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 experimental;
int value;
char *strval;
+ int manyfiles;
- if (r->settings.initialized)
+ 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_KEEP;
+ r->settings.fetch_negotiation_algorithm = FETCH_NEGOTIATION_DEFAULT;
+
+ /* Booleans config or default, cascades to other settings */
+ repo_cfg_bool(r, "feature.manyfiles", &manyfiles, 0);
+ repo_cfg_bool(r, "feature.experimental", &experimental, 0);
- 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);
+ /* 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;
+ }
+
+ /* Boolean config or default, does not cascade (simple) */
+ repo_cfg_bool(r, "core.commitgraph", &r->settings.core_commit_graph, 1);
+ repo_cfg_bool(r, "commitgraph.readchangedpaths", &r->settings.commit_graph_read_changed_paths, 1);
+ repo_cfg_bool(r, "gc.writecommitgraph", &r->settings.gc_write_commit_graph, 1);
+ repo_cfg_bool(r, "fetch.writecommitgraph", &r->settings.fetch_write_commit_graph, 0);
+ repo_cfg_bool(r, "pack.usesparse", &r->settings.pack_use_sparse, 1);
+ repo_cfg_bool(r, "core.multipackindex", &r->settings.core_multi_pack_index, 1);
+
+ /*
+ * 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 most 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", &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;
+ if (!repo_config_get_string(r, "core.untrackedcache", &strval)) {
+ int v = git_parse_maybe_bool(strval);
+
+ /*
+ * If it's set to "keep", or some other non-boolean
+ * value then "v < 0". Then we do nothing and keep it
+ * at the default of UNTRACKED_CACHE_KEEP.
+ */
+ if (v >= 0)
+ r->settings.core_untracked_cache = v ?
+ UNTRACKED_CACHE_WRITE : UNTRACKED_CACHE_REMOVE;
free(strval);
}
@@ -45,34 +81,8 @@ 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);
-
- UPDATE_DEFAULT_BOOL(r->settings.core_untracked_cache, UNTRACKED_CACHE_KEEP);
- UPDATE_DEFAULT_BOOL(r->settings.fetch_negotiation_algorithm, FETCH_NEGOTIATION_DEFAULT);
-
/*
* This setting guards all index reads to require a full index
* over a sparse index. After suitable guards are placed in the
diff --git a/repository.h b/repository.h
index 3740c93bc0f..bf36744e0a7 100644
--- a/repository.h
+++ b/repository.h
@@ -13,18 +13,15 @@ struct submodule_cache;
struct promisor_remote_config;
enum untracked_cache_setting {
- UNTRACKED_CACHE_UNSET = -1,
- UNTRACKED_CACHE_REMOVE = 0,
- UNTRACKED_CACHE_KEEP = 1,
- UNTRACKED_CACHE_WRITE = 2
+ UNTRACKED_CACHE_KEEP,
+ 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 {
--
2.33.0.1098.gf02a64c1a2d
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH v4 5/5] repository.h: don't use a mix of int and bitfields
2021-09-21 13:12 ` [PATCH v4 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2021-09-21 13:13 ` [PATCH v4 4/5] repo-settings.c: simplify the setup Ævar Arnfjörð Bjarmason
@ 2021-09-21 13:13 ` Ævar Arnfjörð Bjarmason
2021-09-21 15:58 ` [PATCH v4 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc Derrick Stolee
5 siblings, 0 replies; 34+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-21 13:13 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, Jeff Hostetler,
Patrick Steinhardt, Jeff King, Taylor Blau,
Ævar Arnfjörð Bjarmason
Change the bitfield added in 58300f47432 (sparse-index: add
index.sparse config option, 2021-03-30) and 3964fc2aae7 (sparse-index:
add guard to ensure full index, 2021-03-30) to just use an "int"
boolean instead.
It might be smart to optimize the space here in the future, but by
consistently using an "int" we can take its address and pass it to
repo_cfg_bool(), and therefore don't need to handle "sparse_index" as
a special-case when reading the "index.sparse" setting.
There's no corresponding config for "command_requires_full_index", but
let's change it too for consistency and to prevent future bugs
creeping in due to one of these being "unsigned".
Using "int" consistently also prevents subtle bugs or undesired
control flow creeping in here. Before the preceding commit the
initialization of "command_requires_full_index" in
prepare_repo_settings() did nothing, i.e. this:
r->settings.command_requires_full_index = 1
Was redundant to the earlier memset() to -1. Likewise for
"sparse_index" added in 58300f47432 (sparse-index: add index.sparse
config option, 2021-03-30) the code and comment added there was
misleading, we weren't initializing it to off, but re-initializing it
from "1" to "0", and then finally checking the config, and perhaps
setting it to "1" again. I.e. we could have applied this patch before
the preceding commit:
+ assert(r->settings.command_requires_full_index == 1);
r->settings.command_requires_full_index = 1;
/*
* Initialize this as off.
*/
+ assert(r->settings.sparse_index == 1);
r->settings.sparse_index = 0;
if (!repo_config_get_bool(r, "index.sparse", &value) && value)
r->settings.sparse_index = 1;
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
repo-settings.c | 8 +-------
repository.h | 5 ++---
2 files changed, 3 insertions(+), 10 deletions(-)
diff --git a/repo-settings.c b/repo-settings.c
index 46b9d56aeac..b93e91a212e 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -45,6 +45,7 @@ void prepare_repo_settings(struct repository *r)
repo_cfg_bool(r, "fetch.writecommitgraph", &r->settings.fetch_write_commit_graph, 0);
repo_cfg_bool(r, "pack.usesparse", &r->settings.pack_use_sparse, 1);
repo_cfg_bool(r, "core.multipackindex", &r->settings.core_multi_pack_index, 1);
+ repo_cfg_bool(r, "index.sparse", &r->settings.sparse_index, 0);
/*
* The GIT_TEST_MULTI_PACK_INDEX variable is special in that
@@ -90,11 +91,4 @@ void prepare_repo_settings(struct repository *r)
* removed.
*/
r->settings.command_requires_full_index = 1;
-
- /*
- * Initialize this as off.
- */
- r->settings.sparse_index = 0;
- if (!repo_config_get_bool(r, "index.sparse", &value) && value)
- r->settings.sparse_index = 1;
}
diff --git a/repository.h b/repository.h
index bf36744e0a7..02599ae2c98 100644
--- a/repository.h
+++ b/repository.h
@@ -31,6 +31,8 @@ struct repo_settings {
int commit_graph_read_changed_paths;
int gc_write_commit_graph;
int fetch_write_commit_graph;
+ int command_requires_full_index;
+ int sparse_index;
int index_version;
enum untracked_cache_setting core_untracked_cache;
@@ -39,9 +41,6 @@ struct repo_settings {
enum fetch_negotiation_setting fetch_negotiation_algorithm;
int core_multi_pack_index;
-
- unsigned command_requires_full_index:1,
- sparse_index:1;
};
struct repository {
--
2.33.0.1098.gf02a64c1a2d
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH v4 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc.
2021-09-21 13:12 ` [PATCH v4 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2021-09-21 13:13 ` [PATCH v4 5/5] repository.h: don't use a mix of int and bitfields Ævar Arnfjörð Bjarmason
@ 2021-09-21 15:58 ` Derrick Stolee
2021-09-21 20:46 ` Taylor Blau
5 siblings, 1 reply; 34+ messages in thread
From: Derrick Stolee @ 2021-09-21 15:58 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason, git
Cc: Junio C Hamano, Derrick Stolee, Jeff Hostetler,
Patrick Steinhardt, Jeff King, Taylor Blau
On 9/21/2021 9:12 AM, Ævar Arnfjörð Bjarmason wrote:
> A hopefully final re-roll addressing Taylor's v3 review, except for
> the suggestion (that I read as) perhaps retaining the test-only code,
> which I've decided not to do per
> http://lore.kernel.org/git/87tuieakms.fsf@evledraar.gmail.com
>
> The x(un)setenv() now returns void, and the error messages are less
> chatty, I also improved a BUG() message in 4/5 that we end up deleting
> in 5/5 anyway, so it doesn't matter for the end-state, just for
> understanding the patches.
>
> Ævar Arnfjörð Bjarmason (5):
> wrapper.c: add x{un,}setenv(), and use xsetenv() in environment.c
> environment.c: remove test-specific "ignore_untracked..." variable
> read-cache & fetch-negotiator: check "enum" values in switch()
> repo-settings.c: simplify the setup
> repository.h: don't use a mix of int and bitfields
This version looks good to me.
Thanks,
-Stolee
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v4 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc.
2021-09-21 15:58 ` [PATCH v4 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc Derrick Stolee
@ 2021-09-21 20:46 ` Taylor Blau
2021-09-22 20:23 ` Junio C Hamano
0 siblings, 1 reply; 34+ messages in thread
From: Taylor Blau @ 2021-09-21 20:46 UTC (permalink / raw)
To: Derrick Stolee
Cc: Ævar Arnfjörð Bjarmason, git, Junio C Hamano,
Derrick Stolee, Jeff Hostetler, Patrick Steinhardt, Jeff King,
Taylor Blau
On Tue, Sep 21, 2021 at 11:58:34AM -0400, Derrick Stolee wrote:
> On 9/21/2021 9:12 AM, Ævar Arnfjörð Bjarmason wrote:
> > A hopefully final re-roll addressing Taylor's v3 review, except for
> > the suggestion (that I read as) perhaps retaining the test-only code,
> > which I've decided not to do per
> > http://lore.kernel.org/git/87tuieakms.fsf@evledraar.gmail.com
> >
> > [...]
>
> This version looks good to me.
Likewise. Thanks Ævar for addressing my review.
Reviewed-by: Taylor Blau <me@ttaylorr.com>
Thanks,
Taylor
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v4 0/5] repo-settings.c: refactor for clarity, get rid of hacks etc.
2021-09-21 20:46 ` Taylor Blau
@ 2021-09-22 20:23 ` Junio C Hamano
0 siblings, 0 replies; 34+ messages in thread
From: Junio C Hamano @ 2021-09-22 20:23 UTC (permalink / raw)
To: Taylor Blau
Cc: Derrick Stolee, Ævar Arnfjörð Bjarmason, git,
Derrick Stolee, Jeff Hostetler, Patrick Steinhardt, Jeff King
Taylor Blau <me@ttaylorr.com> writes:
> On Tue, Sep 21, 2021 at 11:58:34AM -0400, Derrick Stolee wrote:
>> On 9/21/2021 9:12 AM, Ævar Arnfjörð Bjarmason wrote:
>> > A hopefully final re-roll addressing Taylor's v3 review, except for
>> > the suggestion (that I read as) perhaps retaining the test-only code,
>> > which I've decided not to do per
>> > http://lore.kernel.org/git/87tuieakms.fsf@evledraar.gmail.com
>> >
>> > [...]
>>
>> This version looks good to me.
>
> Likewise. Thanks Ævar for addressing my review.
Yeah, I think this one is good enough for 'next'.
Thanks, all.
^ permalink raw reply [flat|nested] 34+ messages in thread