| Message ID | 20241023153736.257733-3-bence@ferdinandy.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | set-head/fetch remote/HEAD updates | expand |
Bence Ferdinandy <bence@ferdinandy.com> writes: > int refs_update_symref(struct ref_store *refs, const char *ref, > const char *target, const char *logmsg) > +{ > + return refs_update_symref_extended(refs, ref, target, logmsg, NULL); > +} OK. As the enhanced and renamed function is also external, we do not have to worry about reordering the old one to come after the new one. > +int refs_update_symref_extended(struct ref_store *refs, const char *ref, > + const char *target, const char *logmsg, > + struct strbuf *referent) > { > struct ref_transaction *transaction; > struct strbuf err = STRBUF_INIT; > @@ -2122,13 +2129,20 @@ int refs_update_symref(struct ref_store *refs, const char *ref, > > transaction = ref_store_transaction_begin(refs, &err); > if (!transaction || > - ref_transaction_update(transaction, ref, NULL, NULL, > + ref_transaction_update(transaction, ref, NULL, NULL, An unwanted patch noise? > target, NULL, REF_NO_DEREF, > logmsg, &err) || > - ref_transaction_commit(transaction, &err)) { > + ref_transaction_prepare(transaction, &err)) { Likewise, but the noise distracts from the real change made on this line, which is even worse. The real change here is to only call _prepare(), which also asks the transaction hook if we are OK to proceed. If we fail, we stop here > ret = error("%s", err.buf); > + goto cleanup; This is also a real change. We could instead make the additional code below into the else clause (see below). > } > + if (referent) > + refs_read_symbolic_ref(refs, ref, referent); And if we were asked to give the value of the symbolic ref, we make this call. What should this code do when reading fails (I know it ignores, as written, but I am asking what it _should_ do)? > + if (ref_transaction_commit(transaction, &err)) > + ret = error("%s", err.buf); And then we commit, or we fail to commit. > +cleanup: We could write the whole thing as a single "do these and leave as soon as we see any failure" ||-cascade, if (!(transaction = ref_store_transaction_begin(refs, &err)) || ref_transaction_update(transaction, ref, NULL, NULL, target, NULL, REF_NO_DEREF, logmsg, &err) || ref_transaction_prepare(transaction, &err)) || (referent ? refs_read_symbolic_ref(refs, ref, referent) : 0) || ref_transaction_commit(transaction, &err)) { if (!err.len) ... stuff default error message to err ...; ret = error("%s", err.buf); } which may not necessarily easier to follow (and in fact it is rather ugly), but at least, the resulting code does not have to special case the "optionally peek into the symref" step.
diff --git a/refs.c b/refs.c index 5f729ed412..24a4172cd2 100644 --- a/refs.c +++ b/refs.c @@ -2115,6 +2115,13 @@ int peel_iterated_oid(struct repository *r, const struct object_id *base, struct int refs_update_symref(struct ref_store *refs, const char *ref, const char *target, const char *logmsg) +{ + return refs_update_symref_extended(refs, ref, target, logmsg, NULL); +} + +int refs_update_symref_extended(struct ref_store *refs, const char *ref, + const char *target, const char *logmsg, + struct strbuf *referent) { struct ref_transaction *transaction; struct strbuf err = STRBUF_INIT; @@ -2122,13 +2129,20 @@ int refs_update_symref(struct ref_store *refs, const char *ref, transaction = ref_store_transaction_begin(refs, &err); if (!transaction || - ref_transaction_update(transaction, ref, NULL, NULL, + ref_transaction_update(transaction, ref, NULL, NULL, target, NULL, REF_NO_DEREF, logmsg, &err) || - ref_transaction_commit(transaction, &err)) { + ref_transaction_prepare(transaction, &err)) { ret = error("%s", err.buf); + goto cleanup; } + if (referent) + refs_read_symbolic_ref(refs, ref, referent); + if (ref_transaction_commit(transaction, &err)) + ret = error("%s", err.buf); + +cleanup: strbuf_release(&err); if (transaction) ref_transaction_free(transaction); @@ -2948,4 +2962,3 @@ int ref_update_expects_existing_old_ref(struct ref_update *update) return (update->flags & REF_HAVE_OLD) && (!is_null_oid(&update->old_oid) || update->old_target); } - diff --git a/refs.h b/refs.h index 108dfc93b3..259191a485 100644 --- a/refs.h +++ b/refs.h @@ -573,6 +573,10 @@ int refs_copy_existing_ref(struct ref_store *refs, const char *oldref, int refs_update_symref(struct ref_store *refs, const char *refname, const char *target, const char *logmsg); +int refs_update_symref_extended(struct ref_store *refs, const char *refname, + const char *target, const char *logmsg, + struct strbuf *referent); + enum action_on_err { UPDATE_REFS_MSG_ON_ERR, UPDATE_REFS_DIE_ON_ERR,
When updating a symref with update_symref it's currently not possible to know for sure what was the previous value that was overwritten. Extend refs_update_symref under a new function name, to record the value after the ref has been locked if the caller of refs_update_symref_extended requests it via a new variable in the function call. Keep the original refs_update_symref function with the same signature, but now as a wrapper around refs_update_symref_extended. Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com> --- Notes: v4: new patch v5: - added before_target to reftables backend - added an extra safety check for transaction's existence in refs.c v6: - no change v7: - remove the whole before_target concept from the backends and handle checking it in refs.c instead (thanks Karthik) - rename the before_target to referent which is how the same concept is called in the backends - change commit prefix to be more in line with project standards v8: no change v9: - instead of adding parameters to refs_update_symref, rename what was in v8 as refs_update_symref_extended and make refs_update_symref a wrapper for that. This significantly reduces the number of files that need to be touched, and avoids adding a lot of dummy NULL-s in unrelated places. v10: no change v11: no change v12: no change refs.c | 19 ++++++++++++++++--- refs.h | 4 ++++ 2 files changed, 20 insertions(+), 3 deletions(-)