diff mbox series

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

Message ID pull.888.git.1614351036334.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series commit-graph: warn about incompatibilities only when trying to write | expand

Commit Message

Johannes Schindelin Feb. 26, 2021, 2:50 p.m. UTC 
From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <avarab@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    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(-)


base-commit: c85eec7fc37e1ca79072f263ae6ea1ee305ba38c

Comments

Ævar Arnfjörð Bjarmason Feb. 26, 2021, 4 p.m. UTC | #1 
On Fri, Feb 26 2021, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> 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...

> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>     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...

>  		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 {
Junio C Hamano Feb. 26, 2021, 10:39 p.m. UTC | #2 
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Fri, Feb 26 2021, Johannes Schindelin via GitGitGadget wrote:
>
>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>
>> 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...
> ...
> ...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.

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

Leftover debugging cruft?

> -			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.
Ævar Arnfjörð Bjarmason Feb. 28, 2021, 4:20 p.m. UTC | #3 
On Fri, Feb 26 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> On Fri, Feb 26 2021, Johannes Schindelin via GitGitGadget wrote:
>>
>>> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>>>
>>> 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...
>> ...
>> ...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.
>
>> 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;
>
> Leftover debugging cruft?

Yeah, see the "Obviously needs amending" later in my E-Mail.

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

Right, we could do that, but I think the path down my "test_line_count =
1" is more useful.

I.e. it's pretty easy to reason about us not calling
write_commit_graph() twice in the same process, so I don't think such
flag clearing is needed.

But the case the user cares about is getting this warning twice when
they invoke git, but we spawn two processes.

> 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.
Junio C Hamano March 1, 2021, 5:18 p.m. UTC | #4 
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

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

Thanks and sigh ;-)
Johannes Schindelin March 1, 2021, 10:10 p.m. UTC | #5 
Hi,

On Sun, 28 Feb 2021, Ævar Arnfjörð Bjarmason wrote:

> On Fri, Feb 26 2021, Junio C Hamano wrote:
>
> > 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.

> 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
Junio C Hamano March 1, 2021, 10:21 p.m. UTC | #6 
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi,
>
> On Sun, 28 Feb 2021, Ævar Arnfjörð Bjarmason wrote:
>
>> On Fri, Feb 26 2021, Junio C Hamano wrote:
>>
>> > 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.
>
>> 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

Thanks, reverted and started today's integration cycle on top.
diff mbox series

Patch

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