Re: [PATCH v2 10/11] maintenance: add auto condition for commit-graph task

From: Derrick Stolee <stolee@gmail.com>
Date: Aug 19, 2020, 15:15 UTC
Message-ID: <8a9f5ca0-73b2-b9f8-b342-a8fff8c938be@gmail.com>
In-Reply-To: <20200819000947.2844392-1-jonathantanmy@google.com>

On 8/18/2020 8:09 PM, Jonathan Tan wrote:

Show 9 quoted lines

>> Instead of writing a new commit-graph in every 'git maintenance run
>> --auto' process (when maintenance.commit-graph.enalbed is configured to
> 
> s/enalbed/enabled/
> 
>> +/* Remember to update object flag allocation in object.h */
>> +#define PARENT1		(1u<<16)
> 
> Why this name? "SEEN" seems perfectly fine.

For some reason I thought that there might be issues using SEEN (1u<<0) but trying it locally does not seem to be a problem. I think I've just been bit by how revision.c uses it a bit too often. The use here is independent enough to not cause problems.

Show 28 quoted lines

>> +static int num_commits_not_in_graph = 0;
>> +static int limit_commits_not_in_graph = 100;
>> +
>> +static int dfs_on_ref(const char *refname,
>> +		      const struct object_id *oid, int flags,
>> +		      void *cb_data)
>> +{
> 
> [snip]
> 
>> +static int should_write_commit_graph(void)
>> +{
>> +	int result;
>> +
>> +	git_config_get_int("maintenance.commit-graph.auto",
>> +			   &limit_commits_not_in_graph);
>> +
>> +	if (!limit_commits_not_in_graph)
>> +		return 0;
>> +	if (limit_commits_not_in_graph < 0)
>> +		return 1;
>> +
>> +	result = for_each_ref(dfs_on_ref, NULL);
> 
> I don't like introducing the mutable global num_commits_not_in_graph
> especially when there seems to be at least one easy alternative (e.g.
> putting it in cb_data) but I know that this is a pattern than existing
> code.

I should have done this right in the first place. Thanks for catching it.

-Stolee