From: Derrick Stolee <stolee@gmail.com>
Date: Tue, 21 Aug 2018 18:39:57 GMT
Subject: Re: [PATCH v2 5/8] commit-graph: not compatible with replace objects
Message-ID: <78b20554-1522-9661-cb29-09ffd438b206@gmail.com>
In-Reply-To: <xmqqa7pfaa15.fsf@gitster-ct.c.googlers.com>

On 8/21/2018 1:45 PM, Junio C Hamano wrote:
> Derrick Stolee <dstolee@microsoft.com> writes:
>
>> Create new method commit_graph_compatible(r) to check if a given
>> repository r is compatible with the commit-graph feature. Fill the
>> method with a check to see if replace-objects exist. Test this
>> interaction succeeds, including ignoring an existing commit-graph and
>> failing to write a new commit-graph. However, we do ensure that
>> we write a new commit-graph by setting read_replace_refs to 0, thereby
>> ignoring the replace refs.
>>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>>   builtin/commit-graph.c  |  4 ++++
>>   commit-graph.c          | 21 +++++++++++++++++++++
>>   replace-object.c        |  2 +-
>>   replace-object.h        |  2 ++
>>   t/t5318-commit-graph.sh | 22 ++++++++++++++++++++++
>>   5 files changed, 50 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/commit-graph.c b/builtin/commit-graph.c
>> index 0bf0c48657..da737df321 100644
>> --- a/builtin/commit-graph.c
>> +++ b/builtin/commit-graph.c
>> @@ -120,6 +120,8 @@ static int graph_read(int argc, const char **argv)
>>   	return 0;
>>   }
>>   
>> +extern int read_replace_refs;
>> +
> Why do you need this (and also in commit-graph.c)?  I thought
> cache.h includes it, which you can just make use of it.


You're right. I don't know how I missed that.


>> +static int commit_graph_compatible(struct repository *r)
>> +{
>> +	if (read_replace_refs) {
>> +		prepare_replace_object(r);
>> +		if (hashmap_get_size(&r->objects->replace_map->map))
>> +			return 0;
>> +	}
>> +
>> +	return 1;
>> +}
>> diff --git a/replace-object.c b/replace-object.c
>> index 3c17864eb7..9821f1477e 100644
>> --- a/replace-object.c
>> +++ b/replace-object.c
>> @@ -32,7 +32,7 @@ static int register_replace_ref(struct repository *r,
>>   	return 0;
>>   }
>>   
>> -static void prepare_replace_object(struct repository *r)
>> +void prepare_replace_object(struct repository *r)
>>   {
>>   	if (r->objects->replace_map)
>>   		return;
> The way the new caller is written, I am wondering if this function
> should return "we are (or, are not) using the replace object
> feature", making it unnecessary for callers on the reading side to
> know about "read-replace-refs" external variable, for example.
>
> 	/*
> 	 * To be called on-demand from codepaths that want to make
> 	 * sure that replacement objects can be found as necessary.
> 	 *
> 	 * Return number of replacement defined for the repository, or
> 	 * -1 when !read_replace_refs tells us not to use replacement
> 	 * mechanism at all.
> 	 */
> 	int prepare_replace_object(struct repository *r)
> 	{
> 		if (!read_replace_refs)
> 			return -1;
>
> 		if (!r->objects->replace_map) {
> 			r->objects->replace_map =
> 				xmalloc(...);
> 			oidmap_init(r->objects->replace_map, 0);
> 			for_each_refplace_ref(r, register_...);
> 		}
> 		return hashmap_get_size(&r->objects->replace_map->map);
> 	}

This is a good idea. I can incorporate it into v3.

> Then, the caller side can simply become something like:
>
> 	#define cgraph_compat(r) (prepare_replace_object(r) <= 0)

With the next two patches that add more conditions to 
commit_graph_compatible(), I'd prefer keeping it a method instead of a 
macro.

> There are various writers to read_replace_refs variable, but I think
> they should first be replaced with calls to something like:
>
> 	void use_replace_refs(struct repository *r, int enable);
>
> which allows us to hide the global variable and later make it per
> repository if we wanted to.

I will incorporate this into v3 as well.

Thanks,

-Stolee