On Tue, Nov 12, 2024 at 03:41:48PM +0900, Junio C Hamano wrote:

Patrick Steinhardt [off-list ref] writes:

quoted

-	ret = read_ref_without_reload(refs, stack, refname, oid, referent, type);
+	ret = read_ref_without_reload(refs, be->stack, refname, oid, referent, type);

The following bit is curious.

quoted

+	ret = backend_for(&be, refs, update->refname, NULL, 0);
+	if (ret)
+		return ret;
+

We locate one without reloading, and ...

quoted

 	/*
 	 * Search for a preexisting stack update. If there is one then we add
 	 * the update to it, otherwise we set up a new stack update.
 	 */
 	for (i = 0; !arg && i < tx_data->args_nr; i++)
-		if (tx_data->args[i].stack == stack)
+		if (tx_data->args[i].be == be)
 			arg = &tx_data->args[i];
 	if (!arg) {

... only when we cannot reuse preexisting one, ...

quoted

 		struct reftable_addition *addition;
 
-		ret = reftable_stack_reload(stack);
+		ret = backend_for(&be, refs, update->refname, NULL, 1);
 		if (ret)
 			return ret;

... instead of directly doing reload on the instance we already
have, we do another _for() to locate one, this time reload set to 1.

That looks like doing some redundant work?  I am confused.

It indeed is redundant work, yes. And in fact it is redundant work that
isn't really required anymore. My first iteration didn't yet have the
`reftable_write_options::on_reload()` callback, and I instead tried to
catch reloads via `backend_for()`, so it was required to reload via that
function.

But now that we do have the callback that isn't needed anymore, and thus
we don't have to call `backend_for()` a second time here. I'll adapt
this accordingly.

Patrick