Re: [PATCH v2 5/8] commit-graph: not compatible with replace objects

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 21, 2018, 17:45 UTC
Message-ID: <xmqqa7pfaa15.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180820182359.165929-6-dstolee@microsoft.com>

Derrick Stolee <dstolee@microsoft.com> writes:

Show 27 quoted lines

> 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.

Show 10 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;
> +}

Show 13 quoted lines

> 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.