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

From: Johannes Schindelin <[email protected]>

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 <[email protected]>

Signed-off-by Johannes Schindelin <[email protected]>

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

As pointed out by Ævar in
https://lore.kernel.org/git/[email protected], 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(-)

commit-graph.c +10 -6

index 8fd480434353..245b7108e0d1 100644
@@ -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;
 	}
 

commit-graph.c +1 -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));

t/t5318-commit-graph.sh +1 -0

index 2ed0c1544da1..2699c55e9a93 100755
@@ -741,4 +741,17 @@ test_expect_success 'corrupt commit-graph write (missing tree)' '
 	)
 '
 
+test_expect_success 'warn about incompatibilities (only) when writing' '

Quoted 10 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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.

Quoted 1 line

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...
...
...doing this will cause "git gc --auto" to run into persistent warnings. See a WIP patch-on-top in [1] below...

builtin/gc.c +1 -1

-			if (!quiet)
+			if (flags & COMMIT_GRAPH_WRITE_WARNINGS)
 				warning(_("repository contains replace "

"objects; skipping commit-graph"));

Quoted 12 lines · Ævar Arnfjörð Bjarmason

Quoted 10 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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.

Quoted 6 lines

Quoted 1 line

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...
...
...doing this will cause "git gc --auto" to run into persistent warnings. See a WIP patch-on-top in [1] below...

Thanks for an input.

builtin/gc.c int cmd_gc(int argc, const char **argv, const char *prefix) · line 597

Quoted 11 lines

builtin/gc.c +1 -1

index 64f2b52d6e2..9109898eacb 100644
@@ -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;

Leftover debugging cruft?

Quoted 4 lines

builtin/gc.c +1 -1

-			if (!quiet)
+			if (flags & COMMIT_GRAPH_WRITE_WARNINGS)
 				warning(_("repository contains replace "

"objects; skipping commit-graph"));

I would have expected the code would arrange to make the bit cleared in the "flags" that would be passed around to the next call that inspects the same bit, so that we won't need to see repeated warning()s, but I do not see where it is done.

I am tempted to say that we should revert c85eec7f (commit-graph:
when incompatible with graphs, indicate why, 2021-02-11) for the upcoming release. That would give us enough time to come up with and cook the solution in 'next' in the meantime.

Quoted 4 lines

I am tempted to say that we should revert c85eec7f (commit-graph:
when incompatible with graphs, indicate why, 2021-02-11) for the upcoming release. That would give us enough time to come up with and cook the solution in 'next' in the meantime.

That's probably sensible.

Also, I noticed that we went through this whole saga in the past, see 25575015ca (repack: silence warnings when auto-enabled bitmaps cannot be built, 2019-07-31), including breaking background gc.

Quoted 4 lines · Junio C Hamano

I am tempted to say that we should revert c85eec7f (commit-graph:
when incompatible with graphs, indicate why, 2021-02-11) for the upcoming release. That would give us enough time to come up with and cook the solution in 'next' in the meantime.

That's probably sensible.

Also, I noticed that we went through this whole saga in the past, see 25575015ca (repack: silence warnings when auto-enabled bitmaps cannot be built, 2019-07-31), including breaking background gc.

Hi,

Quoted 8 lines · Ævar Arnfjörð Bjarmason

Quoted 4 lines · Junio C Hamano

I am tempted to say that we should revert c85eec7f (commit-graph:
when incompatible with graphs, indicate why, 2021-02-11) for the upcoming release. That would give us enough time to come up with and cook the solution in 'next' in the meantime.

That's probably sensible.

I agree. This close to v2.31.0, I do not want to rush things.

Quoted 3 lines

Also, I noticed that we went through this whole saga in the past, see 25575015ca (repack: silence warnings when auto-enabled bitmaps cannot be built, 2019-07-31), including breaking background gc.

Sounds like it.

Thank you for your valuable feedback!

Ciao,
Dscho