Re: [PATCH v5 5/6] refs_update_symref: add create_only option
From: Junio C Hamano <hidden>
Date: 2024-10-09 21:37:08
Bence Ferdinandy [off-list ref] writes:
quoted hunk
diff --git a/builtin/branch.c b/builtin/branch.c index 6c87690b58..3c9bc39800 100644 --- a/builtin/branch.c +++ b/builtin/branch.c@@ -559,7 +559,7 @@ static int replace_each_worktree_head_symref(struct worktree **worktrees, continue; refs = get_worktree_ref_store(worktrees[i]); - if (refs_update_symref(refs, "HEAD", newref, logmsg, NULL)) + if (refs_update_symref(refs, "HEAD", newref, logmsg, NULL, false)) ret = error(_("HEAD of working tree %s is not updated"), worktrees[i]->path); }
Didn't somebody tell you not to use "bool" in previous rounds of
reviews?
We are still smoking out platforms that have trouble with the type
by using it only in a tightly controlled weather balloon we raised
in 8277dbe9 (git-compat-util: convert skip_{prefix,suffix}{,_mem} to
bool, 2023-12-16), if I recall correctly.
Assuming either (1) I forgot that we are OK with bool everywhere, or
(2) the patch will be updated to use plain 0 for false and
everything else for true, let's keep reading. I as a reader expect
that some *.h file will tell us what the new parameter means.
quoted hunk
diff --git a/refs.c b/refs.c index 91cacee6f9..3d2c07dd67 100644 --- a/refs.c +++ b/refs.c@@ -2115,19 +2115,32 @@ 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, - struct strbuf *before_target) + struct strbuf *before_target, bool create_only) { struct ref_transaction *transaction; struct strbuf err = STRBUF_INIT; - int ret = 0; + int ret = 0, create_ret = 0;
Isn't it sufficient for create_ret to be in the scope deep in the nested if/else where it is used?
transaction = ref_store_transaction_begin(refs, &err);
+ if (create_only) {
+ if (!transaction ||
+ ref_transaction_create(transaction, ref, NULL, target,
+ REF_NO_DEREF, logmsg, &err)) {
+ ret = error("%s", err.buf);
+ }
+ else {
+ create_ret = ref_transaction_commit(transaction, &err);
+ if (create_ret && create_ret != TRANSACTION_CREATE_EXISTS)
+ ret = error("%s", err.buf);
+ }
+ }
+ else
+ if (!transaction ||
+ ref_transaction_update(transaction, ref, NULL, NULL,
+ target, NULL, REF_NO_DEREF,
+ logmsg, &err) ||
+ ref_transaction_commit(transaction, &err)) {
+ ret = error("%s", err.buf);
}
Style: in if/else if/... cascade, opening and closing {braces} sit
on the same line as these keywords, i.e.
if (create_only) {
if (...) {
ret = error(...);
} else {
...
}
} else if (!transaction ||
...) {
}
But more importantly, I think it is much cleaner to handle failure
from transaction_begin outside the if/else cascade. i.e.
transaction = ref_store_transaction_begin(refs, &err);
if (!transaction) {
error_return:
ret = error("%s", err.buf);
goto cleanup;
}
if (create_only) {
int create_ret;
if (ref_transaction_create(...))
goto error_return;
create_ret = ref_transaction_commit(...);
if (create_ret && create_ret != TRANSACTION_CREATE_EXISTS)
goto error_return;
} else {
if (ref_transaction_update(...) ||
ref_transaction_commit(...))
goto error_return;
}
cleanup:
strbuf_release(&err);
if (transaction) {
if (before_target && ...)
strbuf_addstr(before_target, ...);
ref_transaction_free(transaction);
}
return ret;
I didn't spot any caller that passes "true" to signal that it wants
the create_only behaviour in this step.
Which is OK---let's keep reading to find such a caller in the next
step.