On 20/09/2021 14:30, Ævar Arnfjörð Bjarmason wrote:
On Mon, Sep 20 2021, Phillip Wood wrote:
On 20/09/2021 02:25, brian m. carlson wrote:
On 2021-09-20 at 00:42:57, Junio C Hamano wrote:
"Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
diff --git a/repo-settings.c b/repo-settings.c
index 309577f6bc..d00b675687 100644
--- a/repo-settings.c
+++ b/repo-settings.c
@@ -2,6 +2,8 @@
#include "config.h"
#include "repository.h"
+#define UPDATE_DEFAULT_BOOL(s,v) do { if (s == -1) { s = v; } }
while(0)
+
void prepare_repo_settings(struct repository *r)
{
int value;
@@ -16,6 +18,8 @@ void prepare_repo_settings(struct repository *r)
r->settings.core_commit_graph = 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.gc_write_commit_graph, 1);
This is a "review comment" that is more than 2 years late X-<, but I
noticed that this is used to muck with a structure that was
initialized by filling it with \0377 bytes.
+ /* Defaults */
+ memset(&r->settings, -1, sizeof(r->settings));
but the structure is is full of "int" and "enum", so apparently this
works only on 2's complement architecture.
This statement is true, but are there systems capable of running Git
which don't use two's complement? Rust requires two's complement signed
integers, and there's a proposal[0] to the C++ working group to only
support two's complement because "[t]o the author’s knowledge no modern
machine uses both C++ and a signed integer representation other than
two’s complement". That proposal goes on to note that none of MSVC,
GCC, or LLVM support other options.
A similar proposal [1] is included in the draft of the next C standard
[2]. As integer representation is implementation defined I believe
this code has well defined behavior on 2's complement
implementations. If an enum has no negative members then the compiler
may choose an unsigned representation but even then the comparison to
-1 is well defined.
That's informative, thanks.
In this case I'm pretty sure the enums all have -1
as a member so are signed. Using memset() to initialize the struct
eases future maintenance when members are added or removed and seems
to me to be a sensible design choice.
It's really not sensible at all in this particular case, as I think my
[1] which gets rid of the pattern convincingly argues.
I meant that it was a sensible way to initialize all the struct members
to -1, I did not mean to comment either way on whether such an
initialization was sensible. If we can just store the default and then
update from the user's config that sounds sensible.
Best Wishes
Phillip
I.e. the only reason it had a memset() of -1 after we'd already memset
it to 0 was because the function was tripping over itself and setting
defaults in the wrong order for no good reason.
I.e. it was doing things like (pseudocode);
memset(&data, -1, ...)
if_config_is_true_set("x.y", data.x_y);
if (data.x_y == -1)
data.x_y = x_y_default();
When we can instead just do:
data.x_y = x_y_default();
set_if_cfg_key_exists("x.y", &data.x_y);
Which is how we e.g. handle options parsing, we have hardcoded defaults,
then read defaults from config, then set options, in that order.
We don't set options, then check if each value is still -1, if so read
config etc. Just read them in priority order, doing it any other way is
just make-work for something that's the equivalent of a simple
short-circuit || operation.
Anyway, there are other cases where we need to read something and
distinguish e.g. false/true from "unset", and there a -1,0,1 tri-state
serves us well.
But even in those cases what repo-settings.c was doing of memsetting the
entire struct to -1 (2's compliment aside) just makes for needlessly
hard to read code.
If we've got some members that need -1 defaults we should instead have
that in an *_INIT macro or equivalent. The pre-[1] repo-settings.c also
has code like this pseudocode:
data.a_b = -1; /* default for a bi-state, not tri-state variable */
set_if_cfg_key_exists("a.b", &data.a_b);
if (data.a_b == -1)
data.a_b = 1; /* on by default */
Which, urm, you can just do as:
data.a_b = 1; /* on by default */
set_if_cfg_key_exists("a.b", &data.a_b);
I.e. the setup for things that never wanted or cared about being set to
-1 was complicated by them needing to un-set themselves from a -1
default they never wanted.
Thus the anti-pattern, yes set defaults for some members to -1, but not
the entire struct. The only value we should memset a whole bag-of-stuff
config struct to is 0, as that's the only sensible default & plays well
with other C semantics.
1. https://lore.kernel.org/git/patch-v3-4.5-28286a61162-20210919T084703Z-avarab@xxxxxxxxx/
Best Wishes
Phillip
[1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2412.pdf
[2] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2596.pdf
Personally I am not aware of any modern processor which provides signed
integer types using other than two's complement.
[0]
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0907r4.html
