Re: [PATCH] DO-NOT-MERGE: write and read commit-graph always
On Wed, Jul 18, 2018 at 8:22 AM, Derrick Stolee wrote: > The following test fails because the repo has ambiguous merge-bases, and > the commit-graph changes the walk order so we select a different one. > This alters the resulting merge from the expected result. > > t6024-recursive-merge.sh, Test 4 > > The tests above are made to pass by deleting the commit-graph file > before the necessary steps. I know you meant for these to not be merged, but perhaps the test in t6024 could be made to be less stringent about order of merge bases. In particular, instead of expecting a certain sha1 to be at stage 2 and a different one to be at stage 3, it could just check that both shas appear in the `git ls-files --stage` output. > diff --git a/t/t6024-recursive-merge.sh b/t/t6024-recursive-merge.sh > index 3f59e58dfb..cec10983cd 100755 > --- a/t/t6024-recursive-merge.sh > +++ b/t/t6024-recursive-merge.sh > @@ -61,6 +61,7 @@ GIT_AUTHOR_DATE="2006-12-12 23:00:08" git commit -m F > ' > > test_expect_success "combined merge conflicts" " > + rm -rf .git/objects/info/commit-graph && > test_must_fail git merge -m final G > " > > -- > 2.18.0.118.gd4f65b8d14
Re: [PATCH] DO-NOT-MERGE: write and read commit-graph always
Stefan Beller writes: >> I wonder though if all those changes to the testsuite shouldn't be >> merged. > > I think Stolee doesn't want this to be merged after rereading > subject and the commit message. Yes, I understand that, and for the most part I agree with it. This commit main purpose is to thoroughly exercise the new feature. But I think that changes to the testsuite could have been extracted into separate commit, and merged (and only those changes). It could serve as note about the intent of the test. Well, perhaps after encapsulating it in some function with a good name... ;-) Anyway, this is a minor insignificant thing. -- Jakub Narębski
Re: [PATCH] DO-NOT-MERGE: write and read commit-graph always
> I wonder though if all those changes to the testsuite shouldn't be > merged. I think Stolee doesn't want this to be merged after rereading subject and the commit message. Thanks, Stefan
Re: [PATCH] DO-NOT-MERGE: write and read commit-graph always
Derrick Stolee writes: > This commit is not intended to be merged, but is only to create a test > environment to see what works with the commit-graph feature and what > does not. The tests that fail are primarily related to corrupting the > object store to remove a commit from visibility and testing that > rev-list fails -- except it doesn't when there is a commit-graph that > prevents parsing from the object database. The following tests will fail > with this commit, but are not "real" bugs: > > t0410-partial-clone.sh, Test 9 > t5307-pack-missing-commit.sh, Tests 3-4 > t6011-rev-list-with-bad-commit.sh, Test 4 > > The following test fails because the repo has ambiguous merge-bases, and > the commit-graph changes the walk order so we select a different one. > This alters the resulting merge from the expected result. > > t6024-recursive-merge.sh, Test 4 > > The tests above are made to pass by deleting the commit-graph file > before the necessary steps. > > Signed-off-by: Derrick Stolee > --- > builtin/commit.c| 2 ++ > builtin/gc.c| 3 +-- > commit-graph.c | 11 --- > t/t0410-partial-clone.sh| 1 + > t/t5307-pack-missing-commit.sh | 2 ++ > t/t6011-rev-list-with-bad-commit.sh | 1 + > t/t6024-recursive-merge.sh | 1 + > 7 files changed, 8 insertions(+), 13 deletions(-) > > diff --git a/builtin/commit.c b/builtin/commit.c > index 158e3f843a..acc31252a9 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -33,6 +33,7 @@ > #include "sequencer.h" > #include "mailmap.h" > #include "help.h" > +#include "commit-graph.h" > > static const char * const builtin_commit_usage[] = { > N_("git commit [] [--] ..."), > @@ -1652,6 +1653,7 @@ int cmd_commit(int argc, const char **argv, const char > *prefix) >"not exceeded, and then \"git reset HEAD\" to recover.")); > > rerere(0); > + write_commit_graph_reachable(get_object_directory(), 1); > run_command_v_opt(argv_gc_auto, RUN_GIT_CMD); > run_commit_hook(use_editor, get_index_file(), "post-commit", NULL); > if (amend && !no_post_rewrite) { This is certainly not for merging. > diff --git a/builtin/gc.c b/builtin/gc.c > index e103f0f85d..60ab773087 100644 > --- a/builtin/gc.c > +++ b/builtin/gc.c > @@ -41,7 +41,7 @@ static int aggressive_depth = 50; > static int aggressive_window = 250; > static int gc_auto_threshold = 6700; > static int gc_auto_pack_limit = 50; > -static int gc_write_commit_graph; > +static int gc_write_commit_graph = 1; > static int detach_auto = 1; > static timestamp_t gc_log_expire_time; > static const char *gc_log_expire = "1.day.ago"; This is switching from default to off to default to on. I think with this feature we would default to off for a long time (wishful thinking: maybe automatically enabling it for large repositories, or if commit-graph file exists already?). > @@ -131,7 +131,6 @@ static void gc_config(void) > git_config_get_int("gc.aggressivedepth", &aggressive_depth); > git_config_get_int("gc.auto", &gc_auto_threshold); > git_config_get_int("gc.autopacklimit", &gc_auto_pack_limit); > - git_config_get_bool("gc.writecommitgraph", &gc_write_commit_graph); > git_config_get_bool("gc.autodetach", &detach_auto); > git_config_get_expiry("gc.pruneexpire", &prune_expire); > git_config_get_expiry("gc.worktreepruneexpire", > &prune_worktrees_expire); This is certainly not for merging, as it disables gc.writeCommitGraph config option entirely. > diff --git a/commit-graph.c b/commit-graph.c > index 237d4e7d1b..ed0d27c12e 100644 > --- a/commit-graph.c > +++ b/commit-graph.c > @@ -227,22 +227,11 @@ static int prepare_commit_graph(struct repository *r) > { > struct alternate_object_database *alt; > char *obj_dir; > - int config_value; > > if (r->objects->commit_graph_attempted) > return !!r->objects->commit_graph; > r->objects->commit_graph_attempted = 1; > > - if (repo_config_get_bool(r, "core.commitgraph", &config_value) || > - !config_value) > - /* > - * This repository is not configured to use commit graphs, so > - * do not load one. (But report commit_graph_attempted anyway > - * so that commit graph loading is not attempted again for this > - * repository.) > - */ > - return 0; > - > if (!commit_graph_compatible(r)) > return 0; > This is certainly not for merging, as it disables core.commitGraph config option entirely. > diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh > index 4984ca583d..c235672b03 100755 > --- a/t/t0410-partial-clone.sh > +++ b/t/t0410-partial-clone.sh > @@ -181,6 +181,7 @@ test_expect_success 'rev-list stops traversal at missing > and promised commit' ' > > git -C repo config core.repositoryformatversion 1 && >
[PATCH] DO-NOT-MERGE: write and read commit-graph always
This commit is not intended to be merged, but is only to create a test environment to see what works with the commit-graph feature and what does not. The tests that fail are primarily related to corrupting the object store to remove a commit from visibility and testing that rev-list fails -- except it doesn't when there is a commit-graph that prevents parsing from the object database. The following tests will fail with this commit, but are not "real" bugs: t0410-partial-clone.sh, Test 9 t5307-pack-missing-commit.sh, Tests 3-4 t6011-rev-list-with-bad-commit.sh, Test 4 The following test fails because the repo has ambiguous merge-bases, and the commit-graph changes the walk order so we select a different one. This alters the resulting merge from the expected result. t6024-recursive-merge.sh, Test 4 The tests above are made to pass by deleting the commit-graph file before the necessary steps. Signed-off-by: Derrick Stolee --- builtin/commit.c| 2 ++ builtin/gc.c| 3 +-- commit-graph.c | 11 --- t/t0410-partial-clone.sh| 1 + t/t5307-pack-missing-commit.sh | 2 ++ t/t6011-rev-list-with-bad-commit.sh | 1 + t/t6024-recursive-merge.sh | 1 + 7 files changed, 8 insertions(+), 13 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index 158e3f843a..acc31252a9 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -33,6 +33,7 @@ #include "sequencer.h" #include "mailmap.h" #include "help.h" +#include "commit-graph.h" static const char * const builtin_commit_usage[] = { N_("git commit [] [--] ..."), @@ -1652,6 +1653,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix) "not exceeded, and then \"git reset HEAD\" to recover.")); rerere(0); + write_commit_graph_reachable(get_object_directory(), 1); run_command_v_opt(argv_gc_auto, RUN_GIT_CMD); run_commit_hook(use_editor, get_index_file(), "post-commit", NULL); if (amend && !no_post_rewrite) { diff --git a/builtin/gc.c b/builtin/gc.c index e103f0f85d..60ab773087 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -41,7 +41,7 @@ static int aggressive_depth = 50; static int aggressive_window = 250; static int gc_auto_threshold = 6700; static int gc_auto_pack_limit = 50; -static int gc_write_commit_graph; +static int gc_write_commit_graph = 1; static int detach_auto = 1; static timestamp_t gc_log_expire_time; static const char *gc_log_expire = "1.day.ago"; @@ -131,7 +131,6 @@ static void gc_config(void) git_config_get_int("gc.aggressivedepth", &aggressive_depth); git_config_get_int("gc.auto", &gc_auto_threshold); git_config_get_int("gc.autopacklimit", &gc_auto_pack_limit); - git_config_get_bool("gc.writecommitgraph", &gc_write_commit_graph); git_config_get_bool("gc.autodetach", &detach_auto); git_config_get_expiry("gc.pruneexpire", &prune_expire); git_config_get_expiry("gc.worktreepruneexpire", &prune_worktrees_expire); diff --git a/commit-graph.c b/commit-graph.c index 237d4e7d1b..ed0d27c12e 100644 --- a/commit-graph.c +++ b/commit-graph.c @@ -227,22 +227,11 @@ static int prepare_commit_graph(struct repository *r) { struct alternate_object_database *alt; char *obj_dir; - int config_value; if (r->objects->commit_graph_attempted) return !!r->objects->commit_graph; r->objects->commit_graph_attempted = 1; - if (repo_config_get_bool(r, "core.commitgraph", &config_value) || - !config_value) - /* -* This repository is not configured to use commit graphs, so -* do not load one. (But report commit_graph_attempted anyway -* so that commit graph loading is not attempted again for this -* repository.) -*/ - return 0; - if (!commit_graph_compatible(r)) return 0; diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh index 4984ca583d..c235672b03 100755 --- a/t/t0410-partial-clone.sh +++ b/t/t0410-partial-clone.sh @@ -181,6 +181,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" && + rm -rf repo/.git/objects/info/commit-graph && git -C repo 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 ae52a1882d..0bb54ae227 100755 --- a/t/t5307-pack-missing-commit.sh +++ b/t/t5307-pack-missing-commit.sh @@ -24,10 +24,12 @@ test_expect_success 'check corruption' ' ' test_expect_success 'rev-list notices corruption (1)' ' + rm -rf .git/objects/info/commit-graph &&