[PATCH] commit-graph: warn about...

[PATCH] commit-graph: warn about incompatibilities only when trying to write

From: Johannes Schindelin via GitGitGadget <hidden>
Date: 2021-02-26 14:51:43

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.

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, 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))
 		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
+'
+
 test_done

base-commit: c85eec7fc37e1ca79072f263ae6ea1ee305ba38c
-- 
gitgitgadget

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

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

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, 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

 		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 {

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help