Re: [PATCH 5/7] refs: introduce the `ref_transaction_update_reflog` function
From: Patrick Steinhardt <hidden>
Date: 2024-12-11 14:26:37
On Mon, Dec 09, 2024 at 12:07:19PM +0100, Karthik Nayak wrote:
quoted hunk ↗ jump to hunk
diff --git a/refs.c b/refs.c index 732c236a3fd0cf324cc172b48d3d54f6dbadf4a4..602a65873181a90751def525608a7fa7bea59562 100644 --- a/refs.c +++ b/refs.c@@ -1160,13 +1160,15 @@ void ref_transaction_free(struct ref_transaction *transaction) free(transaction); } -struct ref_update *ref_transaction_add_update( - struct ref_transaction *transaction, - const char *refname, unsigned int flags, - const struct object_id *new_oid, - const struct object_id *old_oid, - const char *new_target, const char *old_target, - const char *msg) +struct ref_update *ref_transaction_add_update(struct ref_transaction *transaction, + const char *refname, + unsigned int flags, + const struct object_id *new_oid, + const struct object_id *old_oid, + const char *new_target, + const char *old_target, + const char *committer_info, + const char *msg) { struct ref_update *update;
I'd personally avoid reindenting this block. It's somewhat-common practice to not align all arguments with the opening brace when the line would become too long. The reindents also distract a bit from the actual changes done in other places further down.
quoted hunk ↗ jump to hunk
@@ -1190,8 +1192,15 @@ struct ref_update *ref_transaction_add_update( oidcpy(&update->new_oid, new_oid); if ((flags & REF_HAVE_OLD) && old_oid) oidcpy(&update->old_oid, old_oid); - if (!(flags & REF_SKIP_CREATE_REFLOG)) + if (!(flags & REF_SKIP_CREATE_REFLOG)) { + if (committer_info) { + struct strbuf sb = STRBUF_INIT; + strbuf_addstr(&sb, committer_info); + update->committer_info = strbuf_detach(&sb, NULL);
Can't we simplify this via `xstrdup()`?
quoted hunk ↗ jump to hunk
@@ -3080,10 +3081,12 @@ static int files_transaction_finish_initial(struct files_ref_store *refs, } /* - * packed-refs don't support symbolic refs and root refs, so we - * have to queue these references via the loose transaction. + * packed-refs don't support symbolic refs, root refs and reflogs, + * so we have to queue these references via the loose transaction. */ - if (update->new_target || is_root_ref(update->refname)) { + if (update->new_target || + is_root_ref(update->refname) || + (update->flags & REF_LOG_ONLY)) { if (!loose_transaction) { loose_transaction = ref_store_transaction_begin(&refs->base, 0, err); if (!loose_transaction) {
Makes sense. While we already had REF_LOG_ONLY beforehand, it was only used in very specific cases and thus the support implemented by the backends is lacking. And given that the packed-ref backend does not support reflogs we have to queue these up via the loose backend. Patrick