Re: [PATCH v5 5/6] refs_update_symref: add create_only option

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 9, 2024, 21:37 UTC
Message-ID: <xmqqiku1ot4u.fsf@gitster.g>
In-Reply-To: <20241009135747.3563204-5-bence@ferdinandy.com>

Bence Ferdinandy <bence@ferdinandy.com> writes:

Show 13 quoted lines

> 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.

Show 15 quoted lines

> 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?

Show 21 quoted lines

>  	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.