Re: [PATCH] commit-graph: warn about incompatibilities only when trying to write
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-02-26 16:01:28
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Fri, Feb 26 2021, Johannes Schindelin via GitGitGadget wrote:
From: Johannes Schindelin <redacted> In c85eec7fc37 (commit-graph: when incompatible with graphs, indicate why, 2021-02-11), we started warning the user if they tried to write a commit-graph in a shallow repository, or one containing replace objects. However, this patch was a bit overzealous, as Git now _also_ warns when merely checking whether there _is_ a usable commit graph, not only when writing one. Let's suppress that warning unless we want to write a commit-graph.
Ah, so that's what it's for, as I suspected :) Unfortunately...
quoted hunk ↗ jump to hunk
Reported-by: Ævar Arnfjörð Bjarmason <redacted> Signed-off-by: Johannes Schindelin <redacted> --- commit-graph: warn about incompatibilities only when trying to write As pointed out by Ævar in https://lore.kernel.org/git/87pn0o6y1e.fsf@evledraar.gmail.com (local), my recent patch to trigger warnings in repositories that are incompatible with the commit-graph was a bit too overzealous. Here is a fix for that. Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-888%2Fdscho%2Fwarn-a-little-less-if-commit-graph-is-skipped-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-888/dscho/warn-a-little-less-if-commit-graph-is-skipped-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/888 commit-graph.c | 20 ++++++++++++-------- t/t5318-commit-graph.sh | 13 +++++++++++++ 2 files changed, 25 insertions(+), 8 deletions(-)diff --git a/commit-graph.c b/commit-graph.c index 8fd480434353..245b7108e0d1 100644 --- a/commit-graph.c +++ b/commit-graph.c@@ -198,7 +198,7 @@ static struct commit_graph *alloc_commit_graph(void) extern int read_replace_refs; -static int commit_graph_compatible(struct repository *r) +static int commit_graph_compatible(struct repository *r, int quiet) { if (!r->gitdir) return 0;@@ -206,8 +206,9 @@ static int commit_graph_compatible(struct repository *r) if (read_replace_refs) { prepare_replace_object(r); if (hashmap_get_size(&r->objects->replace_map->map)) { - warning(_("repository contains replace objects; " - "skipping commit-graph")); + if (!quiet) + warning(_("repository contains replace " + "objects; skipping commit-graph")); return 0; } }@@ -215,12 +216,15 @@ static int commit_graph_compatible(struct repository *r) prepare_commit_graft(r); if (r->parsed_objects && (r->parsed_objects->grafts_nr || r->parsed_objects->substituted_parent)) { - warning(_("repository contains (deprecated) grafts; " - "skipping commit-graph")); + if (!quiet) + warning(_("repository contains (deprecated) grafts; " + "skipping commit-graph")); return 0; } if (is_repository_shallow(r)) { - warning(_("repository is shallow; skipping commit-graph")); + if (!quiet) + warning(_("repository is shallow; skipping " + "commit-graph")); return 0; }@@ -652,7 +656,7 @@ static int prepare_commit_graph(struct repository *r) */ return 0; - if (!commit_graph_compatible(r)) + if (!commit_graph_compatible(r, 1))
...doing this will cause "git gc --auto" to run into persistent warnings. See a WIP patch-on-top in [1] below...
quoted hunk ↗ jump to hunk
return 0; prepare_alt_odb(r);@@ -2123,7 +2127,7 @@ int write_commit_graph(struct object_directory *odb, warning(_("attempting to write a commit-graph, but 'core.commitGraph' is disabled")); return 0; } - if (!commit_graph_compatible(the_repository)) + if (!commit_graph_compatible(the_repository, 0)) return 0; ctx = xcalloc(1, sizeof(struct write_commit_graph_context));diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh index 2ed0c1544da1..2699c55e9a93 100755 --- a/t/t5318-commit-graph.sh +++ b/t/t5318-commit-graph.sh@@ -741,4 +741,17 @@ test_expect_success 'corrupt commit-graph write (missing tree)' ' ) ' +test_expect_success 'warn about incompatibilities (only) when writing' ' + git init warn && + test_commit -C warn initial && + test_commit -C warn second && + git -C warn replace --graft second && + test_config -C warn gc.writecommitgraph true && + + git -C warn gc 2>err && + test_i18ngrep "skipping commit-graph" err && + git -C warn rev-list -1 second 2>err && + test_i18ngrep ! "skipping commit-graph" err
...I think it makes sense to have a "test_line_count = 1" here for these sorts of warnings we know/suspect might come up N times if we don't carefully tweak things. I think (but have not tested) that we'll still write it 2 times if you have the graph configured to write on fetch, and we end up also running "gc" without gc.autoDetach. In any case, here's a WIP patch to fix/demonstrate the bug here, consider it to have my SOB (but no need to retain the authorship). Obviously needs amending (e.g. the "0 &&" case, just to reproduce the bug), and you might not want the s/int/enum/ cleanup while we're at it. Also makes sense to add to add this case to the "gc --auto" test, i.e. if we have writeCommitGraph=true && a --depth=1 repo do we have no gc.log at the end as we should.
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ecf85376050..73dacf927f9 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c@@ -1924,13 +1924,13 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) if (fetch_write_commit_graph > 0 || (fetch_write_commit_graph < 0 && the_repository->settings.fetch_write_commit_graph)) { - int commit_graph_flags = COMMIT_GRAPH_WRITE_SPLIT; + enum commit_graph_write_flags flags = COMMIT_GRAPH_WRITE_SPLIT | COMMIT_GRAPH_WRITE_WARNINGS; if (progress) - commit_graph_flags |= COMMIT_GRAPH_WRITE_PROGRESS; + flags |= COMMIT_GRAPH_WRITE_PROGRESS; write_commit_graph_reachable(the_repository->objects->odb, - commit_graph_flags, + flags, NULL); }
diff --git a/builtin/gc.c b/builtin/gc.c
index 64f2b52d6e2..9109898eacb 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c@@ -593,7 +593,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix) /* * Auto-gc should be least intrusive as possible. */ - if (!need_to_gc()) + if (0 && !need_to_gc()) return 0; if (!quiet) { if (detach_auto)
@@ -689,10 +689,15 @@ int cmd_gc(int argc, const char **argv, const char *prefix) } prepare_repo_settings(the_repository); - if (the_repository->settings.gc_write_commit_graph == 1) + if (the_repository->settings.gc_write_commit_graph) { + enum commit_graph_write_flags flags = 0; + if (!quiet && !daemonized) + flags |= (COMMIT_GRAPH_WRITE_PROGRESS | + COMMIT_GRAPH_WRITE_WARNINGS); + write_commit_graph_reachable(the_repository->objects->odb, - !quiet && !daemonized ? COMMIT_GRAPH_WRITE_PROGRESS : 0, - NULL); + flags, NULL); + } if (auto_gc && too_many_loose_objects()) warning(_("There are too many unreachable loose objects; "
diff --git a/commit-graph.c b/commit-graph.c
index 245b7108e0d..8afc75b2dbe 100644
--- a/commit-graph.c
+++ b/commit-graph.c@@ -198,7 +198,8 @@ static struct commit_graph *alloc_commit_graph(void) extern int read_replace_refs; -static int commit_graph_compatible(struct repository *r, int quiet) +static int commit_graph_compatible(struct repository *r, + enum commit_graph_write_flags flags) { if (!r->gitdir) return 0;
@@ -206,7 +207,7 @@ static int commit_graph_compatible(struct repository *r, int quiet) if (read_replace_refs) { prepare_replace_object(r); if (hashmap_get_size(&r->objects->replace_map->map)) { - if (!quiet) + if (flags & COMMIT_GRAPH_WRITE_WARNINGS) warning(_("repository contains replace " "objects; skipping commit-graph")); return 0;
@@ -216,13 +217,13 @@ static int commit_graph_compatible(struct repository *r, int quiet) prepare_commit_graft(r); if (r->parsed_objects && (r->parsed_objects->grafts_nr || r->parsed_objects->substituted_parent)) { - if (!quiet) + if (flags & COMMIT_GRAPH_WRITE_WARNINGS) warning(_("repository contains (deprecated) grafts; " "skipping commit-graph")); return 0; } if (is_repository_shallow(r)) { - if (!quiet) + if (flags & COMMIT_GRAPH_WRITE_WARNINGS) warning(_("repository is shallow; skipping " "commit-graph")); return 0;
@@ -2127,7 +2128,7 @@ int write_commit_graph(struct object_directory *odb, warning(_("attempting to write a commit-graph, but 'core.commitGraph' is disabled")); return 0; } - if (!commit_graph_compatible(the_repository, 0)) + if (!commit_graph_compatible(the_repository, flags)) return 0; ctx = xcalloc(1, sizeof(struct write_commit_graph_context));
diff --git a/commit-graph.h b/commit-graph.h
index f8e92500c6e..0d2a12a5e58 100644
--- a/commit-graph.h
+++ b/commit-graph.h@@ -95,9 +95,10 @@ struct bloom_filter_settings *get_bloom_filter_settings(struct repository *r); enum commit_graph_write_flags { COMMIT_GRAPH_WRITE_APPEND = (1 << 0), COMMIT_GRAPH_WRITE_PROGRESS = (1 << 1), - COMMIT_GRAPH_WRITE_SPLIT = (1 << 2), - COMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 3), - COMMIT_GRAPH_NO_WRITE_BLOOM_FILTERS = (1 << 4), + COMMIT_GRAPH_WRITE_WARNINGS = (1 << 2), + COMMIT_GRAPH_WRITE_SPLIT = (1 << 3), + COMMIT_GRAPH_WRITE_BLOOM_FILTERS = (1 << 4), + COMMIT_GRAPH_NO_WRITE_BLOOM_FILTERS = (1 << 5), }; enum commit_graph_split_flags {