Derrick Stolee <dstolee@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> ---
> 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.
> +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);
}
Then, the caller side can simply become something like:
#define cgraph_compat(r) (prepare_replace_object(r) <= 0)
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.
