Re: [PATCH 2/4] refs: trim newline from reflog message

On Mon, Nov 22 2021, Han-Wen Nienhuys via GitGitGadget wrote:

I haven't looked deeply into this topic as a whole, but FWIW this conflicts with a topic I've got locally and was going to submit sometime after the current batch of my own ref fixes in "next".

That we've got verbose output at all in file-backend.c is a wart, and it should just be moved out if it entirely, this commit (stacked on top of various other fixes I've got in the area) does that: https://github.com/avar/git/commit/eff40d2d81b

With that change the reftable code will need to handle far less of this, as it'll be handled in builtin/reflog.c, it just needs to behave properly in the appropriate "expire reflog?" callback. I.e. the backend tells us "does this expire?", the builtin/reflog.c code consumes that and does any verbose or non-verbose (or progress etc.) output.

Now, the merge conflict between that and this looks rather trivial, but I can't help but wonder if we're going in the wrong direction here API-wise, or maybe "wrong direction" is too strong, but "sticking with the wrong patterN?".

I think your cleanup works, but wouldn't a better thing be to move this to callbacks rather than tweaking the fprintf formats?

I.e. in my version the whole body of this function has become:
	
	static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
				     const char *email, timestamp_t timestamp, int tz,
				     const char *message, void *cb_data)
	{
		struct expire_reflog_cb *cb = cb_data;
		reflog_expiry_should_prune_fn *fn = cb->should_prune_fn;
	
		if (cb->rewrite)
			ooid = &cb->last_kept_oid;
	
		if (fn(ooid, noid, email, timestamp, tz, message, cb->policy_cb))
			return 0;
	
		if (cb->dry_run)
			return 0; /* --dry-run */
	
		fprintf(cb->newlog, "%s %s %s %"PRItime" %+05d\t%s", oid_to_hex(ooid),
			oid_to_hex(noid), email, timestamp, tz, message);
		oidcpy(&cb->last_kept_oid, noid);
	
		return 0;
	}

The only file-backend specific part of it is that fprintf(). If we moved towards making that part of it be:

	write_reflog_entry_cb(oid_to_hex(ooid), oid_to_hex(noid), email, timestamp, tz, message);

Then we could lift the whole of this API to a level that makes more sense for a backed to implement.

The refs/files-backend.c shouldn't need to have one function calll the "should_prune_fn" *and* write out the data. Instead some code common to all backends should call the "should prune?", and then call the backend's "here's an entry for you to write" callback.

But maybe I'm overthinking this whole thing. I'm just wondering if we have say a sqlite reflog backend as opposed to the file/reftable backend that wants to store this data in a schema. Such a a backend would need to unpack the data, as we're sprintf() formatting function parameters before it gets passed to the backends.