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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 26, 2021, 22:39 UTC
Message-ID: <xmqqy2faqwr0.fsf@gitster.g>
In-Reply-To: <87r1l27rae.fsf@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 18 quoted lines

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

Show 11 quoted lines

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