Re: [PATCH] DO-NOT-MERGE: write and read commit-graph always
From: Jakub Narebski <hidden>
Date: 2018-07-30 14:40:06
Derrick Stolee [off-list ref] writes:
quoted hunk
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 <redacted> --- 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 [<options>] [--] <pathspec>..."),@@ -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.
quoted hunk
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?).
quoted hunk
@@ -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.
quoted hunk
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.
quoted hunk
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 outdiff --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 && test_must_fail git rev-list HEAD ' test_expect_success 'rev-list notices corruption (2)' ' + rm -rf .git/objects/info/commit-graph && test_must_fail git rev-list --objects HEAD 'diff --git a/t/t6011-rev-list-with-bad-commit.sh b/t/t6011-rev-list-with-bad-commit.sh index e51eb41f4b..9c9cc4d540 100755 --- a/t/t6011-rev-list-with-bad-commit.sh +++ b/t/t6011-rev-list-with-bad-commit.sh@@ -43,6 +43,7 @@ test_expect_success 'corrupt second commit object' \ test_expect_success 'rev-list should fail' \ ' + rm -rf .git/objects/info/commit-graph && test_must_fail git rev-list --all > /dev/null '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 "
I wonder though if all those changes to the testsuite shouldn't be merged. Regards, -- Jakub Narębski