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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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

> From: Han-Wen Nienhuys <hanwen@google.com>
> [...]
> @@ -3059,18 +3059,18 @@ static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
>  	if ((*cb->should_prune_fn)(ooid, noid, email, timestamp, tz,
>  				   message, policy_cb)) {
>  		if (!cb->newlog)
> -			printf("would prune %s", message);
> +			printf("would prune %s\n", message);
>  		else if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
> -			printf("prune %s", message);
> +			printf("prune %s\n", message);
>  	} else {
>  		if (cb->newlog) {
> -			fprintf(cb->newlog, "%s %s %s %"PRItime" %+05d\t%s",
> -				oid_to_hex(ooid), oid_to_hex(noid),
> -				email, timestamp, tz, message);
> +			fprintf(cb->newlog, "%s %s %s %" PRItime " %+05d\t%s\n",
> +				oid_to_hex(ooid), oid_to_hex(noid), email,
> +				timestamp, tz, message);
>  			oidcpy(&cb->last_kept_oid, noid);
>  		}
>  		if (cb->flags & EXPIRE_REFLOGS_VERBOSE)
> -			printf("keep %s", message);
> +			printf("keep %s\n", message);
>  	}
>  	return 0;
>  }

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.