Re: [PATCH v3 0/2] commit-graph: detect commits missing in ODB

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 5402 bytes --]

On Wed, Nov 01, 2023 at 11:06:05AM +0900, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> >> Thanks, the range-diff here looks exactly as expected. Thanks for
> >> working on this, this version LGTM.
> >
> > OK, I'd like a version as incremental to v2 (since it already is in
> > 'next') that results in the same tree state as v3 then.
> >
> > Thanks for working on it, and reviewing it.
> 
> In the meantime, here is a mechanically produced incremental I'll
> tentatively queue.  Hopefully I did not screw up while generating
> it.
> 
> Thanks.

Ah, sorry, didn't notice it was in 'next' already. Anyway, the diff
below looks good to me, thanks!

Patrick

> --- >8 ---
> From: Patrick Steinhardt <ps@pks.im>
> Date: Tue, 31 Oct 2023 08:16:09 +0100
> Subject: [PATCH] commit-graph: clarify GIT_COMMIT_GRAPH_PARANOIA documentation
> 
> In response to reviews of the previous round that has already hit
> 'next', clarify the help text for GIT_COMMIT_GRAPH_PARANOIA and
> rename object_paranoia variable to commit_graph_paranoia for
> consistency.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/git.txt   | 15 ++++++++-------
>  commit-graph.c          |  8 ++++----
>  commit.c                |  8 ++++----
>  t/t5318-commit-graph.sh |  2 +-
>  4 files changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 22c2b537aa..3bac24cf8a 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -912,13 +912,14 @@ for full details.
>  	useful when trying to salvage data from a corrupted repository.
>  
>  `GIT_COMMIT_GRAPH_PARANOIA`::
> -	If this Boolean environment variable is set to false, ignore the
> -	case where commits exist in the commit graph but not in the
> -	object database. Normally, Git will check whether commits loaded
> -	from the commit graph exist in the object database to avoid
> -	issues with stale commit graphs, but this check comes with a
> -	performance penalty. The default is `1` (i.e., be paranoid about
> -	stale commits in the commit graph).
> +	When loading a commit object from the commit-graph, Git performs an
> +	existence check on the object in the object database. This is done to
> +	avoid issues with stale commit-graphs that contain references to
> +	already-deleted commits, but comes with a performance penalty.
> ++
> +The default is "true", which enables the aforementioned behavior.
> +Setting this to "false" disables the existence check. This can lead to
> +a performance improvement at the cost of consistency.
>  
>  `GIT_ALLOW_PROTOCOL`::
>  	If set to a colon-separated list of protocols, behave as if
> diff --git a/commit-graph.c b/commit-graph.c
> index 376f59af73..b37fdcb214 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -907,18 +907,18 @@ int repo_find_commit_pos_in_graph(struct repository *r, struct commit *c,
>  
>  struct commit *lookup_commit_in_graph(struct repository *repo, const struct object_id *id)
>  {
> -	static int object_paranoia = -1;
> +	static int commit_graph_paranoia = -1;
>  	struct commit *commit;
>  	uint32_t pos;
>  
> -	if (object_paranoia == -1)
> -		object_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
> +	if (commit_graph_paranoia == -1)
> +		commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
>  
>  	if (!prepare_commit_graph(repo))
>  		return NULL;
>  	if (!search_commit_pos_in_graph(id, repo->objects->commit_graph, &pos))
>  		return NULL;
> -	if (object_paranoia && !has_object(repo, id, 0))
> +	if (commit_graph_paranoia && !has_object(repo, id, 0))
>  		return NULL;
>  
>  	commit = lookup_commit(repo, id);
> diff --git a/commit.c b/commit.c
> index 7399e90212..8405d7c3fc 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -574,12 +574,12 @@ int repo_parse_commit_internal(struct repository *r,
>  	if (item->object.parsed)
>  		return 0;
>  	if (use_commit_graph && parse_commit_in_graph(r, item)) {
> -		static int object_paranoia = -1;
> +		static int commit_graph_paranoia = -1;
>  
> -		if (object_paranoia == -1)
> -			object_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
> +		if (commit_graph_paranoia == -1)
> +			commit_graph_paranoia = git_env_bool(GIT_COMMIT_GRAPH_PARANOIA, 1);
>  
> -		if (object_paranoia && !has_object(r, &item->object.oid, 0)) {
> +		if (commit_graph_paranoia && !has_object(r, &item->object.oid, 0)) {
>  			unparse_commit(r, &item->object.oid);
>  			return quiet_on_missing ? -1 :
>  				error(_("commit %s exists in commit-graph but not in the object database"),
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 55e3c7ec78..2c62b91ef9 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -847,7 +847,7 @@ test_expect_success 'stale commit cannot be parsed when traversing graph' '
>  		test_commit C &&
>  		git commit-graph write --reachable &&
>  
> -		# Corrupt the repository by deleting the intermittent commit
> +		# Corrupt the repository by deleting the intermediate commit
>  		# object. Commands should notice that this object is absent and
>  		# thus that the repository is corrupt even if the commit graph
>  		# exists.
> -- 
> 2.42.0-530-g692be87cbb
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]