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