Message ID | 651e2d526b2a3f6c63c64cfe1f049eef94d39c07.1565721461.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Create 'feature.*' config area and some centralized config parsing | expand |
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> 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. struct repo_settings { int initialized; int core_commit_graph; int commit_graph_read_changed_paths; int gc_write_commit_graph; int fetch_write_commit_graph; int index_version; enum untracked_cache_setting core_untracked_cache; int pack_use_sparse; enum fetch_negotiation_setting fetch_negotiation_algorithm; int core_multi_pack_index; unsigned command_requires_full_index:1, sparse_index:1; }; I see that the earliest iteration of this series [*1*] set the default explicitly using assignments of the correct types, like this: +void prepare_repo_settings(struct repository *r) +{ + if (r->settings) + return; + + r->settings = xmalloc(sizeof(*r->settings)); + + /* Defaults */ + r->settings->core_commit_graph = -1; + r->settings->gc_write_commit_graph = -1; + r->settings->pack_use_sparse = -1; + r->settings->index_version = -1; + ... which I think should be a reasonable starting point to fix the current code. Another thing I noticed is that while it may have been only for setting the default value for a boolean variable initially, other changes abuse the macro to set an arbitrary integer values to integer members of the structure, e.g. c6cc4c5a (repo-settings: create feature.manyFiles setting, 2019-08-13) sets 4 to the index_version (naturally, the choice between 0 and 1 does not make much sense for the member), and ad0fb659 (repo-settings: parse core.untrackedCache, 2019-08-13) stuffs UNTRACKED_CACHE_* enum to core_untracked_cache. The UPDATE_DEFAULT_BOOL() macro should be renamed to UPDATE_DEFAULT_INT() at least, I would think, to save readers from confusion. Thanks. [Reference] *1* https://lore.kernel.org/git/72f652b89c71526cc423e7812de66f41a079f181.1563818059.git.gitgitgadget@gmail.com/
On Sun, Sep 19 2021, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> 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. > > struct repo_settings { > int initialized; > > int core_commit_graph; > int commit_graph_read_changed_paths; > int gc_write_commit_graph; > int fetch_write_commit_graph; > > int index_version; > enum untracked_cache_setting core_untracked_cache; > > int pack_use_sparse; > enum fetch_negotiation_setting fetch_negotiation_algorithm; > > int core_multi_pack_index; > > unsigned command_requires_full_index:1, > sparse_index:1; > }; > > I see that the earliest iteration of this series [*1*] set the > default explicitly using assignments of the correct types, like > this: > > > +void prepare_repo_settings(struct repository *r) > +{ > + if (r->settings) > + return; > + > + r->settings = xmalloc(sizeof(*r->settings)); > + > + /* Defaults */ > + r->settings->core_commit_graph = -1; > + r->settings->gc_write_commit_graph = -1; > + r->settings->pack_use_sparse = -1; > + r->settings->index_version = -1; > + ... > > which I think should be a reasonable starting point to fix the > current code. > > Another thing I noticed is that while it may have been only for > setting the default value for a boolean variable initially, other > changes abuse the macro to set an arbitrary integer values to > integer members of the structure, e.g. c6cc4c5a (repo-settings: > create feature.manyFiles setting, 2019-08-13) sets 4 to the > index_version (naturally, the choice between 0 and 1 does not make > much sense for the member), and ad0fb659 (repo-settings: parse > core.untrackedCache, 2019-08-13) stuffs UNTRACKED_CACHE_* enum to > core_untracked_cache. The UPDATE_DEFAULT_BOOL() macro should be > renamed to UPDATE_DEFAULT_INT() at least, I would think, to save > readers from confusion. Yes this is all a bit weird and/or broken, but I'm a bit perplexed at this reply to a 2+ year old E-Mail given my outstanding series to fix all these issues you've noted here[1] posted in the last few days, and you having read (at least part of) it[1]. But then again, the last patch you left a comment on was 3/5. It's 4/5 that fixes all the issues you note above[2] :) The macro is gone, so is the memset to -1 and other weird emergent behavior. We can rely on repo_init() having zero'd the structure for us, and we just proceed to set sensible defaults in a way that doesn't stomp over the types in the struct. 1. https://lore.kernel.org/git/cover-v3-0.5-00000000000-20210919T084703Z-avarab@gmail.com/ 2. https://lore.kernel.org/git/patch-v3-4.5-28286a61162-20210919T084703Z-avarab@gmail.com/ > [Reference] > > *1* https://lore.kernel.org/git/72f652b89c71526cc423e7812de66f41a079f181.1563818059.git.gitgitgadget@gmail.com/
On 2021-09-20 at 00:42:57, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> 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. 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
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@gmail.com> 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. 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. 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 >
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@gmail.com> 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.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@gmail.com/ > > 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 >>
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@gmail.com> 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@gmail.com/ > >> >> 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 >>> >
On Mon, Sep 20, 2021 at 03:30:38PM +0200, Ævar Arnfjörð Bjarmason wrote: > 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. FWIW, I agree. I had to scratch my head for a moment at why a memset of "-1" would work at all on multi-byte types. I think it's better avoided in the name of readability and obviousness, not to mention the trap it leaves for items that don't sensibly initialize with it (like, say, pointers). As an aside, memset to 0 is _also_ undefined for pointers, but we long ago decided not to care, as no real-world systems have a problem with this. -Peff
diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt index 75538d27e7..e66d79fd76 100644 --- a/Documentation/config/core.txt +++ b/Documentation/config/core.txt @@ -577,7 +577,7 @@ the `GIT_NOTES_REF` environment variable. See linkgit:git-notes[1]. core.commitGraph:: If true, then git will read the commit-graph file (if it exists) - to parse the graph structure of commits. Defaults to false. See + to parse the graph structure of commits. Defaults to true. See linkgit:git-commit-graph[1] for more information. core.useReplaceRefs:: diff --git a/Documentation/config/gc.txt b/Documentation/config/gc.txt index 02b92b18b5..00ea0a678e 100644 --- a/Documentation/config/gc.txt +++ b/Documentation/config/gc.txt @@ -63,7 +63,7 @@ gc.writeCommitGraph:: If true, then gc will rewrite the commit-graph file when linkgit:git-gc[1] is run. When using `git gc --auto` the commit-graph will be updated if housekeeping is - required. Default is false. See linkgit:git-commit-graph[1] + required. Default is true. See linkgit:git-commit-graph[1] for details. gc.logExpiry:: 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); if (!repo_config_get_bool(r, "index.version", &value)) r->settings.index_version = value; diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh index 5bd892f2f7..181ffa44e9 100755 --- a/t/t0410-partial-clone.sh +++ b/t/t0410-partial-clone.sh @@ -234,7 +234,7 @@ test_expect_success 'rev-list stops traversal at missing and promised commit' ' git -C repo config core.repositoryformatversion 1 && git -C repo config extensions.partialclone "arbitrary string" && - GIT_TEST_COMMIT_GRAPH=0 git -C repo rev-list --exclude-promisor-objects --objects bar >out && + GIT_TEST_COMMIT_GRAPH=0 git -C repo -c core.commitGraph=false rev-list --exclude-promisor-objects --objects bar >out && grep $(git -C repo rev-parse bar) out && ! grep $FOO out ' diff --git a/t/t5307-pack-missing-commit.sh b/t/t5307-pack-missing-commit.sh index dacb440b27..f4338abb78 100755 --- a/t/t5307-pack-missing-commit.sh +++ b/t/t5307-pack-missing-commit.sh @@ -24,11 +24,11 @@ test_expect_success 'check corruption' ' ' test_expect_success 'rev-list notices corruption (1)' ' - test_must_fail env GIT_TEST_COMMIT_GRAPH=0 git rev-list HEAD + test_must_fail env GIT_TEST_COMMIT_GRAPH=0 git -c core.commitGraph=false rev-list HEAD ' test_expect_success 'rev-list notices corruption (2)' ' - test_must_fail env GIT_TEST_COMMIT_GRAPH=0 git rev-list --objects HEAD + test_must_fail env GIT_TEST_COMMIT_GRAPH=0 git -c core.commitGraph=false rev-list --objects HEAD ' test_expect_success 'pack-objects notices corruption' ' diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh index 03f45a1ed9..19aa40de15 100755 --- a/t/t5324-split-commit-graph.sh +++ b/t/t5324-split-commit-graph.sh @@ -8,6 +8,7 @@ GIT_TEST_COMMIT_GRAPH=0 test_expect_success 'setup repo' ' git init && git config core.commitGraph true && + git config gc.writeCommitGraph false && infodir=".git/objects/info" && graphdir="$infodir/commit-graphs" && test_oid_init @@ -332,6 +333,7 @@ test_expect_success 'split across alternate where alternate is not split' ' git clone --no-hardlinks . alt-split && ( cd alt-split && + rm -f .git/objects/info/commit-graph && echo "$(pwd)"/../.git/objects >.git/objects/info/alternates && test_commit 18 && git commit-graph write --reachable --split && diff --git a/t/t6011-rev-list-with-bad-commit.sh b/t/t6011-rev-list-with-bad-commit.sh index 545b461e51..bad02cf5b8 100755 --- a/t/t6011-rev-list-with-bad-commit.sh +++ b/t/t6011-rev-list-with-bad-commit.sh @@ -42,7 +42,7 @@ test_expect_success 'corrupt second commit object' \ ' test_expect_success 'rev-list should fail' ' - test_must_fail env GIT_TEST_COMMIT_GRAPH=0 git rev-list --all > /dev/null + test_must_fail env GIT_TEST_COMMIT_GRAPH=0 git -c core.commitGraph=false rev-list --all > /dev/null ' test_expect_success 'git repack _MUST_ fail' \