Re: [PATCH 2/3] commit: write commit-graph with bloom filters

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 11, 2020, 21:57 UTC
Message-ID: <xmqqa73hheip.fsf@gitster.c.googlers.com>
In-Reply-To: <bb5ce39d0283f14095d87dd0645e641ae799f16c.1586566981.git.gitgitgadget@gmail.com>

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 12 quoted lines

> diff --git a/builtin/commit.c b/builtin/commit.c
> index d3e7781e658..d2fdfdc4363 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1701,7 +1701,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  		      "not exceeded, and then \"git restore --staged :/\" to recover."));
>  
>  	if (git_env_bool(GIT_TEST_COMMIT_GRAPH, 0) &&
> -	    write_commit_graph_reachable(the_repository->objects->odb, 0, NULL))
> +	    write_commit_graph_reachable(the_repository->objects->odb,
> +					 git_env_bool(GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS, 0) ? COMMIT_GRAPH_WRITE_BLOOM_FILTERS : 0,
> +					 NULL))

Yuck.  That is doubly mouthful.

I wonder how much value we are getting by having this call here in the first place. This function being cmd_commit(), by definition we won't be updating the graph file unless the test script does "git commit". We won't update the graph file upon "git rebase", "git merge", "git pull", "git fetch", etc., so it is not like with this hack, the test coverage for various traversal using the graph file would magically be perfect. We'd still need an explicit call to "commit-graph write" at strategic places in the test scripts, no?

How are we testing with and without bitmaps, and do we have a kludge like this one for the feature, or do we use a different mechanism to help writing tests easier to gain better coverage?

In any case, I can see why we want a separate knob specific to the CHANGED_PATHS until the path filter part becomes as stable as the rest of the graph file, but after some time, we should remove that knob, and at that point, we always use the path filter whenever we use commit-graph, so that we won't have to suffer from the ugliness.

Wait. I wonder if we can tweak the arrangement a bit so that this layer does not need to know any more than GIT_TEST_COMMIT_GRAPH?

For example, in commit-graph.c::write_commit_graph(), can we test the CHANGED_PATHS environment variable and flip the .changed_paths bit, no matter what the 'flags' argument to the function says?

Thanks.

>  		return 1;
>  
>  	repo_rerere(the_repository, 0);