Bence Ferdinandy <bence@xxxxxxxxxxxxxx> 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.
