Re: [PATCH 1/4] refs: add referent parameter to refs_resolve_ref_unsafe

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 7, 2024, 15:21 UTC
Message-ID: <xmqqfrtohj80.fsf@gitster.g>
In-Reply-To: <7FC6AD35-0DF4-4228-8708-790470D4B0EA@gmail.com>

John Cai <johncai86@gmail.com> writes:

> Yes, so for 3/4 I was exploring doing the same thing. However, each_repo_fn goes
> pretty deep in the callstack and to provide an alternate set of functions that
> use something like each_repo_referent_fn would still lead to a relatively large
> blast radius, eg, something like:

Hmph. You only care about teaching referent to calls to refs_for_each_ref_in() and friends in apply_ref_filter() and nowhere else. So for example, to preserve the current calling pattern for this pair of caller/callback (and you have dozens more exactly like this one you touch in [3/4]):

	static int register_ref(const char *refname,
                                const struct object_id *oid,
				int flags,
				void *cb_data);

	static int read_bisect_refs(void)
	{
		return refs_for_each_ref_in(get_main_ref_store(the_repository),
					    "refs/bisect", register_ref, NULL);
	}

you could arrange your _new_ API the way you want, e.g.,

	typedef int each_ref_with_referent_fn(const char *refname,
	                                      const char *referent,
                                              const struct object_id *oid,
					      int flags,
					      void *cb_data);

	int refs_for_each_ref_with_referent_in(struct ref_store *refs,
					       const char *prefix,
                                               each_ref_with_referent_fn fn,
					       void *cb_data);

to help the single caller, i.e., apply_ref_filter(), and rebuild the existing API on top as a thin wrapper, e.g.

	/* This cannot change without butchering existing callers */
	typedef int each_ref_fn(const char *refname,
                                const struct object_id *oid,
				int flags,
				void *cb_data);

	/* Hence we introduce an adapter */
	struct each_ref_fn_adapter_cbdata {
		each_ref_fn user_each_ref_fn;
		void *user_cb_data;
	};

	/* This is designed to work as an each_ref_with_referent_fn */
	static int each_ref_adapter_fn(const char *refname,
				       const char *referent,
                                       const struct object_id *oid,
				       int flags,
				       void *cb_data)
	{
		struct each_ref_fn_adapter_cbdata *adapter_cbdata = cbdata;

		/* the callers have no need for referent */
                return adapter_cbdata->user_each_ref_fn(refname, oid, flags,
					                adapter_cbdata->user_cbdata);
	}

	/*
         * The function signature must stay the same to help existing,
         * callers, but the implementation is now a thin wrapper.
	 */
	int refs_for_each_ref_in(struct ref_store *refs,
				 const char *prefix,
                                 each_ref_fn fn,
				 void *cb_data)
        {
		struct each_ref_fn_adapter_cbdata adapter_cbdata = {
                	.user_each_ref_fn = fn,
			.user_cb_data = cb_data,
		};
		return refs_for_each_ref_with_referetnt_in(refs, prefix,
							   each_ref_adapter_fn,
                                                           &adapter_cbdata);
	}

no?

You'd need to pass through the new parameter "referent" through the code paths that implement refs_for_each_ref_in() and friends to update them to refs_for_each_ref_with_referent_in() and friends no matter what, but there are limited number of the top-level refs_for_each_ref_in() and friends that are unaware of the "referent", and each of them would need the above ~20 lines (couting the comment) adapter function that all can share the single each_ref_adapter_fn() callback function.

Or am I missing some other intricacy in the existing for-each-* API?