Re: [PATCH v2 2/4] commit: write commit-graph with Bloom filters

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 14, 2020, 17:26 UTC
Message-ID: <xmqqa73e9dws.fsf@gitster.c.googlers.com>
In-Reply-To: <74e4e8d6-d401-081d-14cc-c4b6087bdeda@gmail.com>

Derrick Stolee <stolee@gmail.com> writes:

Show 21 quoted lines

> On 4/13/2020 6:21 PM, Junio C Hamano wrote:
>> Taylor Blau <me@ttaylorr.com> writes:
>> 
>>> Hmm. I'm not crazy about a library function looking at 'GIT_TEST_*'
>>> environment variables and potentially ignoring its arguments, but given
>>> the discussion we had in v1, I don't feel strongly enough to recommend
>>> that you change this.
>>>
>>> For what it's worth, I think that 'write_commit_graph' could behave more
>>> purely if callers checked 'GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS' and set
>>> 'flags' appropriately from the outside,...
>> 
>> Yeah, I agree that it would be a lot cleaner if that is easy to
>> arrange.  I have a suspicion that Derrick already tried and the
>> resulting code looked messier and was discarded?
>
> Perhaps I could fix both concerns by
>
> 1. Use a macro instead of a library call.
>
> 2. Check the _CHANGED_PATHS variable in the macro.

I am not sure how use of a macro "fixes" purity, though. And what is the other concern?

How widely would this "if we are testing, write out the graph file" call be sprinkled over the codebase? I am hoping that it won't be "everywhere", but only at strategic places (like "just once before we leave a subcommand that creates one or bunch of commits"). And how often would they be called? I am also hoping that it won't be "inside a tight loop". In short, I am wondering if we can promise our codebase that

 - git_test_write_commit_graph_or_die() calls won't be an eyesore
   (and/or distraction) for developers too much.

 - git_env_bool() call won't have performance impact.

Show 15 quoted lines

> The macro would look like this:
>
>
> #define git_test_write_commit_graph_or_die() \
> 	if (git_env_bool(GIT_TEST_COMMIT_GRAPH, 0)) { \
> 		int flags = 0; \
> 		if (git_env_bool(GIT_TEST_COMMIT_GRAPH_CHANGED_PATHS, 0)) \
> 			flags = COMMIT_GRAPH_WRITE_BLOOM_FILTERS; \
> 		if (write_commit_graph_reachable( \
> 			the_repository->objects->odb, flags, NULL)) \
> 			die("failed to write commit-graph under GIT_TEST_COMMIT_GRAPH"); \
> 	}
>
> Thanks,
> -Stolee