Re: [PATCH v2 5/8] commit-graph: not compatible with replace objects
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Aug 21, 2018, 18:39 UTC
- 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:
Show 31 quoted lines
> 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.
Show 49 quoted lines
>> +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.
Show 7 quoted lines
> 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